On Jan 22, 2025 / 00:51, Chaitanya Kulkarni wrote:
> On 1/21/25 14:25, Alan Adamson wrote:
> > Uses scsi_debug to test basic atomic write functionality. Testing
> > areas include:
> >
> > - Verify sysfs atomic write attributes are consistent with
> > atomic write attributes advertised by scsi_debug.
> > - Verify the atomic write paramters of statx are correct using
> > xfs_io.
> > - Perform a pwritev2() (with and without RWF_ATOMIC flag) using
> > xfs_io:
> > - maximum byte size (atomic_write_unit_max_bytes)
> > - minimum byte size (atomic_write_unit_min_bytes)
> > - a write larger than atomic_write_unit_max_bytes
> > - a write smaller than atomic_write_unit_min_bytes
> >
> > Signed-off-by: Alan Adamson <alan.adamson@xxxxxxxxxx>
>
> Thanks a lot for the testcase, this is really useful based on
> amount of work has been done on the kernel side.
Agreed, I also think these test cases are valuable :)
>
> > ---
> > common/xfs | 49 +++++++++++
> > tests/scsi/009 | 213 +++++++++++++++++++++++++++++++++++++++++++++
> > tests/scsi/009.out | 18 ++++
> > 3 files changed, 280 insertions(+)
> > create mode 100755 tests/scsi/009
> > create mode 100644 tests/scsi/009.out
> >
> > diff --git a/common/xfs b/common/xfs
> > index 569770fecd53..284c6d7cdc40 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -6,6 +6,28 @@
> >
> > . common/shellcheck
> >
> > +_have_xfs_io() {
> > + if ! _have_program xfs_io; then
> > + return 1
> > + fi
> > + return 0
> > +}
> > +
> > +# Check whether the version of xfs_io is greater than or equal to $1.$2.$3
> > +_have_xfs_io_ver() {
> > + local d=$1 e=$2 f=$3
> > +
> > + _have_xfs_io || return $?
> > +
> > + IFS='.' read -r a b c < <(xfs_io -V | sed 's/xfs_io version *//')
> > + if [ $((a * 65536 + b * 256 + c)) -lt $((d * 65536 + e * 256 + f)) ];
>
> can we add some comments for above calculations ?
These are checking xfs_io command version, and I think the numbers and the logic
were copied form _have_kver().
Anyway, I wonder if we really need this helper function. In general, version
dependency is not the best approach and we should avoid them as much as we can.
Instead, I suggest to check output of "xfs_io -c help" command. The old version,
the output is as follows and the -A option is not printed.
$ xfs_io -c help | grep pwrite
pwrite [-i infile [-qdDwNOW] [-s skip]] [-b bs] [-S seed] [-FBR [-Z N]] [-V N] off len -- writes a number of bytes at a specified offset
With this approach, we can implement a helper function with name
_have_xfs_io_atomic_write() or something, and call it from requires() in
scsi/009 and nvme/059.
>
> > + then
> > + SKIP_REASONS+=("xfs_io version too old")
> > + return 1
> > + fi
> > + return 0
> > +}
> > +
> > _have_xfs() {
> > _have_fs xfs && _have_program mkfs.xfs
> > }
> > @@ -52,3 +74,30 @@ _xfs_run_fio_verify_io() {
> >
> > return "${rc}"
> > }
> > +
> > +run_xfs_io_pwritev2() {
> > + local dev=$1
> > + local bytes_to_write=$2
> > + local bytes_written
> > +
> > + bytes_written=$(xfs_io -d -C "pwrite -b ${bytes_to_write} -V 1 -D 0 ${bytes_to_write}" "$dev" | grep "wrote" | sed 's/\// /g' | awk '{ print $2 }')
>
> same here little comment would be really useful
>
> > + echo "$bytes_written"
> > +}
> > +
> > +run_xfs_io_pwritev2_atomic() {
> > + local dev=$1
> > + local bytes_to_write=$2
> > + local bytes_written
> > +
> > + bytes_written=$(xfs_io -d -C "pwrite -b ${bytes_to_write} -V 1 -A -D 0 ${bytes_to_write}" "$dev" | grep "wrote" | sed 's/\// /g' | awk '{ print $2 }')
>
> same here
>
> > + echo "$bytes_written"
> > +}
> > +
> > +run_xfs_io_xstat() {
> > + local dev=$1
> > + local field=$2
> > + local statx_output
> > +
> > + statx_output=$(xfs_io -c "statx -r -m 0x00010000" "$dev" | grep "$field" | awk '{ print $3 }')
>
> same here
>
> > + echo "$statx_output"
> > +}
> > diff --git a/tests/scsi/009 b/tests/scsi/009
> > new file mode 100755
> > index 000000000000..f3ab00f61369
> > --- /dev/null
> > +++ b/tests/scsi/009
> > @@ -0,0 +1,213 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-3.0+
> > +# Copyright (C) 2025 Oracle and/or its affiliates
> > +#
> > +# Test SCSI Atomic Writes with scsi_debug
> > +
> > +. tests/scsi/rc
> > +. common/scsi_debug
> > +. common/xfs
> > +
> > +DESCRIPTION="test scsi atomic writes"
> > +QUICK=1
> > +
> > +requires() {
> > + _have_driver scsi_debug
> > + _have_kver 6 11
I suggest to remove this kernel version dependency also. Let me describe how to
do it below.
> > + _have_xfs_io_ver 6 12 0
> > +}
> > +
> > +test() {
> > + local dev
> > + local scsi_debug_atomic_wr_max_length
> > + local scsi_debug_atomic_wr_gran
> > + local scsi_atomic_max_bytes
> > + local scsi_atomic_min_bytes
> > + local sysfs_max_hw_sectors_kb
> > + local max_hw_bytes
> > + local sysfs_logical_block_size
> > + local sysfs_atomic_max_bytes
> > + local sysfs_atomic_unit_max_bytes
> > + local sysfs_atomic_unit_min_bytes
> > + local statx_atomic_min
> > + local statx_atomic_max
> > + local bytes_to_write
> > + local bytes_written
> > +
> > + echo "Running ${TEST_NAME}"
> > +
> > + local scsi_debug_params=(
> > + delay=0
> > + atomic_wr=1
> > + )
> > + _configure_scsi_debug "${scsi_debug_params[@]}"
> > + dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
> > + sysfs_logical_block_size=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/logical_block_size)
>
> you can also use the local variable for following path to trim down the
> cat operations where :-
>
> /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue
>
> makes the code easy to read ...
>
> > + sysfs_max_hw_sectors_kb=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/max_hw_sectors_kb)
> > + max_hw_bytes=$(( "$sysfs_max_hw_sectors_kb" * 1024 ))
> > + sysfs_atomic_max_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_max_bytes)
> > + sysfs_atomic_unit_max_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_unit_max_bytes)
> > + sysfs_atomic_unit_min_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_unit_min_bytes)
I agree that these lines are lengthy.
Taking this chance, let me suggest to use fallback_device() and
cleanup_fallback_device(). (They are not well documented, but are used in some
test cases like zbd/001 or block/007). When a test case with test_device()
implements these hooks, blktests runs the test case even when TEST_DEVS is
empty. It calls fallback_device() to set up the test target TEST_DEV then run
test_device(). After the test completion, it calls cleanup_fallback_device() to
clean up TEST_DEV. For this test case, fallback_device() can call
_configure_scsi_debug with atmoic_wr=1 option. With this approach, we can refer
to TEST_DEV and TEST_DEV_SYSFS in test_device().
This approach has following benefits:
- Using TEST_DEV_SYSFS, the sysfs attribute reference codes will simpler and
similar as nvme/059 in the next patch.
- We can call "_require_test_dev_sysfs queue/atomic_write_max_bytes" in
device_requires to check that the kernel supports amotic writes. This will
avoid the kernel version dependency.
- If users have real scsi devices with atomic write support, the users can
specify the devices in TEST_DEVS and run this test case with them.
(We need to skip the test if the devices do not support atomic writes,
by checking the queue/atomic_write_max_bytes value in device_requires())
For your reference, here I share the change needed for this approach. It is
untested. I hope it is enough to convey my idea.
diff --git a/tests/scsi/009 b/tests/scsi/009
index f3ab00f..686cc8a 100755
--- a/tests/scsi/009
+++ b/tests/scsi/009
@@ -13,12 +13,34 @@ QUICK=1
requires() {
_have_driver scsi_debug
- _have_kver 6 11
- _have_xfs_io_ver 6 12 0
}
-test() {
- local dev
+device_requires() {
+ _require_test_dev_sysfs queue/atomic_write_max_bytes
+ if (( $(< ${TEST_DEV_SYSFS}/queue/atomic_write_max_bytes) == 0 )); then
+ SKIP_REASONS+=("${TEST_DEV} does not support atomic write")
+ return 1
+ fi
+}
+
+fallback_device() {
+ local scsi_debug_params=(
+ delay=0
+ atomic_wr=1
+ )
+ if ! _configure_scsi_debug "${scsi_debug_params[@]}"; then
+ return 1
+ fi
+ echo "/dev/${SCSI_DEBUG_DEVICES[0]}"
+}
+
+cleanup_fallback_device() {
+ _exit_scsi_debug
+
+}
+
+test_device() {
local scsi_debug_atomic_wr_max_length
local scsi_debug_atomic_wr_gran
local scsi_atomic_max_bytes
@@ -36,20 +58,14 @@ test() {
echo "Running ${TEST_NAME}"
- local scsi_debug_params=(
- delay=0
- atomic_wr=1
- )
- _configure_scsi_debug "${scsi_debug_params[@]}"
- dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
- sysfs_logical_block_size=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/logical_block_size)
- sysfs_max_hw_sectors_kb=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/max_hw_sectors_kb)
+ sysfs_logical_block_size=$(< "${TEST_DEV_SYSFS}"/queue/logical_block_size)
+ sysfs_max_hw_sectors_kb=$(< "${TEST_DEV_SYSFS}"/queue/max_hw_sectors_kb)
max_hw_bytes=$(( "$sysfs_max_hw_sectors_kb" * 1024 ))
- sysfs_atomic_max_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_max_bytes)
- sysfs_atomic_unit_max_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_unit_max_bytes)
- sysfs_atomic_unit_min_bytes=$(cat /sys/block/"${SCSI_DEBUG_DEVICES[0]}"/queue/atomic_write_unit_min_bytes)
- scsi_debug_atomic_wr_max_length=$(cat /sys/module/scsi_debug/parameters/atomic_wr_max_length)
- scsi_debug_atomic_wr_gran=$(cat /sys/module/scsi_debug/parameters/atomic_wr_gran)
+ sysfs_atomic_max_bytes=$(< "${TEST_DEV_SYSFS}"/queue/atomic_write_max_bytes)
+ sysfs_atomic_unit_max_bytes=$(< "${TEST_DEV_SYSFS}"/queue/atomic_write_unit_max_bytes)
+ sysfs_atomic_unit_min_bytes=$(< "${TEST_DEV_SYSFS}"/queue/atomic_write_unit_min_bytes)
+ scsi_debug_atomic_wr_max_length=$(< /sys/module/scsi_debug/parameters/atomic_wr_max_length)
+ scsi_debug_atomic_wr_gran=$(< /sys/module/scsi_debug/parameters/atomic_wr_gran)
scsi_atomic_max_bytes=$(( "$scsi_debug_atomic_wr_max_length" * "$sysfs_logical_block_size" ))
scsi_atomic_min_bytes=$(( "$scsi_debug_atomic_wr_gran" * "$sysfs_logical_block_size" ))
@@ -102,7 +118,7 @@ test() {
fi
# TEST 5 - check statx stx_atomic_write_unit_min
- statx_atomic_min=$(run_xfs_io_xstat "$dev" "stat.atomic_write_unit_min")
+ statx_atomic_min=$(run_xfs_io_xstat "$TEST_DEV" "stat.atomic_write_unit_min")
if [ "$statx_atomic_min" = "$scsi_atomic_min_bytes" ]
then
echo "TEST 5 - pass"
@@ -111,7 +127,7 @@ test() {
fi
# TEST 6 - check statx stx_atomic_write_unit_max
- statx_atomic_max=$(run_xfs_io_xstat "$dev" "stat.atomic_write_unit_max")
+ statx_atomic_max=$(run_xfs_io_xstat "$TEST_DEV" "stat.atomic_write_unit_max")
if [ "$statx_atomic_max" = "$sysfs_atomic_unit_max_bytes" ]
then
echo "TEST 6 - pass"
@@ -121,7 +137,7 @@ test() {
# TEST 7 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes with no RWF_ATOMIC flag - pwritev2 should
# be succesful.
- bytes_written=$(run_xfs_io_pwritev2 "$dev" "$sysfs_atomic_unit_max_bytes")
+ bytes_written=$(run_xfs_io_pwritev2 "$TEST_DEV" "$sysfs_atomic_unit_max_bytes")
if [ "$bytes_written" = "$sysfs_atomic_unit_max_bytes" ]
then
echo "TEST 7 - pass"
@@ -131,7 +147,7 @@ test() {
# TEST 8 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes with RWF_ATOMIC flag - pwritev2 should
# be succesful.
- bytes_written=$(run_xfs_io_pwritev2_atomic "$dev" "$sysfs_atomic_unit_max_bytes")
+ bytes_written=$(run_xfs_io_pwritev2_atomic "$TEST_DEV" "$sysfs_atomic_unit_max_bytes")
if [ "$bytes_written" = "$sysfs_atomic_unit_max_bytes" ]
then
echo "TEST 8 - pass"
@@ -142,7 +158,7 @@ test() {
# TEST 9 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + 512 bytes with no RWF_ATOMIC flag - pwritev2
# should be succesful.
bytes_to_write=$(( "${sysfs_atomic_unit_max_bytes}" + "$sysfs_logical_block_size" ))
- bytes_written=$(run_xfs_io_pwritev2 "$dev" "$bytes_to_write")
+ bytes_written=$(run_xfs_io_pwritev2 "$TEST_DEV" "$bytes_to_write")
if [ "$bytes_written" = "$bytes_to_write" ]
then
echo "TEST 9 - pass"
@@ -152,7 +168,7 @@ test() {
# TEST 10 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + 512 bytes with RWF_ATOMIC flag - pwritev2
# should not be succesful.
- bytes_written=$(run_xfs_io_pwritev2_atomic "$dev" "$bytes_to_write")
+ bytes_written=$(run_xfs_io_pwritev2_atomic "$TEST_DEV" "$bytes_to_write")
if [ "$bytes_written" = "" ]
then
echo "TEST 10 - pass"
@@ -162,7 +178,7 @@ test() {
# TEST 11 - perform a pwritev2 with size of sysfs_atomic_unit_min_bytes with no RWF_ATOMIC flag - pwritev2 should
# be succesful.
- bytes_written=$(run_xfs_io_pwritev2 "$dev" "$sysfs_atomic_unit_min_bytes")
+ bytes_written=$(run_xfs_io_pwritev2 "$TEST_DEV" "$sysfs_atomic_unit_min_bytes")
if [ "$bytes_written" = "$sysfs_atomic_unit_min_bytes" ]
then
echo "TEST 11 - pass"
@@ -172,7 +188,7 @@ test() {
# TEST 12 - perform a pwritev2 with size of sysfs_atomic_unit_min_bytes with RWF_ATOMIC flag - pwritev2 should
# be succesful.
- bytes_written=$(run_xfs_io_pwritev2_atomic "$dev" "$sysfs_atomic_unit_min_bytes")
+ bytes_written=$(run_xfs_io_pwritev2_atomic "$TEST_DEV" "$sysfs_atomic_unit_min_bytes")
if [ "$bytes_written" = "$sysfs_atomic_unit_min_bytes" ]
then
echo "TEST 12 - pass"
@@ -191,14 +207,14 @@ test() {
echo "TEST 13 - pass"
echo "TEST 14 - pass"
else
- bytes_written=$(run_xfs_io_pwritev2 "$dev" "$bytes_to_write")
+ bytes_written=$(run_xfs_io_pwritev2 "$TEST_DEV" "$bytes_to_write")
if [ "$bytes_written" = "$bytes_to_write" ]
then
echo "TEST 13 - pass"
else
echo "TEST 13 - fail $bytes_written - $bytes_to_write"
fi
- bytes_written=$(run_xfs_io_pwritev2_atomic "$dev" "$bytes_to_write")
+ bytes_written=$(run_xfs_io_pwritev2_atomic "$TEST_DEV" "$bytes_to_write")
if [ "$bytes_written" = "" ]
then
echo "TEST 14 - pass"
@@ -207,7 +223,5 @@ test() {
fi
fi
- _exit_scsi_debug
-
echo "Test complete"
}
