On 11/07/2012 07:48 AM, Hugh Dickins wrote:

On Tue, 6 Nov 2012, Dave Jones wrote:

quoted

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...

quoted

and still hit the (new!) VM_BUG_ON

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

quoted

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...
  

quoted

------------[ 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 ?

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, 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>

--
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>