Re: [PATCH v3 blktests 2/2] nvme/059: add atomic write tests

[John Garry <john.g.garry@oracle.com>]

On 05/02/2025 23:11, Alan Adamson wrote:
> Tests basic atomic write functionality using NVMe devices
> that support the AWUN and AWUPF Controller Atomic Parameters
> and NAWUN and NAWUPF Namespace Atomic Parameters.
> 
> Testing areas include:
> 
> - Verify sysfs atomic write attributes are consistent with
>    atomic write capablities advertised by the NVMe HW.
> 
> - 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)
>      - a write larger than atomic_write_unit_max_bytes
> 
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>

Generally this looks ok, but some comments below.

> ---
>   tests/nvme/059     | 147 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/nvme/059.out |  10 +++
>   2 files changed, 157 insertions(+)
>   create mode 100755 tests/nvme/059
>   create mode 100644 tests/nvme/059.out
> 
> diff --git a/tests/nvme/059 b/tests/nvme/059
> new file mode 100755
> index 000000000000..24b2bec645a8
> --- /dev/null
> +++ b/tests/nvme/059
> @@ -0,0 +1,147 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2025 Oracle and/or its affiliates
> +#
> +# Test NVMe Atomic Writes
> +
> +. tests/nvme/rc
> +. common/xfs
> +
> +DESCRIPTION="test atomic writes"
> +QUICK=1
> +
> +requires() {
> +	_nvme_requires
> +	_have_program nvme
> +	_have_xfs_io_atomic_write
> +}
> +
> +device_requires() {
> +	_require_device_support_atomic_writes
> +}
> +
> +test_device() {
> +	local ns_dev
> +	local ctrl_dev
> +	local queue_path
> +	local nvme_awupf
> +	local nvme_nsfeat
> +	local nvme_nsabp
> +	local atomic_max_bytes
> +	local statx_atomic_max
> +	local sysfs_atomic_max_bytes
> +	local sysfs_atomic_unit_max_bytes
> +	local sysfs_logical_block_size
> +	local bytes_written
> +	local bytes_to_write
> +	local test_desc
> +
> +	echo "Running ${TEST_NAME}"
> +	ns_dev=${TEST_DEV##*/}
> +	ctrl_dev=${ns_dev%n*}
> +	queue_path="${TEST_DEV_SYSFS}/queue/"
> +
> +	test_desc="TEST 1 - Verify sysfs attributes"
> +
> +	sysfs_logical_block_size=$(cat "$queue_path"/logical_block_size)
> +	sysfs_max_hw_sectors_kb=$(cat "$queue_path"/max_hw_sectors_kb)
> +	max_hw_bytes=$(( "$sysfs_max_hw_sectors_kb" * 1024 ))
> +	sysfs_atomic_max_bytes=$(cat "$queue_path"/atomic_write_max_bytes)
> +	sysfs_atomic_unit_max_bytes=$(cat "$queue_path"/atomic_write_unit_max_bytes)
> +	sysfs_atomic_unit_min_bytes=$(cat "$queue_path"/atomic_write_unit_min_bytes)
> +
> +	if [ "$max_hw_bytes" -ge "$sysfs_atomic_max_bytes" ] &&
> +		[ "$sysfs_atomic_max_bytes" -ge "$sysfs_atomic_unit_max_bytes" ] &&
> +		[ "$sysfs_atomic_unit_max_bytes" -ge "$sysfs_atomic_unit_min_bytes" ]
> +	then
> +		echo "$test_desc - pass"
> +	else
> +		echo "$test_desc - fail $max_hw_bytes - $sysfs_max_hw_sectors_kb -" \
> +			"$sysfs_atomic_max_bytes - $sysfs_atomic_unit_max_bytes -" \
> +			"$sysfs_atomic_unit_min_bytes"
> +	fi
> +
> +	test_desc="TEST 2 - Verify sysfs atomic_write_unit_max_bytes is consistent "
> +	test_desc+="with NVMe AWUPF/NAWUPF"
> +	nvme_nsfeat=$(nvme id-ns /dev/"${ns_dev}" | grep nsfeat | awk '{ print $3}')
> +	nvme_nsabp=$((("$nvme_nsfeat" & 0x2) != 0))
> +	if [ "$nvme_nsabp" = 1 ] # Check if NSABP is set
> +	then
> +		nvme_awupf=$(nvme id-ns /dev/"$ns_dev" | grep nawupf | awk '{ print $3}')
> +		atomic_max_bytes=$(( ("$nvme_awupf" + 1) * "$sysfs_logical_block_size" ))
> +	else
> +		nvme_awupf=$(nvme id-ctrl /dev/"${ctrl_dev}" | grep awupf | awk '{ print $3}')
> +		atomic_max_bytes=$(( ("$nvme_awupf" + 1) * "$sysfs_logical_block_size" ))
> +	fi
> +	if [ "$atomic_max_bytes" -le "$max_hw_bytes" ]
> +	then
> +		if [ "$atomic_max_bytes" = "$sysfs_atomic_max_bytes" ]
> +		then
> +			echo "$test_desc - pass"
> +		else
> +			echo "$test_desc - fail $nvme_nsabp - $atomic_max_bytes - $sysfs_atomic_max_bytes -" \
> +				"$max_hw_bytes"
> +		fi
> +	else
> +		if [ "$sysfs_atomic_max_bytes" = "$max_hw_bytes" ]
> +		then
> +			echo "$test_desc - pass"
> +		else
> +			echo "$test_desc - fail $nvme_nsabp - $atomic_max_bytes - $sysfs_atomic_max_bytes -" \
> +				"$max_hw_bytes"
> +		fi
> +	fi
> +
> +	test_desc="TEST 3 - Verify statx is correctly reporting atomic_unit_max_bytes"

I suppose that you can also check atomic_unit_min_bytes

And min can be checked in other tests (when applicable).

> +	statx_atomic_max=$(run_xfs_io_xstat /dev/"$ns_dev" "stat.atomic_write_unit_max")
> +	if [ "$sysfs_atomic_unit_max_bytes" = "$statx_atomic_max" ]
> +	then
> +		echo "$test_desc - pass"
> +	else
> +		echo "$test_desc - fail $statx_atomic_max - $sysfs_atomic_unit_max_bytes"
> +	fi
> +
> +	test_desc="TEST 4 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes "\
> +	test_desc+="with no RWF_ATOMIC"
> +	# flag - pwritev2 should be succesful.
> +        bytes_written=$(run_xfs_io_pwritev2 /dev/"$ns_dev" "$sysfs_atomic_unit_max_bytes")
> +        if [ "$bytes_written" = "$sysfs_atomic_unit_max_bytes" ]
> +        then
> +                echo "$test_desc - pass"
> +        else
> +                echo "$test_desc - fail $bytes_written - $sysfs_atomic_unit_max_bytes"
> +        fi
> +
> +	test_desc="TEST 5 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes with "
> +	test_desc+="RWF_ATOMIC flag - pwritev2 should  be succesful"
> +	bytes_written=$(run_xfs_io_pwritev2_atomic /dev/"$ns_dev" "$sysfs_atomic_unit_max_bytes")
> +	if [ "$bytes_written" = "$sysfs_atomic_unit_max_bytes" ]
> +	then
> +		echo "$test_desc - pass"
> +	else
> +		echo "$test_desc - fail $bytes_written - $sysfs_atomic_unit_max_bytes"
> +	fi
> +
> +	test_desc="TEST 6 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + 1 logical "
> +	test_desc+="block with no RWF_ATOMIC flag - pwritev2 should be succesful"

I don't really see much value in this test. If this doesn't pass, then 
something is really wrong.

> +	bytes_to_write=$(( "$sysfs_atomic_unit_max_bytes" + "$sysfs_logical_block_size" ))
> +	bytes_written=$(run_xfs_io_pwritev2 /dev/"$ns_dev" "$bytes_to_write")
> +	if [ "$bytes_written" = "$bytes_to_write" ]
> +	then
> +		echo "$test_desc - pass"
> +	else
> +		echo "$test_desc - fail $bytes_written - $bytes_to_write"
> +	fi
> +
> +	test_desc="TEST 7 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + logical "
> +	test_desc+="block with RWF_ATOMIC flag - pwritev2 should not be succesful"

nit: I really don't think that "succesful" is the proper spelling

> +	bytes_written=$(run_xfs_io_pwritev2_atomic /dev/"$ns_dev" "$bytes_to_write")
> +	if [ "$bytes_written" = "" ]
> +	then
> +		echo "$test_desc - pass"
> +	else
> +		echo "$test_desc - fail $bytes_written - $bytes_to_write"
> +	fi
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/059.out b/tests/nvme/059.out
> new file mode 100644
> index 000000000000..e803de35776f
> --- /dev/null
> +++ b/tests/nvme/059.out
> @@ -0,0 +1,10 @@
> +Running nvme/059
> +TEST 1 - Verify sysfs attributes - pass
> +TEST 2 - Verify sysfs atomic_write_unit_max_bytes is consistent with NVMe AWUPF/NAWUPF - pass
> +TEST 3 - Verify statx is correctly reporting atomic_unit_max_bytes - pass
> +TEST 4 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes with no RWF_ATOMIC - pass
> +TEST 5 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes with RWF_ATOMIC flag - pwritev2 should  be succesful - pass
> +TEST 6 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + 1 logical block with no RWF_ATOMIC flag - pwritev2 should be succesful - pass
> +pwrite: Invalid argument
> +TEST 7 - perform a pwritev2 with size of sysfs_atomic_unit_max_bytes + logical block with RWF_ATOMIC flag - pwritev2 should not be succesful - pass
> +Test complete