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

David Howells
2006-05-12T16:23:37Z

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