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. Best Regards, Yan, Zi
