Re: [PATCH v2 1/4] mm/huge_memory: change folio_split_supported() to folio_check_splittable()

[Zi Yan <ziy@nvidia.com>]

On 24 Nov 2025, at 5:33, David Hildenbrand (Red Hat) wrote:

> On 11/23/25 19:38, Barry Song wrote:
>> Hi Zi Yan,
>>
>> Thanks for the nice cleanup.
>>
>> On Sat, Nov 22, 2025 at 10:55 AM Zi Yan <ziy@nvidia.com> wrote:
>>>
>>> folio_split_supported() used in try_folio_split_to_order() requires
>>> folio->mapping to be non NULL, but current try_folio_split_to_order() does
>>> not check it. There is no issue in the current code, since
>>> try_folio_split_to_order() is only used in truncate_inode_partial_folio(),
>>> where folio->mapping is not NULL.
>>>
>>> To prevent future misuse, move folio->mapping NULL check (i.e., folio is
>>> truncated) into folio_split_supported(). Since folio->mapping NULL check
>>> returns -EBUSY and folio_split_supported() == false means -EINVAL, change
>>> folio_split_supported() return type from bool to int and return error
>>> numbers accordingly. Rename folio_split_supported() to
>>> folio_check_splittable() to match the return type change.
>>>
>>> While at it, move is_huge_zero_folio() check and folio_test_writeback()
>>> check into folio_check_splittable() and add kernel-doc.
>>>
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> ---
>>>   include/linux/huge_mm.h | 10 ++++--
>>>   mm/huge_memory.c        | 74 +++++++++++++++++++++++++----------------
>>>   2 files changed, 53 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index 1d439de1ca2c..97686fb46e30 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -375,8 +375,8 @@ int __split_huge_page_to_list_to_order(struct page *page, struct list_head *list
>>>   int folio_split_unmapped(struct folio *folio, unsigned int new_order);
>>>   int min_order_for_split(struct folio *folio);
>>>   int split_folio_to_list(struct folio *folio, struct list_head *list);
>>> -bool folio_split_supported(struct folio *folio, unsigned int new_order,
>>> -               enum split_type split_type, bool warns);
>>> +int folio_check_splittable(struct folio *folio, unsigned int new_order,
>>> +                          enum split_type split_type, bool warns);
>>
>>
>> It feels a bit odd to have a warns parameter here, especially given that it's
>> a bool. I understand that in one case we're only checking whether a split is
>> possible, without actually performing it. In the other case, we are performing
>> the split, so we must confirm it's valid — otherwise it's a bug.
>>
>> Could we rename split_type to something more like gfp_flags, where we have
>> variants such as __GFP_NOWARN or something similar? That would make the code
>> much more readable.

We do not want to make folio split complicated, especially the long term plan
is to move to non uniform split entirely. And this warn parameter is solely
for CONFIG_READ_ONLY_THP_FOR_FS, since large folios created via it
cannot be split in non uniform way.

>
> Could we get rid of the "warns" parameter and simply always do a pr_warn_once()?

The issue with this method is that truncating to a large folio created via
CONFIG_READ_ONLY_THP_FOR_FS triggers an undesirable warning.

>
> As an alternative, simply move the warning to the single caller
>
> VM_WARN_ONCE(ret == -EINVAL, "Tried to split an unsplittable folio");

It sounds good to me. It at most needs the person causes the warning to
add some code to find the actual violation.

I will do this in the next version. All VM_WARN_ONCE() and pr_warn_ratelimited()
in folio_check_splittable() will be removed and __folio_split() will
do it when ret is -EINVAL.

Best Regards,
Yan, Zi