lkml.org 
[lkml]   [2006]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 13/14] FS-Cache: Release page->private in failed readahead [try #8]
    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?
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-05-11 19:46    [W:3.329 / U:0.648 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site