Re: [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON
From: Jaegeuk Hanse
Date: Tue Nov 13 2012 - 22:07:06 EST
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 ?
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@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
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/