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

[Miaohe Lin <linmiaohe@huawei.com>]

On 2025/9/30 12:35, jane.chu@oracle.com wrote:
> 
> 
> On 9/29/2025 7:51 PM, Miaohe Lin wrote:
>> 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?
> 
> The soft offline code was originally written with the assumption that only 1 base page will be offlined.

Yes, only page corresponding to parameter @pfn of soft_offline_page() will be offlined.

>
> With the recent introduction of min_order, it might quietly offline multiple pages, is that a desirable thing? 

I don't think so. Even if try_to_split_thp_page splits folio into smaller one with min_order, page_handle_poison()
will put back the folio into buddy after migrate_pages, set the hwpoisoned flag to raw error page and hold the extra
refcnt. So only raw error page will be offlined while other sub-pages will be put back into buddy.
Or am I miss something?

Thanks.
.