Re: [PATCH blktests 2/7] md/rc: add _md_atomics_test

[John Garry <john.g.garry@oracle.com>]

On 18/09/2025 05:17, Shinichiro Kawasaki wrote:
> On Sep 12, 2025 / 09:57, John Garry wrote:
>> The stacked device atomic writes testing is currently limited.
>>
>> md/002 currently only tests scsi_debug. SCSI does not support atomic
>> boundaries, so it would be nice to test NVMe (which does support them).
>>
>> Furthermore, the testing in md/002 for chunk boundaries is very limited,
>> in that we test once one boundary value. Indeed, for RAID0 and RAID10, a
>> boundary should always be set for testing.
>>
>> Finally, md/002 only tests md RAID0/1/10. In future we will also want to
>> test the following stacked device personalities which support atomic
>> writes:
>> - md-linear (being upstreamed)
>> - dm-linear
>> - dm-stripe
>> - dm-mirror
>>
>> To solve all those problems, add a generic test handler,
>> _md_atomics_test(). This can be extended for more extensive testing.
>>
>> This test handler will accept a group of devices and test as follows:
>> a. calculate expected atomic write limits based on device limits
>> b. Take results from a., and refine expected limits based on any chunk
>>     size
>> c. loop through creating a stacked device for different chunk size. We loop
>>     once for any personality which does not have a chunk size, e.g. RAID1
>> d. test sysfs and statx limits vs what is calculated in a. and b.
>> e. test RWF_ATOMIC is accepted or rejected as expected
>>
>> Steps c, d, and e are really same as md/002.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   tests/md/rc | 372 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 372 insertions(+)
>>
>> diff --git a/tests/md/rc b/tests/md/rc
>> index 96bcd97..105d283 100644
>> --- a/tests/md/rc
>> +++ b/tests/md/rc
>> @@ -5,9 +5,381 @@
>>   # Tests for md raid
>>   
>>   . common/rc
>> +. common/xfs
>>   
>>   group_requires() {
>> +	_have_kver 6 14 0
>>   	_have_root
>>   	_have_program mdadm
>> +	_have_xfs_io_atomic_write
> 
> I don't think either "_have_kver 6 14 0" or "_have_xfs_io_atomic_write" is
> required for md/001. I suggest to introduce a new helper,
> 
>   _stacked_atomic_test_requires() {
>       _have_kver 6 14 0
>       _have_xfs_io_atomic_write
>   }
> 
> and call it from requires() of md/002 and md/003.

ok, fine

> 
>> +	_have_driver raid0
>> +	_have_driver raid1
>> +	_have_driver raid10
>>   	_have_driver md-mod
>>   }
>> +
>> +declare -A MD_DEVICES
>> +
>> +_max_pow_of_two_factor() {
>> +	part1=$1
>> +	part2=-$1
>> +	retval=$(($part1 & $part2))
> 
> Nit: "local" declarations are missing for part1, part2 and retval.
> Same comment for some other local variables introduced by this patch.
> 

ok, will fix

>> +	echo "$retval"
>> +}
>> +
>> +# Find max atomic size given a boundary and chunk size
>> +# @unit is set if we want atomic write "unit" size, i.e power-of-2
>> +# @chunk must be > 0
>> +_md_atomics_boundaries_max() {
>> +	boundary=$1
>> +	chunk=$2
>> +	unit=$3
>> +
>> +	if [ "$boundary" -eq 0 ]
>> +	then
>> +		if [ "$unit" -eq 1 ]
>> +		then
>> +			retval=$(_max_pow_of_two_factor $chunk)
>> +			echo "$retval"
>> +			return 1
>> +		fi
>> +
>> +		echo "$chunk"
>> +		return 1
> 
> When bash functions return non-zero value, it implies the functions failed.
> When this function returns 1 at the line above, does it indicate failure?
> It looks echoing back a good number, so I guess just "return" is more
> appropriate. Same comment for other "return 1" in this function.

this function should not fail, so I will just use "return"

> 
>> +	fi
>> +
>> +	# boundary is always a power-of-2
>> +	if [ "$boundary" -eq "$chunk" ]
>> +	then
>> +		echo "$boundary"
>> +		return 1
>> +	fi
>> +
>> +	if [ "$boundary" -gt "$chunk" ]
>> +	then
>> +		if (( $boundary % $chunk == 0))
>> +		then
>> +			if [ "$unit" -eq 1 ]
>> +			then
>> +				retval=$(_max_pow_of_two_factor $chunk)
>> +				echo "$retval"
>> +				return 1
>> +			fi
>> +			echo "$chunk"
>> +			return 1
>> +		fi
>> +		echo "0"
>> +		return 1
>> +	fi
>> +
>> +	if (( $chunk % $boundary == 0))
>> +	then
>> +		echo "$boundary"
>> +		return 1
>> +	fi
>> +
>> +	echo "0"
>> +}

Thanks,
John