Re: [PATCH blktests 2/4] nvme/071: test that nvmet sends no data for a failed read

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Oct 14, 2026 / 19:00, Andrea Parri wrote:
> Issue a failing Identify into a buffer filled with a sentinel and verify
> that the target transfers no data, with a successful Identify and a
> namespace read as controls. The test runs on every configured transport;
> only nvmet-fc failed it before the fix. Add a small helper because
> nvme-cli cannot prefill the data buffer.
> 
> The test fails over fc on the unpatched kernel and passes with the fix
> applied.

[...]

> diff --git a/tests/nvme/071 b/tests/nvme/071
> new file mode 100755
> index 0000000000000..5333bfac8db53
> --- /dev/null
> +++ b/tests/nvme/071
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2026 Andrea Parri
> +#
> +# Check that the NVMe target transfers no read data for a failed command.
> +# The host buffer is filled with a sentinel, so any data the target sends
> +# shows up as changed bytes.
> +#
> +# Regression test for patch "nvmet-fc: only send read data for successful
> +# commands".
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="verify nvmet does not transfer data for failed reads"
> +QUICK=1
> +
> +requires() {
> +	_nvme_requires
> +	_have_loop
> +	_require_kernel_nvme_target
> +	_have_src_program nvme-sentinel-read
> +}
> +
> +set_conditions() {
> +	_set_nvme_trtype "$@"
> +}
> +
> +# Issue an Identify with CNS $2 to controller $1 and print the NVMe status
> +# and the number of changed bytes in the 4096-byte data buffer.
> +_identify() {

In most cases, the function names that start with underscore are used for
helper functions in common scripts. This function is used only in this script
file, I suggest a function name that does not start with underscore. How about
issue_identify() ?

> +	src/nvme-sentinel-read "$1" 0x06 "$2" 4096 2>>"$FULL"
> +}
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	local changed ctrl ns status
> +
> +	_setup_nvmet
> +	# File backing: the namespace read below fails over loop with the
> +	# default loop device backing, for reasons unrelated to this test.
> +	_nvmet_target_setup --blkdev file
> +	_nvme_connect_subsys
> +
> +	ctrl=$(_find_nvme_dev "$def_subsysnqn")
> +	ns="/dev/${ctrl}n1"
> +
> +	# Identify Controller succeeds and must overwrite the sentinel.
> +	if ! read -r status changed < <(_identify "/dev/$ctrl" 0x01); then
> +		echo "successful Identify could not be issued"
> +	else
> +		echo "cns=0x01 status=$status changed=$changed" >>"$FULL"
> +		if ((status != 0 || changed == 0)); then
> +			echo "successful Identify returned status $status with $changed bytes"
> +		fi
> +	fi
> +
> +	# An unsupported CNS fails and must leave the sentinel intact.
> +	if ! read -r status changed < <(_identify "/dev/$ctrl" 0xff); then
> +		echo "failed Identify could not be issued"
> +	else
> +		echo "cns=0xff status=$status changed=$changed" >>"$FULL"
> +		if (((status & 0x7ff) != 2)); then
> +			echo "failed Identify returned status $status, expected 0x2"
> +		fi
> +		if ((changed != 0)); then
> +			echo "failed Identify transferred data: $changed bytes changed"
> +		fi
> +	fi


Do we really need this dd part below? As far as I understand, the core part of
this test case is Identify command part above.

> +
> +	if ! dd if="$ns" of=/dev/null bs=4096 count=256 iflag=direct \
> +	     status=none >>"$FULL" 2>&1; then
> +		echo "namespace read failed"
> +	fi
> +
> +	_nvme_disconnect_subsys
> +	_nvmet_target_cleanup
> +
> +	echo "Test complete"
> +}