David Howells <dhowells@xxxxxxxxxx> 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
