Re: [RFC PATCH v4 2/6] mm/migrate: skip data copy for already-copied folios

[Garg, Shivank]

On 4/7/2026 12:22 PM, Huang, Ying wrote:
> "Garg, Shivank" <shivankg@xxxxxxx> writes:
> 
>> On 3/24/2026 1:52 PM, Huang, Ying wrote:
>>> Shivank Garg <shivankg@xxxxxxx> writes:
>>>
>>
>>>>  static int move_to_new_folio(struct folio *dst, struct folio *src,
>>>> -				enum migrate_mode mode)
>>>> +		enum migrate_mode mode, bool already_copied)
>>>>  {
>>>>  	struct address_space *mapping = folio_mapping(src);
>>>>  	int rc = -EAGAIN;
>>>> @@ -1096,6 +1114,9 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>>>>  	VM_BUG_ON_FOLIO(!folio_test_locked(src), src);
>>>>  	VM_BUG_ON_FOLIO(!folio_test_locked(dst), dst);
>>>>  
>>>> +	if (already_copied)
>>>> +		dst->private = (void *)(unsigned long)PAGE_ALREADY_COPIED;
>>>> +
>>>
>>> IMHO, this appears to be an unusual way to pass arguments to a function.
>>> Why not adjust the parameters of migrate_folio()?  How about turning enum
>>> migrate_mode into a bitmask (migrate_flags)?
>>>
>>
>> Using folio->private, keeps the change self-contained in migrate.c
>>
>> David suggested adding a dedicated unsigned long migrate_info field in the
>> folio union. I'll switch to that, as this is cleaner and avoid hacky use of ->private.
>>
>> https://lore.kernel.org/linux-mm/27b1b602-129f-4bc5-a553-386e8d1f5d90@xxxxxxxxxx
> 
> That's good for the original usage of folio->private.  That is, to
> record migration related information for a list of folios.
> 
>>
>> Changing the migrate_folio() a_ops signature would touch nearly every
>> filesystem for something that only core migration cares about, and does not look
>> practical. We can't add to migrate_mode enum either as currently those values are mutually
>> exclusive and ordered levels of increasing synchrony (ASYNC < SYNC_LIGHT < SYNC)
>> And there are checks like this (cc->mode < MIGRATE_SYNC) or  mode != MIGRATE_SYNC
>> This could break it.
> 
> IMHO, code readability is more important than limiting the scope of
> changes. The migrate_folio callback of most file systems shares a few
> common implementations in migrate.c. So, I think it is doable.
> 

I agree. For v5, I'll go with the folio union, where I avoid touching the filesystem callbacks,
which felt like too much churn at this point.
If it doesn't read well we can revisit the callback signature.

Thanks,
Shivank