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

Andrew Morton
2006-05-11T17:43:46Z

David Howells <dhowells@redhat.com> wrote:
>
> The attached patch causes read_cache_pages() to release page-private data on a
> page for which add_to_page_cache() fails or the filler function fails. This
> permits pages with caching references associated with them to be cleaned up.
>

> ---
>
> mm/readahead.c | 16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 0f142a4..82deb7f 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -141,6 +141,12 @@ int read_cache_pages(struct address_spac
> page = list_to_page(pages);
> list_del(&page->lru);
> if (add_to_page_cache(page, mapping, page->index, GFP_KERNEL)) {
> + if (PagePrivate(page) && mapping->a_ops->releasepage) {
> + page->mapping = mapping;
> + mapping->a_ops->releasepage(page, GFP_KERNEL);
> + page->mapping = NULL;
> + }
> +

That seems a bit hacky, really. It'd be better to use
try_to_release_page(). It keeps stuff in one place, and what happens if
the filesystem decided to not implement ->releasepage() because it knows
that try_to_release_page() will default to try_to_free_buffers()?

The above code is identical to the below code, so a new helper function
would be appropriate.

> page_cache_release(page);
> continue;
> }
> @@ -153,6 +159,16 @@ int read_cache_pages(struct address_spac
>
> victim = list_to_page(pages);
> list_del(&victim->lru);
> +
> + if (PagePrivate(victim) &&
> + mapping->a_ops->releasepage
> + ) {
> + victim->mapping = mapping;
> + mapping->a_ops->releasepage(
> + victim, GFP_KERNEL);
> + victim->mapping = NULL;
> + }

aaaarrrghhh. David, _why_ do you insist on junk like this when you know
what the coding style is and you've repeatedly been asked to follow it? I
mean, how hard is it? How many similar uglies are hiding in this patchset?
(greps. 53 of them). Ho hum.

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.

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

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

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

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

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