Re: [PATCH] mm/hwpoison: fix race between soft_offline_page and unpoison_memory

[Naoya Horiguchi]

On Fri, Aug 14, 2015 at 03:54:36PM +0800, Wanpeng Li wrote:
> On 8/14/15 3:26 PM, Naoya Horiguchi wrote:
> > On Fri, Aug 14, 2015 at 01:03:53PM +0800, Wanpeng Li wrote:
> >> On 8/14/15 12:19 PM, Naoya Horiguchi wrote:
> > ...
> >>>>>>> If I read correctly, the old migratetype approach has a few problems:
> >>>>>>>   1) it doesn't fix the problem completely, because
> >>>>>>>      set_migratetype_isolate() can fail to set MIGRATE_ISOLATE to the
> >>>>>>>      target page if the pageblock of the page contains one or more
> >>>>>>>      unmovable pages (i.e. has_unmovable_pages() returns true).
> >>>>>>>   2) the original code changes migratetype to MIGRATE_ISOLATE forcibly,
> >>>>>>>      and sets it to MIGRATE_MOVABLE forcibly after soft offline, regardless
> >>>>>>>      of the original migratetype state, which could impact other subsystems
> >>>>>>>      like memory hotplug or compaction.
> >>>>>> Maybe we can add a "FIXME" comment on the Migratetype stuff, since the
> >>>>>> current linus tree calltrace and it should be fixed immediately, and I
> >>>>>> don't see obvious bugs appear on migratetype stuffs at least currently,
> >>>>>> so "FIXME" is enough. :-)
> >>>>> Sorry if confusing, but my intention in saying about "FIXME" comment was
> >>>>> that we can find another solution for this race rather than just reverting,
> >>>>> so adding comment about the reported bug in current code (keeping code from
> >>>>> 4491f712606) is OK for very short term.
> >>>>> I understand that leaving a race window of BUG_ON is not the best thing, but
> >>>>> as I said, this race shouldn't affect end-users, so this is not an urgent bug.
> >>>>> # It's enough if testers know this.
> >>>> The 4.2 is coming, this patch can be applied as a temporal solution in
> >>>> order to fix the broken linus tree, and the any final solution can be
> >>>> figured out later.
> >>> I didn't reproduce this problem yet in my environment, but from code reading
> >>> I guess that checking PageHWPoison flag in unmap_and_move() like below could
> >>> avoid the problem. Could you testing with this, please?
> >> I have already try to modify unmap_and_move() the same as what you do
> >> before I post migratetype stuff. It doesn't work and have other calltrace.
> > OK, then I rethink of handling the race in unpoison_memory().
> >
> > Currently properly contained/hwpoisoned pages should have page refcount 1
> > (when the memory error hits LRU pages or hugetlb pages) or refcount 0
> > (when the memory error hits the buddy page.) And current unpoison_memory()
> > implicitly assumes this because otherwise the unpoisoned page has no place
> > to go and it's just leaked.
> > So to avoid the kernel panic, adding prechecks of refcount and mapcount
> > to limit the page to unpoison for only unpoisonable pages looks OK to me.
> > The page under soft offlining always has refcount >=2 and/or mapcount > 0,
> > so such pages should be filtered out.
> >
> > Here's a patch. In my testing (run soft offline stress testing then repeat
> > unpoisoning in background,) the reported (or similar) bug doesn't happen.
> > Can I have your comments?
> 
> As page_action() prints out page maybe still referenced by some users,
> however, PageHWPoison has already set. So you will leak many poison pages.

Right, but it isn't a problem, because error handling doesn't always succeed.
Our basic policy for such case is to leak the page intentionally. IOW, the
memory leakage happen even in current kernel (unpoison doesn't work because
leaked page never return to buddy.) So my suggestion doesn't make things worse.

Thanks,
Naoya Horiguchi--
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/