Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | From | Neil Brown <> | Date | Thu, 20 Apr 2006 19:24:24 +1000 | Subject | Re: [PATCH] Remove softlockup from invalidate_mapping_pages. |
| |
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? Does Jens know that? __generic_file_splice_read seems to violate this principle!
Are you allowed to look at ->mapping? Or can that change magically too? What's the threat-model? Is it splice(), or something more wicked?
> > 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....
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.
Thanks for the feedback,
NeilBrown
> --- devel/mm/truncate.c~remove-softlockup-from-invalidate_mapping_pages 2006-04-20 00:20:49.000000000 -0700 > +++ devel-akpm/mm/truncate.c 2006-04-20 00:28:18.000000000 -0700 > @@ -230,14 +230,24 @@ unsigned long invalidate_mapping_pages(s > pagevec_lookup(&pvec, mapping, next, PAGEVEC_SIZE)) { > for (i = 0; i < pagevec_count(&pvec); i++) { > struct page *page = pvec.pages[i]; > + pgoff_t index; > + int locked; > > - if (TestSetPageLocked(page)) { > - next++; > - continue; > - } > - if (page->index > next) > - next = page->index; > + locked = TestSetPageLocked(page); > + > + /* > + * We really shouldn't be looking at the ->index of an > + * unlocked page. But we're not allowed to lock these > + * pages. So we rely upon nobody altering the ->index > + * of this (pinned-by-us) page. > + */ > + index = page->index; > + if (index > next) > + next = index; > next++; > + if (!locked) > + continue; > + > if (PageDirty(page) || PageWriteback(page)) > goto unlock; > if (page_mapped(page)) > _ > > > - > 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/ - 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/
| ![\](/images/icornerr.gif) |