Re: [PATCH] Remove softlockup from invalidate_mapping_pages.

[Neil Brown <neilb@suse.de>]

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/