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:

> The above code is identical to the below code, so a new helper function
> would be appropriate.
> ...
> I think the above will be called against an unlocked page, in which case
> the ->releasepage() implementation might choose to go BUG, or something.
> I suppose locking the page here will suffice.

I'll move that bit of code into a helper function, along with the
page_cache_release() and call it from both places.  I'll also call
try_to_release_page() as you suggest rather than going directly.  I'll lock
the page too:

static inline void read_cache_pages_release_page(struct address_space *mapping,
						 struct page *page)
{
	if (PagePrivate(page)) {
		page->mapping = mapping;
		SetPageLocked(page);
		try_to_release_page(page, GFP_KERNEL);
		page->mapping = NULL;
	}

	page_cache_release(page);
}

> But it all seems a bit abusive of what ->releasepage() is supposed to do.

Where else should I do it?  I'm using releasepage() to break the association
that the cache has made with a page.  If I don't do this, the cache may wind
up retaining metadata unnecessarily.

I suppose I could add another address space op to do this, and have
page_cache_release() check page->mapping->a_ops->destroypage(), and then force
the mapping to be passed through to page_cache_release() where necessary.

> add_to_page_cache() won't set PagePrivate() anyway, so what point is there
> in the first hunk?

The PagePrivate() bit is already set before read_cache_pages() is called.
What happens is that the cache is invoked first: it sets to read any pages it
can satisfy from the data it holds, and marks those pages for which it has
allocated buffer space; the unsatisfied pages are then returned to NFS, which
then calls read_cache_pages() to invoke readpage() serially - but if any pages
get discarded, the cache metadata _also_ needs to be discarded.

> For the second hunk, is it not possible to do this cleanup in the callback
> function?

Which callback function?  The cleanup must be done before the page is returned
to the page allocator, and since that is performed by read_cache_pages(), in
read_cache_pages() the cleanup must be done.  The other option is to not use
read_cache_pages(), I suppose.

> If read_cache_pages() needs this treatment, shouldn't we also do it in
> read_pages()?

Because read_pages() doesn't give the filesystem a chance to know about pages
between it allocating them and it releasing them when add_to_page_cache()
fails.  Although it calls readpage(), if that fails it should clean up for
itself.

read_cache_pages() does not allocate the pages for itself.  It's called from a
filesystem's readpages() op, which gives the filesystem ample opportunity to
know about the pages that read_pages() doesn't afford it.

> And in mpage_readpages()?

mpage_readpages() uses PG_private for its own purposes, and so keying on that
for any purpose but holding buffers is impossible, and if mpage_readpages()
needs to clean those up, it must do so already.

However, you've raised a good point, and it's one that'll need to be solved if
I want to do caching on ISOFS and suchlike.

> Again, as this appears to be some special treatment for cachefs wouldn't it
> be better to keep this special handling within cachefs?

How?  CacheFS can't practically monitor the pages it has been told about just
in case they've been given back.  The netfs has to drive that end of things.

I could copy read_cache_pages() and place that in fscache and change it
thusly, but there's no requirement that a netfs should use PG_private for
marking cached pages - that just happens to be the way I've done it in NFS and
AFS, but it can't be the way I do it in ISOFS.

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?

David