On 2025/4/28 16:13, Shinichiro Kawasaki wrote:
> On Apr 28, 2025 / 12:32, Zhang Yi wrote:
>> On 2025/4/3 15:43, Shinichiro Kawasaki wrote:
> [...]
>>>> +
>>>> +setup_test_device() {
>>>> + if ! _configure_scsi_debug "$@"; then
>>>> + return 1
>>>> + fi
>>>
>>> In same manner as the 1st patch, I suggest to check
>>> /queue/write_zeroes_unmap
>>> here.
>>>
>>> if [[ ! -f /sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap
>>> ]]; then
>>> _exit_scsi_debug
>>> SKIP_REASONS+=("kernel does not support unmap write zeroes
>>> sysfs interface")
>>> return 1
>>> fi
>>>
>>> The caller will need to check setup_test_device() return value.
>>
>> Sure.
>>
>>>
>>>> +
>>>> + local dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
>>>> + local blk_sz="$(blockdev --getsz "$dev")"
>>>> + dmsetup create test --table "0 $blk_sz linear $dev 0"
>>>
>>> I suggest to call _real_dev() here, and echo back the device name.
>>>
>>> dpath=$(_real_dev /dev/mapper/test)
>>> echo ${dpath##*/}
>>>
>>> The bash parameter expansion ${xxx##*/} works in same manner as the basename
>>> command. The caller can receive the device name in a local variable. This
>>> will
>>> avoid a bit of code duplication, and allow to avoid _short_dev().
>>>
>>
>> I'm afraid this approach will not work since we may set the
>> SKIP_REASONS parameter. We cannot pass the device name in this
>> manner as it will overlook the SKIP_REASONS setting when the caller
>> invokes $(setup_test_device xxx), this function runs in a subshell.
>
> Ah, that's right. SKIP_REASONS modification in subshell won't work.
>
>>
>> If you don't like _short_dev(), I think we can pass dname through a
>> global variable, something like below:
>>
>> setup_test_device() {
>> ...
>> dpath=$(_real_dev /dev/mapper/test)
>> dname=${dpath##*/}
>> }
>>
>> if ! setup_test_device lbprz=0; then
>> return 1
>> fi
>> umap="$(< "/sys/block/${dname}/queue/write_zeroes_unmap")"
>>
>> What do you think?
>
> I think global variable is a bit dirty. So my suggestion is to still echo back
> the short device name from the helper, and set the SKIP_REASONS after calling
> the helper, as follows:
>
> diff --git a/tests/dm/003 b/tests/dm/003
> index 1013eb5..e00fa99 100755
> --- a/tests/dm/003
> +++ b/tests/dm/003
> @@ -20,13 +20,23 @@ device_requries() {
> }
>
> setup_test_device() {
> + local dev blk_sz dpath
> +
> if ! _configure_scsi_debug "$@"; then
> return 1
Hmm, if we encounter an error here, the test will be skipped instead of
returning a failure. This is not the expected outcome.
Thanks,
Yi.
> fi
>
> - local dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
> - local blk_sz="$(blockdev --getsz "$dev")"
> + if [[ ! -f
> /sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap ]]; then
> + _exit_scsi_debug
> + return 1
> + fi
> +
> + dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
> + blk_sz="$(blockdev --getsz "$dev")"
> dmsetup create test --table "0 $blk_sz linear $dev 0"
> +
> + dpath=$(_real_dev /dev/mapper/test)
> + echo ${dpath##*/}
> }
>
> cleanup_test_device() {
> @@ -38,17 +48,21 @@ test() {
> echo "Running ${TEST_NAME}"
>
> # disable WRITE SAME with unmap
> - setup_test_device lbprz=0
> - umap="$(cat "/sys/block/$(_short_dev
> /dev/mapper/test)/queue/write_zeroes_unmap")"
> + local dname
> + if ! dname=$(setup_test_device lbprz=0); then
> + SKIP_REASONS+=("kernel does not support unmap write zeroes
> sysfs interface")
> + return 1
> + fi
> + umap="$(cat "/sys/block/${dname}/queue/zoned")"
> if [[ $umap -ne 0 ]]; then
> echo "Test disable WRITE SAME with unmap failed."
> fi
> cleanup_test_device
>
