Re: [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON

[Hugh Dickins <hughd@google.com>]

On Tue, 6 Nov 2012, Dave Jones wrote:
> On Mon, Nov 05, 2012 at 05:32:41PM -0800, Hugh Dickins wrote:
> 
>  > -			/* We already confirmed swap, and make no allocation */
>  > -			VM_BUG_ON(error);
>  > +			/*
>  > +			 * We already confirmed swap under page lock, and make
>  > +			 * no memory allocation here, so usually no possibility
>  > +			 * of error; but free_swap_and_cache() only trylocks a
>  > +			 * page, so it is just possible that the entry has been
>  > +			 * truncated or holepunched since swap was confirmed.
>  > +			 * shmem_undo_range() will have done some of the
>  > +			 * unaccounting, now delete_from_swap_cache() will do
>  > +			 * the rest (including mem_cgroup_uncharge_swapcache).
>  > +			 * Reset swap.val? No, leave it so "failed" goes back to
>  > +			 * "repeat": reading a hole and writing should succeed.
>  > +			 */
>  > +			if (error) {
>  > +				VM_BUG_ON(error != -ENOENT);
>  > +				delete_from_swap_cache(page);
>  > +			}
>  >  		}
> 
> I ran with this overnight,

Thanks a lot...

> and still hit the (new!) VM_BUG_ON

... but that's even more surprising than your original report.

> 
> Perhaps we should print out what 'error' was too ?  I'll rebuild with that..

Thanks; though I thought the error was going to turn out too boring,
and was preparing a debug patch for you to show the expected and found
values too.  But then got very puzzled...
 
> 
> ------------[ cut here ]------------
> WARNING: at mm/shmem.c:1151 shmem_getpage_gfp+0xa5c/0xa70()
> Hardware name: 2012 Client Platform
> Pid: 21798, comm: trinity-child4 Not tainted 3.7.0-rc4+ #54

That's the very same line number as in your original report, despite
the long comment which the patch adds.  Are you sure that kernel was
built with the patch in?

I wouldn't usually question you, but I'm going mad trying to understand
how the VM_BUG_ON(error != -ENOENT) fires.  At the time I wrote that
line, and when I was preparing the debug patch, I was thinking that an
error from shmem_radix_tree_replace could also be -EEXIST, for when a
different something rather than nothing is found [*].  But that's not
the case, shmem_radix_tree_replace returns either 0 or -ENOENT.

So if error != -ENOENT, that means shmem_add_to_page_cache went the
radix_tree_insert route instead of the shmem_radix_tree_replace route;
which means that its 'expected' is NULL, so swp_to_radix_entry(swap)
is NULL; but swp_to_radix_entry() does an "| 2", so however corrupt
the radix_tree might be, I do not understand the new VM_BUG_ON firing.

Please tell me it was the wrong kernel!
Hugh

[*] But in thinking it over, I realize that if shmem_radix_tree_replace
had returned -EEXIST for the "wrong something" case, I would have been
wrong to BUG on that; because just as truncation could remove an entry,
something else could immediately after instantiate a new page there.

So although I believe my VM_BUG_ON(error != -ENOENT) is safe, it's
not saying what I had intended to say with it, and would have been
wrong to say that anyway.  It just looks stupid to me now, rather
like inserting a VM_BUG_ON(false) - but that does become interesting
when you report that you've hit it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>