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