Re: [PATCH v3] mm/page_isolation: fix potential missing call to unset_migratetype_isolate()

[David Hildenbrand]

On 14.09.21 13:43, Miaohe Lin wrote:
> In start_isolate_page_range() undo path, pfn_to_online_page() just checks
> the first pfn in a pageblock while __first_valid_page() will traverse the
> pageblock until the first online pfn is found. So we may miss the call to
> unset_migratetype_isolate() in undo path and pages will remain isolated
> unexpectedly. Fix this by calling undo_isolate_page_range() and this will
> also help to simplify the code further. Note we shouldn't ever trigger it
> because MAX_ORDER-1 aligned pfn ranges shouldn't contain memory holes now.
>
> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>

I read Michals reply, however, I am quite conservative with Fixes: tags. 
If there is nothing to fix, there is no BUG and the patch consequently 
merely a cleanup.

I'd have gone with a patch description/subject as follows:

"
mm/page_isolation: cleanup start_isolate_page_range()

We can heavily simplify the code by reusing undo_isolate_page_range().

Note that this also tackles a theoretical issue that would have been a 
real BUG before commit c5e79ef561b0 ("mm/memory_hotplug.c: don't allow 
to online/offline memory blocks with holes"). In 
start_isolate_page_range() undo path, pfn_to_online_page() just checks
the first pfn in a pageblock while __first_valid_page() will traverse 
the pageblock until the first online pfn is found. So we may miss the 
call to unset_migratetype_isolate() in undo path and pages will remain 
isolated unexpectedly.

Nowadays, start_isolate_page_range() never gets called on ranges that 
might contain memory holes. Consequently, this patch is not a fix but a 
cleanup.
"

Anyhow, whatever the other people prefer, no strong opinion.

Reviewed-by: David Hildenbrand <david@xxxxxxxxxx>

--
Thanks,

David / dhildenb