Re: [PATCH v2] mm/migrate: fix shmem xarray update during migration

[Zi Yan <ziy@nvidia.com>]

On 4 Mar 2025, at 12:18, Zi Yan wrote:

> On 4 Mar 2025, at 4:47, Hugh Dickins wrote:
>
>> On Fri, 28 Feb 2025, Zi Yan wrote:
>>
>>> Pagecache uses multi-index entries for large folio, so does shmem. Only
>>> swap cache still stores multiple entries for a single large folio.
>>> Commit fc346d0a70a1 ("mm: migrate high-order folios in swap cache correctly")
>>> fixed swap cache but got shmem wrong by storing multiple entries for
>>> a large shmem folio. Fix it by storing a single entry for a shmem
>>> folio.
>>>
>>> Fixes: fc346d0a70a1 ("mm: migrate high-order folios in swap cache correctly")
>>> Reported-by: Liu Shixin <liushixin2@huawei.com>
>>> Closes: https://lore.kernel.org/all/28546fb4-5210-bf75-16d6-43e1f8646080@huawei.com/
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> Reviewed-by: Shivank Garg <shivankg@amd.com>
>>
>> It's a great find (I think), and your commit message is okay:
>> but unless I'm much mistaken, NAK to the patch itself.
>
> Got it. Thank you for the review.
>
>>
>> First, I say "(I think)" there, because I don't actually know what the
>> loop writing the same folio nr times to the multi-index entry does to
>> the xarray: I can imagine it as being completely harmless, just nr
>> times more work than was needed.

It seems that you are right on this one. I am trying to reproduce the
issue on mainline but could not and I did see shmem hits the entries = nr.
So it is likely there is no bug in mainline just inefficiency.

This fix might just mask the bugs introduced in my folio_split() patchset,
since I reverted my xas_try_split() in shmem_large_split_entry() patch
and still hit the issue. Let me do more debugging and get back.

>>
>> But I guess it does something bad, since Matthew was horrified,
>> and we have all found that your patch appears to improve behaviour
>> (or at least improve behaviour in the context of your folio_split()
>> series: none of us noticed a problem before that, but it may be
>> that your new series is widening our exposure to existing bugs).
>>
>> Maybe your orginal patch, with the shmem_mapping(mapping) check there,
>> was good, and it's only wrong when changed to !folio_test_anon(folio);
>> but TBH I find it too confusing, with the conditionals the way they are.
>> See my preferred alternative below.
>>
>> The vital point is that multi-index entries are not used in swap cache:
>> whether the folio in question orginates from anon or from shmem.  And
>> it's easier to understand once you remember that a shmem folio is never
>> in both page cache and swap cache at the same time (well, there may be an
>> instant of transition from one to other while that folio is held locked) -
>> once it's in swap cache, folio->mapping is NULL and it's no longer
>> recognizable as from a shmem mapping.
>
> Got it. Now it all makes sense to me. Thank you for the explanation.
>
>>
>> The way I read your patch originally, I thought it meant that shmem
>> folios go into the swap cache as multi-index, but anon folios do not;
>> which seemed a worrying mixture to me.  But crashes on the
>> VM_BUG_ON_PAGE(entry != folio, entry) in __delete_from_swap_cache()
>> yesterday (with your patch in) led me to see how add_to_swap_cache()
>> inserts multiple non-multi-index entries, whether for anon or for shmem.
>
> Thanks for the pointer.
>
>>
>> If this patch really is needed in old releases, then I suspect that
>> mm/huge_memory.c needs correction there too; but let me explain in
>> a response to your folio_split() series.
>>
>>> ---
>>>  mm/migrate.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 365c6daa8d1b..2c9669135a38 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -524,7 +524,11 @@ static int __folio_migrate_mapping(struct address_space *mapping,
>>>  			folio_set_swapcache(newfolio);
>>>  			newfolio->private = folio_get_private(folio);
>>>  		}
>>> -		entries = nr;
>>> +		/* shmem uses high-order entry */
>>> +		if (!folio_test_anon(folio))
>>> +			entries = 1;
>>> +		else
>>> +			entries = nr;
>>>  	} else {
>>>  		VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
>>>  		entries = 1;
>>> -- 
>>> 2.47.2
>>
>> NAK to that patch above, here's how I think it should be:
>
> OK. I will resend your fix with __split_huge_page() fixes against Linus’s tree.
> My folio_split() will conflict with the fix, but the merge fix should be
> simple, since the related patch just deletes __split_huge_page() entirely.

Best Regards,
Yan, Zi