Re: [PATCH v2 blktests 1/2] scsi/009: add atomic write tests
From: Shinichiro Kawasaki <hidden>
Date: 2025-01-31 12:44:40
Also in:
linux-nvme, linux-scsi
Thanks for the v2 patches. Please find my comments below. On Jan 28, 2025 / 15:50, Alan Adamson wrote:
quoted hunk ↗ jump to hunk
Tests basic atomic write functionality. If no scsi test device is provided, a scsi_debug device will be used. 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 <redacted> --- common/xfs | 61 ++++++++++++ tests/scsi/009 | 233 +++++++++++++++++++++++++++++++++++++++++++++ tests/scsi/009.out | 18 ++++ 3 files changed, 312 insertions(+) create mode 100755 tests/scsi/009 create mode 100644 tests/scsi/009.outdiff --git a/common/xfs b/common/xfs index 569770fecd53..5db052be7e1c 100644 --- a/common/xfs +++ b/common/xfs@@ -6,6 +6,30 @@ . common/shellcheck +_have_xfs_io() { + if ! _have_program xfs_io; then + return 1 + fi + return 0 +}
This helper function is used only one place, so it does not add much value. I think "_have_program xfs_io" is enough for this patch. I would say null_blk and scsi_debug are exceptions. They are used at many places in blktests, so they have the value to have special helper _have_null_blk and _have_scsi_debug.
+ +# Check whether the version of xfs_io is greater than or equal to $1.$2.$3
This line should be removed.
+
+_have_xfs_io_atomic_write() {
+ local s
+
+ _have_xfs_io || return $?
+
+ # If the pwrite command supports the -A option then this version
+ # of xfs_io supports atomic writes.
+ s=$(xfs_io -c help | grep pwrite | awk '{ print $4}')
+ if [[ $s == *"A"* ]];
+ then
+ return 0
+ fi
SKIP_REASONS+=("..") should be set here, or the test cases are not skipped
even with older xfs_io.
quoted hunk ↗ jump to hunk
+ return 1 +} + _have_xfs() { _have_fs xfs && _have_program mkfs.xfs }@@ -52,3 +76,40 @@ _xfs_run_fio_verify_io() { return "${rc}" } + +# Use xfs_io to perform a non-atomic write using pwritev2(). +# Args: $1 - device to write to +# $2 - number of bytes to write +# Returns: Number of bytes written +run_xfs_io_pwritev2() { + local dev=$1 + local bytes_to_write=$2 + local bytes_written + + # Perform write and extract out bytes written from xfs_io output + 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 }')
This line is lengthy and hard to read. Can we split it with \ to fit in 80 characters?
+ echo "$bytes_written"
+}
+
+# Use xfs_io to perform an atomic write using pwritev2().
+# Args: $1 - device to write to
+# $2 - number of bytes to write
+# Returns: Number of bytes written
+run_xfs_io_pwritev2_atomic() {
+ local dev=$1
+ local bytes_to_write=$2
+ local bytes_written
+
+ # Perform atomic write and extract out bytes written from xfs_io output
+ 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.
quoted hunk ↗ jump to hunk
+ echo "$statx_output" +}diff --git a/tests/scsi/009 b/tests/scsi/009 new file mode 100755 index 000000000000..7624447a6633 --- /dev/null +++ b/tests/scsi/009@@ -0,0 +1,233 @@ +#!/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_xfs_io_atomic_write +} + +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 +}
This check in device_requires() looks exactly same as that in nvme/059. I suggest factor it out to a helper function in common/rc. Other than the above comments, this patch looks good to me.