mm, fs: warn on missing address space operations

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

...

mm/truncate.c +0 -0

@@ -40,8 +40,14 @@ void do_invalidatepage(struct page *page
 	void (*invalidatepage)(struct page *, unsigned long);
 	invalidatepage = page->mapping->a_ops->invalidatepage;

Quoted 11 lines · On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <[email protected]> wrote:

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

I guess this is one way of waking people up.

What happens is that hundreds of bug reports land in my inbox and I get to route them to various maintainers, most of whom don't exist, so warnings keep on landing in my inbox.  Please send a mailing address for my invoices.

It would be more practical, more successful and quicker to hunt down the miscreants and send them rude emails.  Plus it would save you money.

Quoted 8 lines

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

...

mm/truncate.c +0 -0

@@ -40,8 +40,14 @@ void do_invalidatepage(struct page *page
       void (*invalidatepage)(struct page *, unsigned long);
       invalidatepage = page->mapping->a_ops->invalidatepage;

mm/truncate.c void do_invalidatepage(struct page *page · line 50

Quoted 10 lines

mm/truncate.c +7 -1

  #ifdef CONFIG_BLOCK
-     if (!invalidatepage)
+     if (!invalidatepage) {
+             static bool warned = false;
+             if (!warned) {
+                     warned = true;
+                     print_symbol("address_space_operations %s missing invalidatepage method. Use block_invalidatepage.\n", (unsigned long)page->mapping->a_ops);
+             }
               invalidatepage = block_invalidatepage;
+     }

erk, I realise 80 cols can be a pain, but 165 cols is just out of bounds.  Why not

       /* this fs should use block_invalidatepage() */ WARN_ON_ONCE(!invalidatepage);

Quoted 11 lines · On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <[email protected]> wrote:

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

I guess this is one way of waking people up.

What happens is that hundreds of bug reports land in my inbox and I get to route them to various maintainers, most of whom don't exist, so warnings keep on landing in my inbox. Please send a mailing address for my invoices.

It would be more practical, more successful and quicker to hunt down the miscreants and send them rude emails. Plus it would save you money.

Quoted 9 lines

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

...

mm/truncate.c +0 -0

@@ -40,8 +40,14 @@ void do_invalidatepage(struct page *page
 	void (*invalidatepage)(struct page *, unsigned long);
 	invalidatepage = page->mapping->a_ops->invalidatepage;

mm/truncate.c void do_invalidatepage(struct page *page · line 50

Quoted 10 lines

mm/truncate.c +7 -1

 #ifdef CONFIG_BLOCK
-	if (!invalidatepage)
+	if (!invalidatepage) {
+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing invalidatepage method. Use block_invalidatepage.\n", (unsigned long)page->mapping->a_ops);
+		}
 		invalidatepage = block_invalidatepage;
+	}

erk, I realise 80 cols can be a pain, but 165 cols is just out of bounds. Why not

	/* this fs should use block_invalidatepage() */
	WARN_ON_ONCE(!invalidatepage);

Quoted 3 lines

	/* this fs should use block_invalidatepage() */
	WARN_ON_ONCE(!invalidatepage);

Problem is that it doesn't give you the aop name (and call trace probably won't help).

Quoted 6 lines · On Mon, 22 Mar 2010 21:40:57 +1100 Nick Piggin <[email protected]> wrote:

Quoted 3 lines

	/* this fs should use block_invalidatepage() */
	WARN_ON_ONCE(!invalidatepage);

Problem is that it doesn't give you the aop name (and call trace probably won't help).

Yes it does - you have the filename and line number. You go there and read "invalidatepage". And the backtrace identifies the filesystem.

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

Index: linux-2.6/mm/filemap.c
===================================================================

mm/filemap.c +0 -0

@@ -2472,7 +2472,14 @@ int try_to_release_page(struct page *pag
 

mm/filemap.c +1 -0

+	}
 }
 
 EXPORT_SYMBOL(try_to_release_page);

Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +0 -0

@@ -1159,8 +1159,14 @@ int set_page_dirty(struct page *page)
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;

mm/page-writeback.c +0 -0

 #endif
 		return (*spd)(page);
 	}

Index: linux-2.6/mm/truncate.c
===================================================================

mm/truncate.c +9 -3

@@ -16,8 +16,8 @@
 #include <linux/highmem.h>
 #include <linux/pagevec.h>
 #include <linux/task_io_accounting_ops.h>
-#include <linux/buffer_head.h>	/* grr. try_to_release_page,
-				   do_invalidatepage */
+#include <linux/buffer_head.h>	/* block_invalidatepage */
+#include <linux/kallsyms.h>
 #include "internal.h"
 
 

@@ -40,8 +40,14 @@ void do_invalidatepage(struct page *page
 	void (*invalidatepage)(struct page *, unsigned long);
 	invalidatepage = page->mapping->a_ops->invalidatepage;
 #ifdef CONFIG_BLOCK
-	if (!invalidatepage)
+	if (!invalidatepage) {
+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing invalidatepage method. Use block_invalidatepage.\n", (unsigned long)page->mapping->a_ops);
+		}
 		invalidatepage = block_invalidatepage;
+	}
 #endif
 	if (invalidatepage)
 		(*invalidatepage)(page, offset);

