[PATCH blktests v3 1/2] nvme/rc: fix nvme device readiness check after _nvme_connect_subsys

[Daniel Wagner]

On Fri, Aug 18, 2023 at 01:40:56PM +0900, Shin'ichiro Kawasaki wrote:
> The helper function _nvme_connect_subsys() creates a nvme device. It may
> take some time after the function call until the device gets ready for
> I/O. So it is expected that the test cases call _find_nvme_dev() after
> _nvme_connect_subsys() before I/O. _find_nvme_dev() returns the path of
> the created device, and it also waits for uuid and wwid sysfs attributes
> of the created device get ready. This wait works as the wait for the
> device I/O readiness.
> 
> However, this wait by _find_nvme_dev() has two problems. The first
> problem is missing call of _find_nvme_dev(). The test case nvme/047
> calls _nvme_connect_subsys() twice, but _find_nvme_dev() is called only
> for the first _nvme_connect_subsys() call. This causes too early I/O to
> the device with tcp transport [1]. Fix this by moving the wait for the
> device readiness from _find_nvme_dev() to _nvme_connect_subsys(). Also
> add --no-wait option to _nvme_connect_subsys(). It allows to skip the
> wait in _nvmet_passthru_target_connect() which has its own wait for
> device readiness.
> 
> The second problem is wrong paths for the sysfs attributes. The paths
> do not include namespace index, so the check for the attributes always
> fail. Still _find_nvme_dev() does 1 second wait and allows the device
> get ready for I/O in most cases, but this is not intended behavior.
> Fix this by checking sysfs paths with the namespace index. Get list of
> namespace indices for the sub-system and do the check for all indices.
> 
> On top of the checks for sysfs attributes, add 'udevadm settle' and a
> check for the created device file. These ensures that the create device
> is ready for I/O.
> 
> [1] https://lore.kernel.org/linux-block/CAHj4cs9GNohGUjohNw93jrr8JGNcRYC-ienAZz+sa7az1RK77w@mail.gmail.com/
> 
> Fixes: c766fccf3aff ("Make the NVMe tests more reliable")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>

just a minor nitpick but feel free to add:

Reviewed-by: Daniel Wagner <dwagner at suse.de>

> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 0b964e9..92eac06 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -428,6 +428,8 @@ _nvme_connect_subsys() {
>  	local keep_alive_tmo=""
>  	local reconnect_delay=""
>  	local ctrl_loss_tmo=""
> +	local no_wait=""

I suggest to use the default 'no_wait=false' here

> +	local i
>  
>  	while [[ $# -gt 0 ]]; do
>  		case $1 in
> @@ -483,6 +485,10 @@ _nvme_connect_subsys() {
>  				ctrl_loss_tmo="$2"
>  				shift 2
>  				;;
> +			--no-wait)
> +				no_wait=true
> +				shift 1
> +				;;
>  			*)
>  				positional_args+=("$1")
>  				shift
> @@ -532,6 +538,33 @@ _nvme_connect_subsys() {
>  	fi
>  
>  	nvme connect "${ARGS[@]}" 2> /dev/null
> +
> +	# Wait until device file and uuid/wwid sysfs attributes get ready for
> +	# all namespaces.
> +	if [[ -z ${no_wait} ]]; then

and do a

	if [[ "${no_wait}" = true ]] ; then

instead using a testing for an empty string.