lkml.org 
[lkml]   [2006]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Remove softlockup from invalidate_mapping_pages.
    Neil Brown <neilb@suse.de> wrote:
    >
    > On Thursday April 20, akpm@osdl.org wrote:
    > > > Cc: "Steinar H. Gunderson" <sgunderson@bigfoot.com>
    > > > Signed-off-by: Neil Brown <neilb@suse.de>
    > > >
    > > > ### Diffstat output
    > > > ./mm/truncate.c | 10 ++++------
    > > > 1 file changed, 4 insertions(+), 6 deletions(-)
    > > >
    > > > diff ./mm/truncate.c~current~ ./mm/truncate.c
    > > > --- ./mm/truncate.c~current~ 2006-04-20 15:27:22.000000000 +1000
    > > > +++ ./mm/truncate.c 2006-04-20 15:38:20.000000000 +1000
    > > > @@ -238,13 +238,11 @@ unsigned long invalidate_mapping_pages(s
    > > > for (i = 0; i < pagevec_count(&pvec); i++) {
    > > > struct page *page = pvec.pages[i];
    > > >
    > > > - if (TestSetPageLocked(page)) {
    > > > - next++;
    > > > + next = page->index+1;
    > > > +
    > > > + if (TestSetPageLocked(page))
    > > > continue;
    > > > - }
    > > > - if (page->index > next)
    > > > - next = page->index;
    > > > - next++;
    > > > +
    > > > if (PageDirty(page) || PageWriteback(page))
    > > > goto unlock;
    > > > if (page_mapped(page))
    > >
    > > We're not supposed to look at page->index of an unlocked page.
    >
    > We're not?

    We've avoided it. But I think we could change the rules.

    > Does Jens know that?
    > __generic_file_splice_read seems to violate this principle!

    It looks OK from a quick read (but the code duplication is saddening)

    > Are you allowed to look at ->mapping? Or can that change magically
    > too?

    No. ->mapping can be set to NULL by truncate. That's why everyone does

    get_page(page);
    lock_page(page);
    if (page->mapping == NULL)
    someone_just_truncated_this_page();

    But truncate doesn't alter ->index. And I think we can assume that.

    > What's the threat-model? Is it splice(), or something more wicked?

    truncate.

    > >
    > > In practice, I think it's OK - there's no _reason_ why anyone would want to
    > > trash the ->index of a just-truncated page. However I think it'd be saner
    > > to a) only look at ->index after we've tried to lock the page and b) make
    > > sure that ->index is really "to the right" of where we're currently at.
    > >
    > > How's this look?
    > >
    >
    > Uhmm... possibly OK, but I think I'd rather change find_get_pages to
    > take an index pointer like find_get_pages_tag does, and do the thing
    > safely. However that started turning into a big patch (reiserfs calls
    > find_get_pages directly a few times, and I hadn't even got up to
    > callers of pagevec_lookup....

    Yes, that could get involved.

    > So when I have a cleared head and a bit more time I'll see if I can
    > come up with a better patch which only looks at ->index under
    > ->tree_lock.

    tree_lock will stabilise ->index, yes.

    But I think we'd be OK assuming that ->index is stable. Although that may
    break if splice() is concurrently pulling the page out of pagecache and
    stuffing it into a pipe.

    -
    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-04-20 11:55    [W:0.025 / U:0.944 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site