Index: linux-2.6/fs/inode.c
===================================================================

fs/inode.c +1 -0

@@ -25,6 +25,7 @@
 #include <linux/mount.h>
 #include <linux/async.h>
 #include <linux/posix_acl.h>
+#include <linux/kallsyms.h>
 
 /*
  * This is needed for the following functions:

fs/inode.c +6 -0

+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing release method. Use invalidate_inode_buffers.\n", (unsigned long)mapping->a_ops);
+		}
 		invalidate_inode_buffers(inode);
+	}
 
 	BUG_ON(mapping_has_private(mapping));
 	BUG_ON(mapping->nrpages);

fs/inode.c +13 -2

+			static bool warned = false;
+			if (!warned) {
+				warned = true;
+				print_symbol("address_space_operations %s missing release method. Using invalidate_inode_buffers.\n", (unsigned long)mapping->a_ops);
+			}
 			invalidate_inode_buffers(inode);
-		else
+		} else
 			mapping->a_ops->release(mapping, AOP_RELEASE_FORCE);
 		BUG_ON(mapping_has_private(mapping));
 		if (!atomic_read(&inode->i_count)) {

@@ -497,8 +509,14 @@ static void prune_icache(int nr_to_scan)
 			spin_unlock(&inode_lock);
 			if (mapping->a_ops->release)
 				ret = mapping->a_ops->release(mapping, 0);
-			else
+			else {
+				static bool warned = false;
+				if (!warned) {
+					warned = true;
+					print_symbol("address_space_operations %s missing release method. Using remove_inode_buffers.\n", (unsigned long)mapping->a_ops);
+				}
 				ret = !remove_inode_buffers(inode);
+			}
 			if (ret)
 				reap += invalidate_mapping_pages(&inode->i_data,
 								0, -1);

Index: linux-2.6/fs/libfs.c
===================================================================

fs/libfs.c +1 -0

@@ -11,6 +11,7 @@
 #include <linux/exportfs.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/kallsyms.h>
 
 #include <asm/uaccess.h>
 

Quoted 12 lines · Nick Piggin

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

Quoted 3 lines

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

Guilty as charged.

It could be nice if the first thing this patch will
do is: Add a comment at struct address_space_operations that states that all operations should be defined and perhaps what are the minimum defaults for some of these, as below. I do try to be good, I was under the impression that defaults are OK.

Below are changes to exofs as proposed by this mail. Please comment? I will push such a patch to exofs later.

Quoted 7 lines

Index: linux-2.6/mm/filemap.c
===================================================================

mm/filemap.c +0 -0

@@ -2472,7 +2472,14 @@ int try_to_release_page(struct page *pag
 

mm/filemap.c int try_to_release_page(struct page *pag · line 2481

Quoted 10 lines

mm/filemap.c +7 -1

 	if (mapping && mapping->a_ops->releasepage)
 		return mapping->a_ops->releasepage(page, gfp_mask);
-	return try_to_free_buffers(page);
+	else {
+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing releasepage method. Use try_to_free_buffers.\n", (unsigned long)page->mapping->a_ops);
+		}
+		return try_to_free_buffers(page);

This is not nice! we should have an export that looks like:

int default_releasepage(struct page *page, gfp_t gfp)

{
	return try_to_free_buffers(page);
}

otherwise 3ton FSes would have to define the same thing all over again

Quoted 16 lines

mm/filemap.c +1 -0

+	}
 }
 
 EXPORT_SYMBOL(try_to_release_page);

Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +2 -1

@@ -1159,8 +1159,14 @@ int set_page_dirty(struct page *page)
 	if (likely(mapping)) {
 		int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
 #ifdef CONFIG_BLOCK
-		if (!spd)
+		if (!spd) {
+			static bool warned = false;

mm/page-writeback.c int set_page_dirty(struct page *page) · line 1169

Quoted 6 lines

mm/page-writeback.c +5 -0

+			if (!warned) {
+				warned = true;
+				print_symbol("address_space_operations %s missing set_page_dirty method. Use __set_page_dirty_buffers.\n", (unsigned long)page->mapping->a_ops);
+			}
 			spd = __set_page_dirty_buffers;
+		}

We could do better then __set_page_dirty_buffers for something lots of FSes use

mm/page-writeback.c void clear_inode(struct inode *inode) · line 315

Quoted 6 lines

mm/page-writeback.c +1 -1

@@ -311,8 +312,14 @@ void clear_inode(struct inode *inode)
 
 	might_sleep();
 	/* XXX: filesystems should invalidate this before calling */
