Re: [PATCH v3 1/2] mm/filemap: use xas_try_split() in __filemap_add_folio()

[Zi Yan]

On 8 Mar 2025, at 13:14, SeongJae Park wrote:

> Hello,
>
> On Wed, 26 Feb 2025 16:08:53 -0500 Zi Yan <ziy@xxxxxxxxxx> wrote:
>
>> During __filemap_add_folio(), a shadow entry is covering n slots and a
>> folio covers m slots with m < n is to be added.  Instead of splitting all
>> n slots, only the m slots covered by the folio need to be split and the
>> remaining n-m shadow entries can be retained with orders ranging from m to
>> n-1.  This method only requires
>>
>> 	(n/XA_CHUNK_SHIFT) - (m/XA_CHUNK_SHIFT)
>>
>> new xa_nodes instead of
>> 	(n % XA_CHUNK_SHIFT) * ((n/XA_CHUNK_SHIFT) - (m/XA_CHUNK_SHIFT))
>>
>> new xa_nodes, compared to the original xas_split_alloc() + xas_split()
>> one.  For example, to insert an order-0 folio when an order-9 shadow entry
>> is present (assuming XA_CHUNK_SHIFT is 6), 1 xa_node is needed instead of
>> 8.
>>
>> xas_try_split_min_order() is introduced to reduce the number of calls to
>> xas_try_split() during split.
>>
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
>> Cc: Kairui Song <kasong@xxxxxxxxxxx>
>> Cc: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>> Cc: Mattew Wilcox <willy@xxxxxxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
>> Cc: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
>> Cc: Kirill A. Shuemov <kirill.shutemov@xxxxxxxxxxxxxxx>
>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>> Cc: Yang Shi <yang@xxxxxxxxxxxxxxxxxxxxxx>
>> Cc: Yu Zhao <yuzhao@xxxxxxxxxx>
>> ---
>>  include/linux/xarray.h |  7 +++++++
>>  lib/xarray.c           | 25 +++++++++++++++++++++++
>>  mm/filemap.c           | 45 +++++++++++++++++-------------------------
>>  3 files changed, 50 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
>> index 4010195201c9..78eede109b1a 100644
>> --- a/include/linux/xarray.h
>> +++ b/include/linux/xarray.h
>> @@ -1556,6 +1556,7 @@ int xas_get_order(struct xa_state *xas);
>>  void xas_split(struct xa_state *, void *entry, unsigned int order);
>>  void xas_split_alloc(struct xa_state *, void *entry, unsigned int order, gfp_t);
>>  void xas_try_split(struct xa_state *xas, void *entry, unsigned int order);
>> +unsigned int xas_try_split_min_order(unsigned int order);
>>  #else
>>  static inline int xa_get_order(struct xarray *xa, unsigned long index)
>>  {
>> @@ -1582,6 +1583,12 @@ static inline void xas_try_split(struct xa_state *xas, void *entry,
>>  		unsigned int order)
>>  {
>>  }
>> +
>> +static inline unsigned int xas_try_split_min_order(unsigned int order)
>> +{
>> +	return 0;
>> +}
>> +
>>  #endif
>>
>>  /**
>> diff --git a/lib/xarray.c b/lib/xarray.c
>> index bc197c96d171..8067182d3e43 100644
>> --- a/lib/xarray.c
>> +++ b/lib/xarray.c
>> @@ -1133,6 +1133,28 @@ void xas_split(struct xa_state *xas, void *entry, unsigned int order)
>>  }
>>  EXPORT_SYMBOL_GPL(xas_split);
>>
>> +/**
>> + * xas_try_split_min_order() - Minimal split order xas_try_split() can accept
>> + * @order: Current entry order.
>> + *
>> + * xas_try_split() can split a multi-index entry to smaller than @order - 1 if
>> + * no new xa_node is needed. This function provides the minimal order
>> + * xas_try_split() supports.
>> + *
>> + * Return: the minimal order xas_try_split() supports
>> + *
>> + * Context: Any context.
>> + *
>> + */
>> +unsigned int xas_try_split_min_order(unsigned int order)
>> +{
>> +	if (order % XA_CHUNK_SHIFT == 0)
>> +		return order == 0 ? 0 : order - 1;
>> +
>> +	return order - (order % XA_CHUNK_SHIFT);
>> +}
>> +EXPORT_SYMBOL_GPL(xas_try_split_min_order);
>> +
>
> I found this makes build fails when CONFIG_XARRAY_MULTI is unset, like below.
>
>     /linux/lib/xarray.c:1251:14: error: redefinition of ‘xas_try_split_min_order’
>      1251 | unsigned int xas_try_split_min_order(unsigned int order)
>           |              ^~~~~~~~~~~~~~~~~~~~~~~
>     In file included from /linux/lib/xarray.c:13:
>     /linux/include/linux/xarray.h:1587:28: note: previous definition of ‘xas_try_split_min_order’ with type ‘unsigned int(unsigned int)’
>      1587 | static inline unsigned int xas_try_split_min_order(unsigned int order)
>           |                            ^~~~~~~~~~~~~~~~~~~~~~~
>
> I think we should have the definition only when CONFIG_XARRAY_MULTI?

I think it might be a merge issue, since my original patch[1] places
xas_try_split_min_order() above xas_try_split(), both of which are
in #ifdef CONFIG_XARRAY_MULTI #endif. But mm-everything-2025-03-08-00-43
seems to move xas_try_split_min_order() below xas_try_split() and
out of CONFIG_XARRAY_MULTI guard.

[1] https://lore.kernel.org/linux-mm/20250226210854.2045816-2-ziy@xxxxxxxxxx/

--
Best Regards,
Yan, Zi