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 from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
