Re: [PATCH mm-unstable] mm: migrate: folio_ref_freeze() under xas_lock_irq()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/6/26 3:51, Hugh Dickins wrote:
> On Tue, 25 Jun 2024, Kefeng Wang wrote:
>> On 2024/6/25 13:04, Hugh Dickins wrote:
>>> Commit "mm: migrate: split folio_migrate_mapping()" drew attention to
>>> "Note, the folio_ref_freeze() is moved out of xas_lock_irq(), Since the
>>> folio is already isolated and locked during migration, so suppose that
>>> there is no functional change."
>>>
>>> That was a mistake. Freezing a folio's refcount to 0 is much like taking
>>> a spinlock: see how filemap_get_entry() takes rcu_read_lock() then spins
>>> around until the folio is unfrozen. If the task freezing is preempted (or
>>> calls cond_resched(), as folio_mc_copy() may do), then it risks deadlock:
>>> in my case, one CPU in zap_pte_range() with free_swap_and_cache_nr()
>>> trying to reclaim swap while PTL is held, all the other CPUs in reclaim
>>> spinning for that PTL.
>>
>> Oh, thanks for pointing this out, I didn't take that into account.
>>>
>>> I'm uncertain whether it's necessary for interrupts to be disabled as
>>> well as preemption, but since they have to be disabled for the page
>>> cache migration, it's much the best to do it all together as before.
>>> So revert to folio_ref_freeze() under xas_lock_irq(): but keep the
>>> preliminary folio_ref_count() check, which does make sense before
>>> trying to copy the folio's data.
>>
>> This is what my RFC version[1] does, which adds same reference check to
>> avoid the unnecessary folio_mc_copy().
>>
>> Should I resend all patches, or Andrew directly pick this one?
> 
> Andrew asks for a resend: I was only aiming to fix the bug, and
> have no perspective on how much of the series remains worthwhile.
> 
>>
>>
>> Thanks.
>>
>> [1]
>> https://lore.kernel.org/linux-mm/20240129070934.3717659-7-wangkefeng.wang@huawei.com/
>>
>>
>>>
>>> Use "expected_count" for the expected count throughout.
>>>
>>> Signed-off-by: Hugh Dickins <hughd@google.com>
>>> ---
>>>    mm/migrate.c | 59 +++++++++++++++++++++++++---------------------------
>>>    1 file changed, 28 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 27f070f64f27..8beedbb42a93 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -400,8 +400,8 @@ static int folio_expected_refs(struct address_space
>>> *mapping,
>>>     * 2 for folios with a mapping
>>>     * 3 for folios with a mapping and PagePrivate/PagePrivate2 set.
>>>     */
>>> -static void __folio_migrate_mapping(struct address_space *mapping,
>>> -		struct folio *newfolio, struct folio *folio, int expected_cnt)
>>> +static int __folio_migrate_mapping(struct address_space *mapping,
>>> +		struct folio *newfolio, struct folio *folio, int
>>> expected_count)
>>
>> We can rename it back to folio_migrate_mapping().
> 
> Almost.  I did want to remove the __ layer, but internally the last
> parameter is expected_count, whereas externally it is extra_count:
> each has merit in its place, so I left them as is.

Indeed， I refresh all patches, we still need this 
__filemap_migrate_mapping, thanks.
> 
> Hugh
>