Re: [PATCH 13/14] FS-Cache: Release page->private in failed readahead [try #8]

[David Howells <dhowells@redhat.com>]

Andrew Morton <akpm@osdl.org> wrote:

> > 		SetPageLocked(page);
> 
> 	if (TestSetPagLocked(page))
> 		BUG();
> 
> would make me more comfortable..

That shouldn't be necessary if add_to_page_cache() also doesn't do that, but
if you wish, I can do that - it's the error handling path, so it doesn't
matter too much performancewise.

> > > For the second hunk, is it not possible to do this cleanup in the callback
> > > function?
> > 
> > Which callback function?
> 
> I was referring to the filler_t thingy.  Is it not possible to get control
> of that?

Well, the filler_t thing is generally a_ops->readpage from the caller fs, but
we don't want to call that if add_to_page_cache() failed, and we don't want to
call it if we're just discarding a bunch of pages we've now no intention of
actually reading.

I suppose we could add another callback for ditching pages we don't want to
keep.  This has the potential to be called quite a lot because of the way
readahead works on Linux.

> So please, can we have some comments in there which describe the new
> behaviour in a manner sufficient for a maintainer to follow so people don't
> break your stuff?

Okay... I'll add more comments.  I should probably also extend the
documentation on releasepage().  It won't be till Monday though.

> > Out of interest, why do we need PG_private to say there's something in
> > page->private?  Can't it just be assumed either that if page->private is
> > non-zero or that if a_ops->releasepage() is non-NULL, then we need to
> > "release" the page?
> 
> page->private is an unsigned long, not a pointer.  The core kernel hence
> cannot determine from its value whether or not it is live.  For example, the
> fs might choose to treat it as a bitmap of which-blocks-are-uptodate and
> which-blocks-are-dirty.

Then the second option is still possible (calling releasepage()
unconditionally).

David