Re: [PATCH 2/2] lib/test_xarray.c: fix error assumptions on check_xa_multi_store_adv_add()

[Liam R. Howlett]

* Luis Chamberlain <mcgrof@xxxxxxxxxx> [240423 14:05]:
> While testing lib/test_xarray in userspace I've noticed we can fail with:
> 
> make -C tools/testing/radix-tree
> ./tools/testing/radix-tree/xarray
> 
> BUG at check_xa_multi_store_adv_add:749
> xarray: 0x55905fb21a00x head 0x55905fa1d8e0x flags 0 marks 0 0 0
> 0: 0x55905fa1d8e0x
> xarray: ../../../lib/test_xarray.c:749: check_xa_multi_store_adv_add: Assertion `0' failed.
> Aborted
> 
> We get a failure with a BUG_ON(), and that is because we actually can
> fail due to -ENOMEM, the check in xas_nomem() will fix this for us so
> it makes no sense to expect no failure inside the loop. So modify the
> check and since this is also useful for instructional purposes clarify
> the situation.

The default behaviour in the testing framework is to test the error
path, which is what you are seeing with the less likely return of
-ENOMEM.

> 
> The check for XA_BUG_ON(xa, xa_load(xa, index) != p) is already done
> at the end of the loop so just remove the bogus on inside the loop.
> 
> With this we now pass the test in both kernel and userspace:
> 
> In userspace:
> 
> ./tools/testing/radix-tree/xarray
> XArray: 149092856 of 149092856 tests passed
> 
> In kernel space:
> 
> XArray: 148257077 of 148257077 tests passed
> 
> Signed-off-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>

Reviewed-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>

> ---
>  lib/test_xarray.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_xarray.c b/lib/test_xarray.c
> index ebe2af2e072d..5ab35190aae3 100644
> --- a/lib/test_xarray.c
> +++ b/lib/test_xarray.c
> @@ -744,15 +744,20 @@ static noinline void check_xa_multi_store_adv_add(struct xarray *xa,
>  
>  	do {
>  		xas_lock_irq(&xas);
> -
>  		xas_store(&xas, p);
> -		XA_BUG_ON(xa, xas_error(&xas));
> -		XA_BUG_ON(xa, xa_load(xa, index) != p);
> -
>  		xas_unlock_irq(&xas);
> +		/*
> +		 * In our selftest case the only failure we can expect is for
> +		 * there not to be enough memory as we're not mimicking the
> +		 * entire page cache, so verify that's the only error we can run
> +		 * into here. The xas_nomem() which follows will ensure to fix
> +		 * that condition for us so to chug on on the loop.
> +		 */
> +		XA_BUG_ON(xa, xas_error(&xas) && xas_error(&xas) != -ENOMEM);
>  	} while (xas_nomem(&xas, GFP_KERNEL));
>  
>  	XA_BUG_ON(xa, xas_error(&xas));
> +	XA_BUG_ON(xa, xa_load(xa, index) != p);
>  }
>  
>  /* mimics page_cache_delete() */
> -- 
> 2.43.0
>