[PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Guixin Liu]

Use def_nsid instead of hard code, the default of def_nsid is 1.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 common/nvme    | 3 ++-
 tests/nvme/051 | 2 +-
 tests/nvme/052 | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/common/nvme b/common/nvme
index c1aa8d6..6371efe 100644
--- a/common/nvme
+++ b/common/nvme
@@ -16,6 +16,7 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
 def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
 export def_subsysnqn="blktests-subsystem-1"
 export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e"
+export def_nsid="1"
 _check_conflict_and_set_default NVMET_TRTYPES nvme_trtype "loop"
 _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
 _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
@@ -537,7 +538,7 @@ _create_nvmet_subsystem() {
 	mkdir -p "${cfs_path}"
 	echo 0 > "${cfs_path}/attr_allow_any_host"
 	_create_nvmet_ns --subsysnqn "${nvmet_subsystem}" \
-			 --nsid "1" \
+			 --nsid "${def_nsid}" \
 			 --blkdev "${blkdev}" \
 			 --uuid "${uuid}" \
 			 ${resv_enable}
diff --git a/tests/nvme/051 b/tests/nvme/051
index 624b42f..4757b80 100755
--- a/tests/nvme/051
+++ b/tests/nvme/051
@@ -33,7 +33,7 @@ test() {
 	_setup_nvmet
 
 	_nvmet_target_setup
-	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/1"
+	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/${def_nsid}"
 
 	# fire off two enable/disable loops concurrently and wait
 	# for them to complete...
diff --git a/tests/nvme/052 b/tests/nvme/052
index 1dcda23..781a9f0 100755
--- a/tests/nvme/052
+++ b/tests/nvme/052
@@ -57,10 +57,10 @@ test() {
 
 	_nvme_connect_subsys
 
-	# start iteration from ns-id 2 because ns-id 1 is created
-	# by default when nvme target is setup. Also ns-id 1 is
+	# start iteration from def_nsid+1 because def_nsid is created
+	# by default when nvme target is setup. Also def_nsid is
 	# deleted when nvme target is cleaned up.
-	for ((i = 2; i <= iterations; i++)); do {
+	for ((i = $((${def_nsid})) + 1; i <= iterations; i++)); do {
 		truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i"
 		uuid="$(uuidgen -r)"
 
-- 
2.43.0

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Chaitanya Kulkarni]

On 10/28/24 20:19, Guixin Liu wrote:
> Use def_nsid instead of hard code, the default of def_nsid is 1.
>
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Daniel Wagner]

On Tue, Oct 29, 2024 at 11:19:31AM GMT, Guixin Liu wrote:
> Use def_nsid instead of hard code, the default of def_nsid is 1.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

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

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Shinichiro Kawasaki]

On Oct 29, 2024 / 11:19, Guixin Liu wrote:
> Use def_nsid instead of hard code, the default of def_nsid is 1.

Hi Guixin, thnaks for the patch.

May I know the motivation to introduce def_nsid? Does it aim to improve code
readability? Or does it aim to allow testing various def_nsid values?

I guessed that the latter one is the motivation, and tried it. After applying
the patch, I edited common/nvme to have differnet value: export def_nsid="2".
With this change, nvme/004 failed. I guess your patch missed a change in
_remove_nvmet_subsystem(). With this change, nvme/004 passed.

@@ -589,7 +589,7 @@ _remove_nvmet_subsystem() {
        local nvmet_subsystem="$1"
        local subsys_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"

-       _remove_nvmet_ns "${nvmet_subsystem}" "1"
+       _remove_nvmet_ns "${nvmet_subsystem}" "$def_nsid"
        rm -f "${subsys_path}"/allowed_hosts/*
        rmdir "${subsys_path}"
 }


> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>  common/nvme    | 3 ++-
>  tests/nvme/051 | 2 +-
>  tests/nvme/052 | 6 +++---
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/common/nvme b/common/nvme
> index c1aa8d6..6371efe 100644
> --- a/common/nvme
> +++ b/common/nvme
> @@ -16,6 +16,7 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
>  def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
>  export def_subsysnqn="blktests-subsystem-1"
>  export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e"
> +export def_nsid="1"
>  _check_conflict_and_set_default NVMET_TRTYPES nvme_trtype "loop"
>  _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
>  _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
> @@ -537,7 +538,7 @@ _create_nvmet_subsystem() {
>  	mkdir -p "${cfs_path}"
>  	echo 0 > "${cfs_path}/attr_allow_any_host"
>  	_create_nvmet_ns --subsysnqn "${nvmet_subsystem}" \
> -			 --nsid "1" \
> +			 --nsid "${def_nsid}" \
>  			 --blkdev "${blkdev}" \
>  			 --uuid "${uuid}" \
>  			 ${resv_enable}
> diff --git a/tests/nvme/051 b/tests/nvme/051
> index 624b42f..4757b80 100755
> --- a/tests/nvme/051
> +++ b/tests/nvme/051
> @@ -33,7 +33,7 @@ test() {
>  	_setup_nvmet
>  
>  	_nvmet_target_setup
> -	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/1"
> +	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/${def_nsid}"
>  
>  	# fire off two enable/disable loops concurrently and wait
>  	# for them to complete...
> diff --git a/tests/nvme/052 b/tests/nvme/052
> index 1dcda23..781a9f0 100755
> --- a/tests/nvme/052
> +++ b/tests/nvme/052
> @@ -57,10 +57,10 @@ test() {
>  
>  	_nvme_connect_subsys
>  
> -	# start iteration from ns-id 2 because ns-id 1 is created
> -	# by default when nvme target is setup. Also ns-id 1 is
> +	# start iteration from def_nsid+1 because def_nsid is created
> +	# by default when nvme target is setup. Also def_nsid is
>  	# deleted when nvme target is cleaned up.
> -	for ((i = 2; i <= iterations; i++)); do {
> +	for ((i = $((${def_nsid})) + 1; i <= iterations; i++)); do {
>  		truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i"
>  		uuid="$(uuidgen -r)"
>

After this change, when def_nsid is larger than iteration=20, nvme/052 will do
nothing. I think the change should be like this:

    for ((i = 2; i <= iterations; i++)); do {
        nsid=$((def_nsid + i - 1))
	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).${nsid}"
        uuid=$(_create_nvmet_ns --blkdev "$(_nvme_def_file_path).${nsid}" \
                                   --nsid "${nsid}")
        ....

Also, same change will be required in nvme/016 and nvme/017. They also use the
--nsid option.

$ git grep -e "--nsid"
common/nvme:                    --nsid)
tests/nvme/016:         _create_nvmet_ns --nsid "${i}" \
tests/nvme/017:         _create_nvmet_ns --nsid "${i}" \
tests/nvme/052:                                 --nsid "${i}")

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Chaitanya Kulkarni]

On 10/30/24 00:31, Shinichiro Kawasaki wrote:
> On Oct 29, 2024 / 11:19, Guixin Liu wrote:
>> Use def_nsid instead of hard code, the default of def_nsid is 1.
> Hi Guixin, thnaks for the patch.
>
> May I know the motivation to introduce def_nsid? Does it aim to improve code
> readability? Or does it aim to allow testing various def_nsid values?
>

I found it useful where we don't have to hard code the values also
it's easier to search for the default namespace id than individually
looking for it when it's require as number of testcases are growing.

Thanks for suggesting fixes on other testcases.

-ck

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Guixin Liu]

在 2024/10/30 15:31, Shinichiro Kawasaki 写道:
> On Oct 29, 2024 / 11:19, Guixin Liu wrote:
>> Use def_nsid instead of hard code, the default of def_nsid is 1.
> Hi Guixin, thnaks for the patch.
>
> May I know the motivation to introduce def_nsid? Does it aim to improve code
> readability? Or does it aim to allow testing various def_nsid values?

Just like def_subsysnqn and def_subsys_uuid, avoid hard code in testcases,

and let us to know the ns_id created by default.

> I guessed that the latter one is the motivation, and tried it. After applying
> the patch, I edited common/nvme to have differnet value: export def_nsid="2".
> With this change, nvme/004 failed. I guess your patch missed a change in
> _remove_nvmet_subsystem(). With this change, nvme/004 passed.
>
> @@ -589,7 +589,7 @@ _remove_nvmet_subsystem() {
>          local nvmet_subsystem="$1"
>          local subsys_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
>
> -       _remove_nvmet_ns "${nvmet_subsystem}" "1"
> +       _remove_nvmet_ns "${nvmet_subsystem}" "$def_nsid"
>          rm -f "${subsys_path}"/allowed_hosts/*
>          rmdir "${subsys_path}"
>   }
>
Thanks, I miss this, will be changed in v2.

Best Regards,

Guixin Liu

>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   common/nvme    | 3 ++-
>>   tests/nvme/051 | 2 +-
>>   tests/nvme/052 | 6 +++---
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/nvme b/common/nvme
>> index c1aa8d6..6371efe 100644
>> --- a/common/nvme
>> +++ b/common/nvme
>> @@ -16,6 +16,7 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
>>   def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
>>   export def_subsysnqn="blktests-subsystem-1"
>>   export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e"
>> +export def_nsid="1"
>>   _check_conflict_and_set_default NVMET_TRTYPES nvme_trtype "loop"
>>   _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
>>   _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
>> @@ -537,7 +538,7 @@ _create_nvmet_subsystem() {
>>   	mkdir -p "${cfs_path}"
>>   	echo 0 > "${cfs_path}/attr_allow_any_host"
>>   	_create_nvmet_ns --subsysnqn "${nvmet_subsystem}" \
>> -			 --nsid "1" \
>> +			 --nsid "${def_nsid}" \
>>   			 --blkdev "${blkdev}" \
>>   			 --uuid "${uuid}" \
>>   			 ${resv_enable}
>> diff --git a/tests/nvme/051 b/tests/nvme/051
>> index 624b42f..4757b80 100755
>> --- a/tests/nvme/051
>> +++ b/tests/nvme/051
>> @@ -33,7 +33,7 @@ test() {
>>   	_setup_nvmet
>>   
>>   	_nvmet_target_setup
>> -	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/1"
>> +	ns="${NVMET_CFS}subsystems/${def_subsysnqn}/namespaces/${def_nsid}"
>>   
>>   	# fire off two enable/disable loops concurrently and wait
>>   	# for them to complete...
>> diff --git a/tests/nvme/052 b/tests/nvme/052
>> index 1dcda23..781a9f0 100755
>> --- a/tests/nvme/052
>> +++ b/tests/nvme/052
>> @@ -57,10 +57,10 @@ test() {
>>   
>>   	_nvme_connect_subsys
>>   
>> -	# start iteration from ns-id 2 because ns-id 1 is created
>> -	# by default when nvme target is setup. Also ns-id 1 is
>> +	# start iteration from def_nsid+1 because def_nsid is created
>> +	# by default when nvme target is setup. Also def_nsid is
>>   	# deleted when nvme target is cleaned up.
>> -	for ((i = 2; i <= iterations; i++)); do {
>> +	for ((i = $((${def_nsid})) + 1; i <= iterations; i++)); do {
>>   		truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i"
>>   		uuid="$(uuidgen -r)"
>>
> After this change, when def_nsid is larger than iteration=20, nvme/052 will do
> nothing. I think the change should be like this:
>
>      for ((i = 2; i <= iterations; i++)); do {
>          nsid=$((def_nsid + i - 1))
> 	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).${nsid}"
>          uuid=$(_create_nvmet_ns --blkdev "$(_nvme_def_file_path).${nsid}" \
>                                     --nsid "${nsid}")
>          ....
>
> Also, same change will be required in nvme/016 and nvme/017. They also use the
> --nsid option.
>
> $ git grep -e "--nsid"
> common/nvme:                    --nsid)
> tests/nvme/016:         _create_nvmet_ns --nsid "${i}" \
> tests/nvme/017:         _create_nvmet_ns --nsid "${i}" \
> tests/nvme/052:                                 --nsid "${i}")

Re: [PATCH] nvme/{common/nvme, 051, 052}: introduce def_nsid

[Shinichiro Kawasaki]

On Oct 30, 2024 / 17:44, Guixin Liu wrote:
> 
> 在 2024/10/30 15:31, Shinichiro Kawasaki 写道:
> > On Oct 29, 2024 / 11:19, Guixin Liu wrote:
> > > Use def_nsid instead of hard code, the default of def_nsid is 1.
> > Hi Guixin, thnaks for the patch.
> > 
> > May I know the motivation to introduce def_nsid? Does it aim to improve code
> > readability? Or does it aim to allow testing various def_nsid values?
> 
> Just like def_subsysnqn and def_subsys_uuid, avoid hard code in testcases,
> 
> and let us to know the ns_id created by default.

Thank you, I see it. I think Chaitanya finds the same value.

I forgot to mention that your v1 patch has this shellcheck waning:

  tests/nvme/052:63:15: note: $/${} is unnecessary on arithmetic variables. [SC2004]

It will be appreciated to address it, and run "make check" for confirmation.