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

[SeongJae Park]

On Sat, 08 Mar 2025 13:32:02 -0500 Zi Yan <ziy@xxxxxxxxxx> wrote:

> 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:
[...]
> >> 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.

You're right.  I was testing this on the mm-unstable tree, more specifically,
commit 2f0c87542d97.

I confirmed the build failure goes away after moving the definition to the
original place.

> 
> [1] https://lore.kernel.org/linux-mm/20250226210854.2045816-2-ziy@xxxxxxxxxx/
> 
> --
> Best Regards,
> Yan, Zi


Thanks,
SJ