Re: [PATCH] xarray: Add a BUG_ON() to ensure caller is not sibling

[Dev Jain]

On 02/06/25 8:33 pm, Zi Yan wrote:
> On 29 May 2025, at 23:44, Dev Jain wrote:
>
> > On 30/05/25 4:17 am, Zi Yan wrote:
> > > On 28 May 2025, at 23:17, Dev Jain wrote:
> > >
> > > > On 28/05/25 10:42 pm, Zi Yan wrote:
> > > > > On 28 May 2025, at 7:31, Dev Jain wrote:
> > > > >
> > > > > > Suppose xas is pointing somewhere near the end of the multi-entry batch.
> > > > > > Then it may happen that the computed slot already falls beyond the batch,
> > > > > > thus breaking the loop due to !xa_is_sibling(), and computing the wrong
> > > > > > order. Thus ensure that the caller is aware of this by triggering a BUG
> > > > > > when the entry is a sibling entry.
> > > > > Is it possible to add a test case in lib/test_xarray.c for this?
> > > > > You can compile the tests with “make -C tools/testing/radix-tree”
> > > > > and run “./tools/testing/radix-tree/xarray”.
> > > > Sorry forgot to Cc you.
> > > > I can surely do that later, but does this patch look fine?
> > > I am not sure the exact situation you are describing, so I asked you
> > > to write a test case to demonstrate the issue. :)
> >
> > Suppose we have a shift-6 node having an order-9 entry => 8 - 1 = 7 siblings,
> > so assume the slots are at offset 0 till 7 in this node. If xas->xa_offset is 6,
> > then the code will compute order as 1 + xas->xa_node->shift = 7. So I mean to
> > say that the order computation must start from the beginning of the multi-slot
> > entries, that is, the non-sibling entry.
> Got it. Thanks for the explanation. It will be great to add this explanation
> to the commit log.
>
> I also notice that in the comment of xas_get_order() it says
> “Called after xas_load()” and xas_load() returns NULL or an internal
> entry for a sibling. So caller is responsible to make sure xas is not pointing
> to a sibling entry. It is good to have a check here.
>
> In terms of the patch, we are moving away from BUG()/BUG_ON(), so I wonder
> if there is a less disruptive way of handling this. Something like return
> -EINVAL instead with modified function comments and adding a comment
> at the return -EIVAL saying something like caller needs to pass
> a non-sibling entry.

What's the reason for moving away from BUG_ON()? I would think that it is
better that we don't have any overhead without the relevant debug config.
Also, returning any negative return value seems more disruptive :) we will
have to change all the callers to handle that, and in turn, handle that
for their callers, and so on.

> Best Regards,
> Yan, Zi