Re: [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON

[Jaegeuk Hanse]

On 11/14/2012 11:50 AM, Hugh Dickins wrote:
> On Wed, 14 Nov 2012, Jaegeuk Hanse wrote:
> > On 11/07/2012 07:48 AM, Hugh Dickins wrote:
> > > 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.
> > Hi Hugh,
> >
> > As you said, swp_to_radix_entry() does an "| 2", so even if truncation could
> > remove an entry and something else could immediately after instantiate a new
> > page there, but the expected parameter will not be NULL, the result is
> > radix_tree_insert will not be called and shmem_add_to_page_cache will not
> > return -EEXIST, then why trigger BUG_ON ?
> Why insert the VM_BUG_ON?  Because at the time I thought that it
> asserted something useful; but I was mistaken, as explained above.
>
> How can the VM_BUG_ON trigger (without stack corruption, or something
> of that kind)?  I have no idea.
>
> We are in agreement: I now think that VM_BUG_ON is misleading and silly,
> and sent Andrew a further patch to remove it a just couple of hours ago.
>
> Originally I was waiting to hear further from Dave; but his test
> machine was giving trouble, and it occurred to me that, never mind
> whether he says he has hit it again, or he has not hit it again,
> the answer is the same: don't send that VM_BUG_ON upstream.
>
> Hugh

Thanks Hugh.

Another question. Why the function shmem_fallocate which you add to 
kernel need call shmem_getpage?

Regards,
Jaegeuk

> > Regards,
> > Jaegeuk
> >
> > > 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/