Re: [rfc][patch] mm, fs: warn on missing address space operations

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Andrew Morton]

On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <npiggin@suse.de> 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.

> 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.
> 
> ...
>
> @@ -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;
> +	}

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);


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[rfc][patch] mm, fs: warn on missing address space operations

[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.

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
===================================================================
--- linux-2.6.orig/mm/filemap.c
+++ linux-2.6/mm/filemap.c
@@ -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);
+	}
 }
 
 EXPORT_SYMBOL(try_to_release_page);
Index: linux-2.6/mm/page-writeback.c
===================================================================
--- linux-2.6.orig/mm/page-writeback.c
+++ linux-2.6/mm/page-writeback.c
@@ -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;
+			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;
+		}
 #endif
 		return (*spd)(page);
 	}
Index: linux-2.6/mm/truncate.c
===================================================================
--- linux-2.6.orig/mm/truncate.c
+++ linux-2.6/mm/truncate.c
@@ -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
===================================================================
--- linux-2.6.orig/fs/inode.c
+++ linux-2.6/fs/inode.c
@@ -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:
@@ -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) {
+		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);
@@ -393,9 +400,14 @@ static int invalidate_list(struct list_h
 		if (inode->i_state & I_NEW)
 			continue;
 		mapping = &inode->i_data;
-		if (!mapping->a_ops->release)
+		if (!mapping->a_ops->release) {
+			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
===================================================================
--- linux-2.6.orig/fs/libfs.c
+++ linux-2.6/fs/libfs.c
@@ -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>
 
@@ -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) {
+		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))

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Pekka Enberg]

On Mon, Mar 22, 2010 at 6:56 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <npiggin@suse.de> 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.
>
>> 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.
>>
>> ...
>>
>> @@ -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;
>> +     }
>
> 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);

/me gets his paint bucket...

How about

    WARN_ONCE(!invalidatepage, "this fs should use block_invalidatepage()")

                           Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Boaz Harrosh]

On 03/22/2010 07:39 AM, Nick Piggin 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.
> 
> 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.

> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c
> +++ linux-2.6/mm/filemap.c
> @@ -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);

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

> +	}
>  }
>  
>  EXPORT_SYMBOL(try_to_release_page);
> Index: linux-2.6/mm/page-writeback.c
> ===================================================================
> --- linux-2.6.orig/mm/page-writeback.c
> +++ linux-2.6/mm/page-writeback.c
> @@ -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;
> +			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

>  #endif
>  		return (*spd)(page);
>  	}
> Index: linux-2.6/mm/truncate.c
> ===================================================================
> --- linux-2.6.orig/mm/truncate.c
> +++ linux-2.6/mm/truncate.c
> @@ -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
> ===================================================================
> --- linux-2.6.orig/fs/inode.c
> +++ linux-2.6/fs/inode.c
> @@ -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:
> @@ -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->

