Re: [PATCH blktests 0/7] Further stacked device atomic writes testing

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

On 16/09/2025 13:23, Shinichiro Kawasaki wrote:
> On Sep 16, 2025 / 12:55, John Garry wrote:
>> On 16/09/2025 11:20, John Garry wrote:
>>>> also modified md/003 to adapt to the change [2].
>>>>
>>>> [1]
>>>> https://urldefense.com/v3/__https://github.com/kawasaki/blktests/
>>>> commit/7db35a16d7410cae728da8d6b9b2483e33e9c99b__;!!ACWV5N9M2RV99hQ! Lm9AlQ3T9qSGDEjCR0nGmjCGC_2wfuAbkP6zN9EfZD7L2Y7mgpKPah_fYZh6L_OPkH9IIxP4f9n1Q9dRRRJxbZQeqRtr$
>>>> [2]
>>>> https://urldefense.com/v3/__https://github.com/kawasaki/blktests/
>>>> commit/278e6c74deba68e3044abf0e6c3ec350c0bc4a40__;!!ACWV5N9M2RV99hQ! Lm9AlQ3T9qSGDEjCR0nGmjCGC_2wfuAbkP6zN9EfZD7L2Y7mgpKPah_fYZh6L_OPkH9IIxP4f9n1Q9dRRRJxbSlcsw0E$
>>>>
>>>> Please let me know your thoughts on the two approaches.
>>>
>>> Let me check it, thanks!
>>
>> I gave it a spin for 003 and it looks to work ok - thanks!
> 
> Sounds good! Then let's seek for this solution approach :)

ok, great!

> 
> Let me have a day or two to improve the patch [1]. I rethough and now I think
> TEST_DEV_ARRAY values will be test case dependent. When we have another test
> case to have multiple devices, the new test will require different set of
> devices from md/002, probably. So TEST_DEV_ARRAY can be an associative array:
> 
>    TEST_DEV_ARRAY[md/002]="/dev/nvme0n1 /dev/nvme1n1 /dev/nvme2n1 /dev/nvvme3n1"
> 
> I need to do some trials to see if this idea is feasible.

ok

> 
>>
>> I further comment I have on my own code is about this snippet from 003:
>>
>> 	for ((i = 0; i < ${#NVME_TEST_DEVS[@]}; ++i)); do
>> 		TEST_DEV_SYSFS="${NVME_TEST_DEVS_SYSFS[$i]}"
>> 		TEST_DEV="${NVME_TEST_DEVS[$i]}"
>> 		_require_device_support_atomic_writes
>> 		_require_test_dev_size 16m
>> 	done
>>
>> Notice that I set TEST_DEV_SYSFS and TEST_DEV, as these are required for the
>> _require_device_support_atomic_writes and _require_test_dev_size calls. I'm
>> just trying to reuse helpers normally used for test_device(). Is this ok to
>> do so? I'm not sure if it is a bit of a bodge...
> 
> Not really, TEST_DEV_SYSFS and TEST_DEV are part of the interface between the
> blktests framework and test cases. They should be set only by the blktests
> framework, and test cases should not modify them. My prototype patch [1]
> ensures that device_requires() is called for each element of the
> TEST_DEV_ARRAY. Then, the code snippet you quoted can be replaced as follows:

ok, good

> 
> diff --git a/tests/md/003 b/tests/md/003
> index 94c1132..765c4cc 100755
> --- a/tests/md/003
> +++ b/tests/md/003
> @@ -14,6 +14,11 @@ requires() {
>   	_nvme_requires
>   }
>   
> +device_requires() {
> +	_require_device_support_atomic_writes

Incidentally, I think that we can drop this, as it is worth testing 
stacking of devices which don't support atomic writes (to unsure that 
the stacked device also do not support atomic writes).

> +	_require_test_dev_size 16m
> +}
> +
>   test_device_array() {
>   	local ns
>   	local testdev_count=0
> @@ -33,13 +38,6 @@ test_device_array() {
>   		fi
>   	done
>   

Thanks for the help!