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    [from the cache]
©2003-2011 Jasper Spaans