Re: [syzbot] [mm?] WARNING in memory_failure

[Miaohe Lin <linmiaohe@huawei.com>]

On 2025/9/30 2:23, jane.chu@oracle.com wrote:
> 
> 
> On 9/29/2025 10:49 AM, jane.chu@oracle.com wrote:
>>
>> On 9/29/2025 10:29 AM, jane.chu@oracle.com wrote:
>>>
>>> On 9/29/2025 4:08 AM, Pankaj Raghav (Samsung) wrote:
>>>>>
>>>>> I want to change all the split functions in huge_mm.h and provide
>>>>> mapping_min_folio_order() to try_folio_split() in truncate_inode_partial_folio().
>>>>>
>>>>> Something like below:
>>>>>
>>>>> 1. no split function will change the given order;
>>>>> 2. __folio_split() will no longer give VM_WARN_ONCE when provided new_order
>>>>> is smaller than mapping_min_folio_order().
>>>>>
>>>>> In this way, for an LBS folio that cannot be split to order 0, split
>>>>> functions will return -EINVAL to tell caller that the folio cannot
>>>>> be split. The caller is supposed to handle the split failure.
>>>>
>>>> IIUC, we will remove warn on once but just return -EINVAL in __folio_split()
>>>> function if new_order < min_order like this:
>>>> ...
>>>>         min_order = mapping_min_folio_order(folio->mapping);
>>>>         if (new_order < min_order) {
>>>> -            VM_WARN_ONCE(1, "Cannot split mapped folio below min- order: %u",
>>>> -                     min_order);
>>>>             ret = -EINVAL;
>>>>             goto out;
>>>>         }
>>>> ...
>>>
>>> Then the user process will get a SIGBUS indicting the entire huge page at higher order -
>>>                  folio_set_has_hwpoisoned(folio);
>>>                  if (try_to_split_thp_page(p, false) < 0) {
>>>                          res = -EHWPOISON;
>>>                          kill_procs_now(p, pfn, flags, folio);
>>>                          put_page(p);
>>>                          action_result(pfn, MF_MSG_UNSPLIT_THP, MF_FAILED);
>>>                          goto unlock_mutex;
>>>                  }
>>>                  VM_BUG_ON_PAGE(!page_count(p), p);
>>>                  folio = page_folio(p);
>>>
>>> the huge page is not usable any way, kind of similar to the hugetlb page situation: since the page cannot be splitted, the entire page is marked unusable.
>>>
>>> How about keep the current huge page split code as is, but change the M- F code to recognize that in a successful splitting case, the poisoned page might just be in a lower folio order, and thus, deliver the SIGBUS ?
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index a24806bb8e82..342c81edcdd9 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -2291,7 +2291,9 @@ int memory_failure(unsigned long pfn, int flags)
>>>                   * page is a valid handlable page.
>>>                   */
>>>                  folio_set_has_hwpoisoned(folio);
>>> -               if (try_to_split_thp_page(p, false) < 0) {
>>> +               ret = try_to_split_thp_page(p, false);
>>> +               folio = page_folio(p);
>>> +               if (ret < 0 || folio_test_large(folio)) {
>>>                          res = -EHWPOISON;
>>>                          kill_procs_now(p, pfn, flags, folio);
>>>                          put_page(p);
>>> @@ -2299,7 +2301,6 @@ int memory_failure(unsigned long pfn, int flags)
>>>                          goto unlock_mutex;
>>>                  }
>>>                  VM_BUG_ON_PAGE(!page_count(p), p);
>>> -               folio = page_folio(p);
>>>          }
>>>
>>> thanks,
>>> -jane
>>
>> Maybe this is better, in case there are other reason for split_huge_page() to return -EINVAL.
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index a24806bb8e82..2bfa05acae65 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1659,9 +1659,10 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>>   static int try_to_split_thp_page(struct page *page, bool release)
>>   {
>>          int ret;
>> +       int new_order = min_order_for_split(page_folio(page));
>>
>>          lock_page(page);
>> -       ret = split_huge_page(page);
>> +       ret = split_huge_page_to_list_to_order(page, NULL, new_order);
>>          unlock_page(page);
>>
>>          if (ret && release)
>> @@ -2277,6 +2278,7 @@ int memory_failure(unsigned long pfn, int flags)
>>          folio_unlock(folio);
>>
>>          if (folio_test_large(folio)) {
>> +               int ret;
>>                  /*
>>                   * The flag must be set after the refcount is bumped
>>                   * otherwise it may race with THP split.
>> @@ -2291,7 +2293,9 @@ int memory_failure(unsigned long pfn, int flags)
>>                   * page is a valid handlable page.
>>                   */
>>                  folio_set_has_hwpoisoned(folio);
>> -               if (try_to_split_thp_page(p, false) < 0) {
>> +               ret = try_to_split_thp_page(p, false);
>> +               folio = page_folio(p);
>> +               if (ret < 0 || folio_test_large(folio)) {
>>                          res = -EHWPOISON;
>>                          kill_procs_now(p, pfn, flags, folio);
>>                          put_page(p);
>> @@ -2299,7 +2303,6 @@ int memory_failure(unsigned long pfn, int flags)
>>                          goto unlock_mutex;
>>                  }
>>                  VM_BUG_ON_PAGE(!page_count(p), p);
>> -               folio = page_folio(p);
>>          }
>>
>>          /*
>> @@ -2618,7 +2621,8 @@ static int soft_offline_in_use_page(struct page *page)
>>          };
>>
>>          if (!huge && folio_test_large(folio)) {
>> -               if (try_to_split_thp_page(page, true)) {
>> +               if ((try_to_split_thp_page(page, true)) ||
>> +                       folio_test_large(page_folio(page))) {
>>                          pr_info("%#lx: thp split failed\n", pfn);
>>                          return -EBUSY;
>>                  }
> 
> In soft offline, better to check if (min_order_for_split > 0), no need to split, just return for now ...

I might be miss something but why we have to split it? Could we migrate the whole thp or folio with min_order instead?

Thanks.
.