-	if (!mapping->a_ops->release)
+	if (!mapping->a_ops->release) {

Where is this code from? As of 2.6.34-rc2 I don't have a release in a_ops->

mm/page-writeback.c int simple_fsync(struct file *file, stru · line 831

Quoted 6 lines

mm/page-writeback.c +1 -1

@@ -827,9 +828,14 @@ int simple_fsync(struct file *file, stru
 	int err;
 	int ret;
 
-	if (!mapping->a_ops->sync)
+	if (!mapping->a_ops->sync) {

And this one is new as well, right?

New added vectors should be added to all in-tree FSes by the submitter.

and a deprecate warning for out of tree FSes (If we care)

mm/page-writeback.c int simple_fsync(struct file *file, stru

Quoted 13 lines

mm/page-writeback.c +6 -2

+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing sync method. Using sync_mapping_buffers.\n", (unsigned long)mapping->a_ops);
+		}
 		ret = sync_mapping_buffers(mapping);
-	else
+	} else
 		ret = mapping->a_ops->sync(mapping, AOP_SYNC_WRITE|AOP_SYNC_WAIT);
 
 	if (!(inode->i_state & I_DIRTY))
--

---
exofs: Add default address_space_operations

All vectors of address_space_operations should be initialized by the filesystem. Add the missing parts.

---
git diff --stat -p -M fs/exofs/inode.c

 fs/exofs/inode.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

fs/exofs/inode.c +5 -0

index a17e4b7..85dd847 100644
@@ -754,6 +754,11 @@ static int exofs_write_end(struct file *file, struct address_space *mapping,
 	return ret;
 }
 
+static int exofs_releasepage(struct page *page, gfp_t gfp)
+{
+	return try_to_free_buffers(page);
+}
+
 const struct address_space_operations exofs_aops = {
 	.readpage	= exofs_readpage,
 	.readpages	= exofs_readpages,

Quoted 22 lines · Boaz Harrosh

---
git diff --stat -p -M fs/exofs/inode.c

 fs/exofs/inode.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

fs/exofs/inode.c +5 -0

index a17e4b7..85dd847 100644
@@ -754,6 +754,11 @@ static int exofs_write_end(struct file *file, struct address_space *mapping,
 	return ret;
 }
 
+static int exofs_releasepage(struct page *page, gfp_t gfp)
+{
+	return try_to_free_buffers(page);
+}
+
 const struct address_space_operations exofs_aops = {
 	.readpage	= exofs_readpage,
 	.readpages	= exofs_readpages,

fs/exofs/inode.c const struct address_space_operations exofs_aops = { · line 772

Quoted 7 lines

fs/exofs/inode.c +3 -0

@@ -761,6 +766,9 @@ const struct address_space_operations exofs_aops = {
 	.writepages	= exofs_writepages,
 	.write_begin	= exofs_write_begin_export,
 	.write_end	= exofs_write_end,
+	.releasepage	= exofs_releasepage,
+	.set_page_dirty	= __set_page_dirty_buffers,
+	.invalidatepage = block_invalidatepage,
 };

AFAIKS, you aren't using buffer heads at all (except nobh_truncate, which will not attach buffers to pages)?

If so, you should only need __set_page_dirty_nobuffers.

Thanks,
Nick

Quoted 12 lines

mm/filemap.c +7 -1

@@ -2472,7 +2472,14 @@ int try_to_release_page(struct page *pag
 
 	if (mapping && mapping->a_ops->releasepage)
 		return mapping->a_ops->releasepage(page, gfp_mask);
-	return try_to_free_buffers(page);
+	else {
+		static bool warned = false;
+		if (!warned) {
+			warned = true;
+			print_symbol("address_space_operations %s missing releasepage method. Use try_to_free_buffers.\n", (unsigned long)page->mapping->a_ops);
+		}
+		return try_to_free_buffers(page);

Review int try_to_release_page(struct page *pag · line 2482

Quoted 1 line

(quoted diff) +1 -0

+	}

I don't think this is correct. We currently also call try_to_free_buffers if the page does not have a mapping, and from conversations with Andrew long time ago that case actually does seem to be nessecary due to behaviour in ext3/jbd. So you really should only warn if there is a mapping to start with. In fact your code will dereference a potential NULL pointer in that case.

And as others said, this patch only makes sense after the existing filesystems are updated to fill out all methods, and for the case of try_to_free_buffers and set_page_dirty until we have suitable and well-named default operations available.

Btw, any reason this doesn't use the %pf specifier to printk instead of dragging in print_symbol? Even better would be to just print the fs type from mapping->host->i_sb->s_type->name.

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

Quoted 12 lines · Nick Piggin

It's ugly and lazy that we do these default aops in case it has not been filled in by the filesystem.

A NULL operation should always mean either: we don't support the operation; we don't require any action; or a bug in the filesystem, depending on the context.

In practice, if we get rid of these fallbacks, it will be clearer what operations are used by a given address_space_operations struct, reduce branches, reduce #if BLOCK ifdefs, and should allow us to get rid of all the buffer_head knowledge from core mm and fs code.

Quoted 2 lines

We could add a patch like this which spits out a recipe for how to fix up filesystems and get them all converted quite easily.

Um. Seeing that part of that is for methods absent in mainline (->release(), ->sync()), I'd say that making it mandatory at that point is a bad idea.

As for the rest... We have 90 instances of address_space_operations in the kernel. Out of those:

	28 have ->releasepage != NULL
	27 have ->set_page_dirty != NULL
	25 have ->invalidatepage != NULL

So I'm not even sure that adding that much boilerplate makes sense.