On 2025/4/28 17:25, Shinichiro Kawasaki wrote:
> On Apr 28, 2025 / 17:04, Zhang Yi wrote:
>> 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.
>
> Ah, rigth. That's not good.
> How about to return differnt values for the failure case above,
>
>>
>> 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
>
> and this "should skip" case?
>
>
> diff --git a/tests/dm/003 b/tests/dm/003
> index 1013eb5..5e617fd 100755
> --- a/tests/dm/003
> +++ b/tests/dm/003
> @@ -19,14 +19,26 @@ device_requries() {
> _require_test_dev_sysfs queue/write_zeroes_unmap
> }
>
> +readonly TO_SKIP=255
> +
> setup_test_device() {
> + local dev blk_sz dpath
> +
> if ! _configure_scsi_debug "$@"; then
> return 1
> 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 $TO_SKIP
> + 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 +50,25 @@ 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
> + dname=$(setup_test_device lbprz=0)
> + ret=$?
> + if ((ret)); then
> + if ((ret == TO_SKIP)); then
> + SKIP_REASONS+=("kernel does not support unmap write
> zeroes sysfs interface")
> + fi
> + 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
>
Yeah, I believe this will work, and it looks good to me. Thank you for
the suggestion！
Thanks,
Yi.
