Re: [PATCH v4 4/6] mm: migrate: support poisoned recover from migrate folio

[Jane Chu <jane.chu@oracle.com>]

On 6/6/2024 3:28 PM, Jane Chu wrote:
> On 6/6/2024 2:27 PM, Jane Chu wrote:
>
>> On 6/3/2024 2:24 AM, Kefeng Wang wrote:
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index e930376c261a..28aa9da95781 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -663,16 +663,29 @@ static int __migrate_folio(struct 
>>> address_space *mapping, struct folio *dst,
>>>                  struct folio *src, void *src_private,
>>>                  enum migrate_mode mode)
>>>   {
>>> -    int rc;
>>> +    int ret, expected_cnt = folio_expected_refs(mapping, src);
>>>   -    rc = folio_migrate_mapping(mapping, dst, src, 0);
>>> -    if (rc != MIGRATEPAGE_SUCCESS)
>>> -        return rc;
>>> +    if (!mapping) {
>>> +        if (folio_ref_count(src) != expected_cnt)
>>> +            return -EAGAIN;
>>> +    } else {
>>> +        if (!folio_ref_freeze(src, expected_cnt))
>>> +            return -EAGAIN;
>>> +    }
>>> +
>>
>> Let me take a guess, the reason you split up folio_migrate_copy() is 
>> that
>>
>> folio_mc_copy() should be done before the 'src' folio's ->flags is 
>> changed, right?
>>
>> Is there any other reason?  Could you add a comment please?
>
> I see, both the clearing of the 'dirty' bit in the source folio, and 
> the xas_store of the
>
> new folio to the mapping, these need to be done after folio_mc_copy 
> considering in the
>
> event of UE, memory_failure() is called to handle the poison in the 
> source page.
>
> That said, since the poisoned page was queued up and handling is 
> asynchronous, so in
>
> theory, there is an extremely unlikely chance that memory_failure() is 
> invoked after
>
> folio_migrate_mapping(), do you think things would still be cool?

Hmm, perhaps after xas_store, the source folio->mapping should be set to 
NULL.

thanks,

-jane

>
>
> thanks,
>
> -jane
>
>>
>>> +    ret = folio_mc_copy(dst, src);
>>> +    if (unlikely(ret)) {
>>> +        if (mapping)
>>> +            folio_ref_unfreeze(src, expected_cnt);
>>> +        return ret;
>>> +    }
>>> +
>>> +    __folio_migrate_mapping(mapping, dst, src, expected_cnt);
>>>         if (src_private)
>>>           folio_attach_private(dst, folio_detach_private(src));
>>>   -    folio_migrate_copy(dst, src);
>>> +    folio_migrate_flags(dst, src);
>>>       return MIGRATEPAGE_SUCCESS;
>>>   }
>>
>> thanks,
>>
>> -jane
>>
>>
>