Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <380cde65-8794-cfbc-237f-30a8d7e9330c@grimberg.me>
Date:   Tue, 20 Jun 2023 16:49:45 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH blktests v1 1/3] nvme/048: Check for queue count check
 directly
Content-Language: en-US
To:     Daniel Wagner <dwagner@suse.de>, linux-nvme@lists.infradead.org
Cc:     linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Shin'ichiro Kawasaki <shinichiro@fastmail.com>,
        Hannes Reinecke <hare@suse.de>,
        James Smart <jsmart2021@gmail.com>,
        Martin Belanger <Martin.Belanger@dell.com>
References: <20230620132703.20648-1-dwagner@suse.de>
 <20230620132703.20648-2-dwagner@suse.de>
From:   Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <20230620132703.20648-2-dwagner@suse.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


> The test monitored the state changes live -> resetting -> connecting ->
> live, to figure out the queue count change was successful.
> 
> The fc transport is reconnecting very fast and the state transitions
> are not observed by the current approach.
> 
> So instead trying to monitor the state changes, let's just wait for the
> live state and the correct queue number.
> 
> As queue count is depending on the number of online CPUs we explicitly
> use 1 and 2 for the max_queue count. This means the queue_count value
> needs to reach either 2 or 3 (admin queue included).
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   tests/nvme/048 | 35 ++++++++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/nvme/048 b/tests/nvme/048
> index 81084f0440c2..3dc5169132de 100755
> --- a/tests/nvme/048
> +++ b/tests/nvme/048
> @@ -42,6 +42,26 @@ nvmf_wait_for_state() {
>   	return 0
>   }
>   
> +nvmf_wait_for_queue_count() {
> +	local subsys_name="$1"
> +	local queue_count="$2"
> +	local nvmedev
> +
> +	nvmedev=$(_find_nvme_dev "${subsys_name}")
> +
> +	queue_count_file="/sys/class/nvme-fabrics/ctl/${nvmedev}/queue_count"
> +
> +	nvmf_wait_for_state "${subsys_name}" "live" || return 1
> +
> +	queue_count=$((queue_count + 1))
> +	if grep -q "${queue_count}" "${queue_count_file}"; then
> +		return 0
> +	fi
> +
> +	echo "expected queue count ${queue_count} not set"
> +	return 1
> +}
> +
>   set_nvmet_attr_qid_max() {
>   	local nvmet_subsystem="$1"
>   	local qid_max="$2"
> @@ -56,10 +76,7 @@ set_qid_max() {
>   	local qid_max="$3"
>   
>   	set_nvmet_attr_qid_max "${subsys_name}" "${qid_max}"
> -
> -	# Setting qid_max forces a disconnect and the reconntect attempt starts
> -	nvmf_wait_for_state "${subsys_name}" "connecting" || return 1
> -	nvmf_wait_for_state "${subsys_name}" "live" || return 1
> +	nvmf_wait_for_queue_count "${subsys_name}" "${qid_max}" || return 1

Why not simply wait for live? The connecting is obviously racy...

>   
>   	return 0
>   }
> @@ -77,12 +94,8 @@ test() {
>   
>   	_setup_nvmet
>   
> -	hostid="$(uuidgen)"
> -	if [ -z "$hostid" ] ; then
> -		echo "uuidgen failed"
> -		return 1
> -	fi
> -	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
> +	hostid="${def_hostid}"
> +	hostnqn="${def_hostnqn}"
>   
>   	truncate -s "${nvme_img_size}" "${file_path}"
>   
> @@ -103,7 +116,7 @@ test() {
>   			echo FAIL
>   		else
>   			set_qid_max "${port}" "${subsys_name}" 1 || echo FAIL
> -			set_qid_max "${port}" "${subsys_name}" 128 || echo FAIL
> +			set_qid_max "${port}" "${subsys_name}" 2 || echo FAIL
>   		fi
>   
>   		_nvme_disconnect_subsys "${subsys_name}"