Thanks for the v2 patches. Please find my comments below.
On Jan 28, 2025 / 15:50, Alan Adamson wrote:
> 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 <alan.adamson@xxxxxxxxxx>
> ---
> 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.out
>
> diff --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.
> + 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.
> + 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.
