[rfc][patch] mm, fs: warn on missing address space operations
Nick Piggin
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Andrew Morton
> 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);
Pekka Enberg
/me gets his paint bucket...
How about
WARN_ONCE(!invalidatepage, "this fs should use block_invalidatepage()")
Pekka
Boaz Harrosh
> 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
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
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,
};
/******************************************************************************
Nick Piggin
> On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin <npi...@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.
Nick Piggin
> 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).
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
Christoph Hellwig
>
> 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.
Nick Piggin
> > @@ -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.
Al Viro
> 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.
Boaz Harrosh
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
Nick Piggin
> 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.
Andrew Morton
> >
> > /* 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.
Nick Piggin
> On Mon, 22 Mar 2010 21:40:57 +1100 Nick Piggin <npi...@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).