> +		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);
> @@ -393,9 +400,14 @@ static int invalidate_list(struct list_h
>  		if (inode->i_state & I_NEW)
>  			continue;
>  		mapping = &inode->i_data;
> -		if (!mapping->a_ops->release)
> +		if (!mapping->a_ops->release) {

And here

> +			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
> ===================================================================
> --- linux-2.6.orig/fs/libfs.c
> +++ linux-2.6/fs/libfs.c
> @@ -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>
>  
> @@ -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)

> +		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(-)

diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index a17e4b7..85dd847 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -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,
@@ -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,
 };
 
 /******************************************************************************

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Nick Piggin]

On Mon, Mar 22, 2010 at 12:56:10AM -0400, Andrew Morton wrote:
> On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <npiggin@suse.de> 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.

The Linux Foundation
1796 18th Street, Suite C
San Francisco, CA 94107

:)


> 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.

I could do my best at the obvious (and easy to test filesystems) before
asking you to merge the warning patch. It's probably not totally trivial
to work out what aops are left NULL because they want the default
buffer-head helper, and which are left NULL because they aren't needed.
(this is one problem of having the default callback of course)


>
> > 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.
> > 
> > ...
> >
> > @@ -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;
> > +	}
> 
> 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);

Problem is that it doesn't give you the aop name (and call trace
probably won't help). I'll make it all into a macro though.

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Nick Piggin]

On Mon, Mar 22, 2010 at 11:17:15AM +0200, Boaz Harrosh wrote:
> On 03/22/2010 07:39 AM, Nick Piggin 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.
> > 
> > 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.

Well, everyone's a bit guilty.


> 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.

Defaults will usually be OK for aops that use buffer head functions for
their other operations.


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

OK, thanks very much for being such a proactive fs maintainer. And
for the comments.


> 
> > Index: linux-2.6/mm/filemap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/filemap.c
> > +++ linux-2.6/mm/filemap.c
> > @@ -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);
> 
> 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

Yes definitely (or block_releasepage really).

> 
> > +	}
> >  }
> >  
> >  EXPORT_SYMBOL(try_to_release_page);
> > Index: linux-2.6/mm/page-writeback.c
> > ===================================================================
> > --- linux-2.6.orig/mm/page-writeback.c
> > +++ linux-2.6/mm/page-writeback.c
> > @@ -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;
> > +			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

block_set_page_dirty? I don't know, not such a big deal and it's already
widely used. Probably worry about renames as another issue. (I do agree
that good names are important though)


> > @@ -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->

Sorry this is from the earlier patch I posted (get rid of buffer heads
from mm/fs). That is what prompted this patch.


> > @@ -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.

Yes they should. Although maybe not trivial because the fs may not
use that functionality even if it uses buffer-heads. We don't want
to fill in an operation if it is unused. But the patches should go
through fs maintainers trees.


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

We do care, and yes this is another good reason for the warnings (even
once we do fix up all in-tree filesystems).

> 
> > +		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(-)
> 
> diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
> index a17e4b7..85dd847 100644
> --- a/fs/exofs/inode.c
> +++ b/fs/exofs/inode.c
> @@ -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,
> @@ -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

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Christoph Hellwig]

> @@ -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);
> +	}

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Nick Piggin]

On Mon, Mar 22, 2010 at 07:07:58AM -0400, Christoph Hellwig wrote:
> > @@ -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);
> > +	}
> 
> 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.

Good point.

I think some of that code is actually dead.

is_page_cache_freeable will check for the page reclaim reference,
the pagecache reference, and the PagePrivate reference.

If the page is removed from pagecache, that reference will be
dropped but is_page_cache_freeable() will not consider that and
fail.

NULL page can still come in there from buffer_heads_over_limit AFAIKS,
but if we are relying on that for freeing pages then it can break if a
lot of memory is tied up in other things.

That's all really ugly too. It means no other filesystem may take an 
action to take in case of NULL page->mapping, which means it is really
the wrong thing to do. Fortunately fsblock has proper refcounting so it
would never need to handle this 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.

__set_page_dirty_nobuffers seems OK. Everyone has had to use that
until now anyway. Agreed about try_to_free_buffers.

> 
> 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.

Ah, because I didn't know about it. Thanks. Name I guess can be
ambiguous if there is more than one aop. I'll make it a macro and
print both maybe.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Al Viro]

On Mon, Mar 22, 2010 at 04:39:37PM +1100, Nick Piggin 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.
> 
> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Boaz Harrosh]

On 03/22/2010 12:54 PM, Nick Piggin wrote:
> On Mon, Mar 22, 2010 at 11:17:15AM +0200, Boaz Harrosh wrote:
>> ---
>> git diff --stat -p -M fs/exofs/inode.c
>>  fs/exofs/inode.c |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
>> index a17e4b7..85dd847 100644
>> --- a/fs/exofs/inode.c
>> +++ b/fs/exofs/inode.c
>> @@ -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,
>> @@ -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.
> 

Ho, thanks, that one is much better, yes.

BTW:
The use of nobh_truncate, I hope will go away after your:
	fs: truncate introduce new sequence
with these two helpers you added I can actually get rid of
that as well. (I think. I keep postponing this work ;-))

> Thanks,
> Nick
> 

Thanks
Boaz

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Nick Piggin]

On Mon, Mar 22, 2010 at 11:55:08AM +0000, Al Viro wrote:
> On Mon, Mar 22, 2010 at 04:39:37PM +1100, Nick Piggin 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.
> > 
> > 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.

Yea I didn't have patch order right for a real submission. And clearly
_most_ of the in-tree fses should be converted before actually merging
such warnings.

> 
> 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.

Fair position. The arguments pro are more about cleaner code than any
major improvement. Main thing I don't like that it isn't trivial to see
whether an address space class will use a given function or not. You'd
have to first check the aop to find it's NULL, then check callers to see
whether there is a fallback, then check the fs in case it can attach
buffers that will still be attached at point of calls.

I personally would prefer function pointers filled in.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Andrew Morton]

On Mon, 22 Mar 2010 21:40:57 +1100 Nick Piggin <npiggin@suse.de> wrote:

> > 
> > 	/* 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.

Re: [rfc][patch] mm, fs: warn on missing address space operations

[Nick Piggin]

On Mon, Mar 22, 2010 at 09:30:41AM -0400, Andrew Morton wrote:
> On Mon, 22 Mar 2010 21:40:57 +1100 Nick Piggin <npiggin@suse.de> wrote:
> 
> > > 
> > > 	/* 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.

The backtrace is usually coming from just generic code paths though.
Granted that it's usually not too hard to figure what filesystem it is
(although it may have several aops).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>