Re: [RFC PATCH v3 0/2] Fix storing in XArray check_split tests

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Mar 16, 2026 at 05:23:17PM +0100, David Hildenbrand (Arm) wrote:
>On 2/23/26 08:34, Ackerley Tng wrote:
>> Hi,
>> 

Hi,

Hope I can help here.

>> I hit an assertion while making some modifications to
>> lib/test_xarray.c [1] and I believe this is the fix.
>> 
>> In check_split, the tests split the XArray node and then store values
>> after the split to verify that splitting worked. While storing and
>> retrieval works as expected, the node's metadata, specifically
>> node->nr_values, is not updated correctly.
>> 
>> This led to the assertion being hit in [1], since the storing process
>> did not increment node->nr_values sufficiently, while the erasing
>> process assumed the fully-incremented node->nr_values state.
>> 
>> Would like to check my understanding on these:
>> 
>> 1. In the multi-index xarray world, is node->nr_values definitely the
>>    total number of values *and siblings* in the node?
>> 

I think so.

As the comment of struct xa_node says:

 * @nr_values is the count of every element in ->slots which is
 * either a value entry or a sibling of a value entry.

And I play with xas_store() and xas_split(), then dump the xarray, which shows
nr_values counts value and its siblings.

>> 2. IIUC xas_store() has significantly different behavior when entry is
>>    NULL vs non-NULL: when entry is NULL, xas_store() does not make
>>    assumptions on the number of siblings and erases all the way till
>>    the next non-sibling entry. This sounds fair to me, but it's also
>>    kind of surprising that it is differently handled when entry is
>>    non-NULL, where xas_store() respects xas->xa_sibs.
>> 

Agree with your.

	max = xas->xa_offset + xas->xa_sibs;

	if (entry) {                          // non-NULL entry
		if (offset == max)            // respect xa_sibs
			break;
		if (!xa_is_sibling(entry))
			entry = xa_mk_sibling(xas->xa_offset);
	} else {
		if (offset == XA_CHUNK_MASK)  // NULL entry, run all way down..
			break;
	}
	next = xa_entry_locked(xas->xa, node, ++offset);
	if (!xa_is_sibling(next)) {           // .. until a non-sibling entry
		if (!entry && (offset > max)) // then respect xa_sibs
			break;
		first = next;
	}

This does has difference.  Confused a little.

This is the reason why we see the nr_values is not updated as expected. When
xas_store() an order 0 non-NULL entry, it just iterate once. Then count the
difference as 1 instead of total counts it represents.

>> 3. If xas_store() is dependent on its caller to set up xas correctly
>>    (also sounds fair), then there are places where xas_store() is
>>    used, like replace_page_cache_folio() or
>>    migrate_huge_page_move_mapping(), where xas is set up assuming 0
>>    order pages. Are those buggy?

This is a good question.

When I look into these two places, I noticed the purpose here is to replace an
existing folio in pagecache with another folio. This means the old data and
new data are neither "value". So we don't expect nr_values would change.

One place we would store "value" into pagecache is swap, IIUC. Maybe we need
to take a look into that place.

The rule seems to be not mixture store "value" and "non-value" into xarray, it
is safe.

>
>Zi, do you have any familiarity with that code and could help?
>
>Thanks!
>
>-- 
>Cheers,
>
>David

-- 
Wei Yang
Help you, Help me