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