Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Shinichiro Kawasaki]

CC+: linux-nvme

On May 31, 2023 / 09:07, Yang Xu wrote:
> Since commit 328943e3 ("Update tests for discovery log page changes"),
> blktests also include the discovery subsystem itself. But it
> will lead these cases fails on older nvme-cli system.

Thanks for this report. What is the nvme-cli version with the issue?

> 
> To avoid this, like nvme/002, use _check_genctr to check instead of
> comparing many discovery Log Entry output.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>

The change looks fine to me, but I'd wait for comments by nvme developers.

> ---
>  tests/nvme/016     | 4 +++-
>  tests/nvme/016.out | 7 -------
>  tests/nvme/017     | 5 ++++-
>  tests/nvme/017.out | 7 -------
>  4 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/nvme/016 b/tests/nvme/016
> index c0c31a5..f617cf1 100755
> --- a/tests/nvme/016
> +++ b/tests/nvme/016
> @@ -24,6 +24,7 @@ test() {
>  	_setup_nvmet
>  
>  	loop_dev="$(losetup -f)"
> +	local genctr=1
>  
>  	_create_nvmet_subsystem "${subsys_nqn}" "${loop_dev}"
>  
> @@ -34,7 +35,8 @@ test() {
>  	port="$(_create_nvmet_port "${nvme_trtype}")"
>  	_add_nvmet_subsys_to_port "$port" "${subsys_nqn}"
>  
> -	_nvme_discover loop | _filter_discovery
> +	genctr=$(_check_genctr "${genctr}" "adding a subsystem to a port")
> +
>  	_remove_nvmet_subsystem_from_port "${port}" "${subsys_nqn}"
>  	_remove_nvmet_port "${port}"
>  
> diff --git a/tests/nvme/016.out b/tests/nvme/016.out
> index ee631a4..fd244d5 100644
> --- a/tests/nvme/016.out
> +++ b/tests/nvme/016.out
> @@ -1,9 +1,2 @@
>  Running nvme/016
> -Discovery Log Number of Records 2, Generation counter X
> -=====Discovery Log Entry 0======
> -trtype:  loop
> -subnqn:  nqn.2014-08.org.nvmexpress.discovery
> -=====Discovery Log Entry 1======
> -trtype:  loop
> -subnqn:  blktests-subsystem-1
>  Test complete
> diff --git a/tests/nvme/017 b/tests/nvme/017
> index e167450..3dbb7c1 100755
> --- a/tests/nvme/017
> +++ b/tests/nvme/017
> @@ -27,6 +27,8 @@ test() {
>  
>  	truncate -s "${nvme_img_size}" "${file_path}"
>  
> +	local genctr=1
> +
>  	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
>  		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
>  
> @@ -37,7 +39,8 @@ test() {
>  	port="$(_create_nvmet_port "${nvme_trtype}")"
>  	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
>  
> -	_nvme_discover loop | _filter_discovery
> +	genctr=$(_check_genctr "${genctr}" "adding a subsystem to a port")
> +
>  	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
>  	_remove_nvmet_port "${port}"
>  
> diff --git a/tests/nvme/017.out b/tests/nvme/017.out
> index 12787f7..6ce9a80 100644
> --- a/tests/nvme/017.out
> +++ b/tests/nvme/017.out
> @@ -1,9 +1,2 @@
>  Running nvme/017
> -Discovery Log Number of Records 2, Generation counter X
> -=====Discovery Log Entry 0======
> -trtype:  loop
> -subnqn:  nqn.2014-08.org.nvmexpress.discovery
> -=====Discovery Log Entry 1======
> -trtype:  loop
> -subnqn:  blktests-subsystem-1
>  Test complete
> -- 
> 2.39.1
> 

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Yang Xu (Fujitsu)]

on 2023/05/31 13:36, Shinichiro Kawasaki wrote:
> CC+: linux-nvme
> 
> On May 31, 2023 / 09:07, Yang Xu wrote:
>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>> blktests also include the discovery subsystem itself. But it
>> will lead these cases fails on older nvme-cli system.
> 
> Thanks for this report. What is the nvme-cli version with the issue?

I used nvme-cli-1.16-7.el8.x86_64.


Best Regards
Yang Xu
> 
>>
>> To avoid this, like nvme/002, use _check_genctr to check instead of
>> comparing many discovery Log Entry output.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> 
> The change looks fine to me, but I'd wait for comments by nvme developers.
> 
>> ---
>>   tests/nvme/016     | 4 +++-
>>   tests/nvme/016.out | 7 -------
>>   tests/nvme/017     | 5 ++++-
>>   tests/nvme/017.out | 7 -------
>>   4 files changed, 7 insertions(+), 16 deletions(-)
>>
>> diff --git a/tests/nvme/016 b/tests/nvme/016
>> index c0c31a5..f617cf1 100755
>> --- a/tests/nvme/016
>> +++ b/tests/nvme/016
>> @@ -24,6 +24,7 @@ test() {
>>   	_setup_nvmet
>>   
>>   	loop_dev="$(losetup -f)"
>> +	local genctr=1
>>   
>>   	_create_nvmet_subsystem "${subsys_nqn}" "${loop_dev}"
>>   
>> @@ -34,7 +35,8 @@ test() {
>>   	port="$(_create_nvmet_port "${nvme_trtype}")"
>>   	_add_nvmet_subsys_to_port "$port" "${subsys_nqn}"
>>   
>> -	_nvme_discover loop | _filter_discovery
>> +	genctr=$(_check_genctr "${genctr}" "adding a subsystem to a port")
>> +
>>   	_remove_nvmet_subsystem_from_port "${port}" "${subsys_nqn}"
>>   	_remove_nvmet_port "${port}"
>>   
>> diff --git a/tests/nvme/016.out b/tests/nvme/016.out
>> index ee631a4..fd244d5 100644
>> --- a/tests/nvme/016.out
>> +++ b/tests/nvme/016.out
>> @@ -1,9 +1,2 @@
>>   Running nvme/016
>> -Discovery Log Number of Records 2, Generation counter X
>> -=====Discovery Log Entry 0======
>> -trtype:  loop
>> -subnqn:  nqn.2014-08.org.nvmexpress.discovery
>> -=====Discovery Log Entry 1======
>> -trtype:  loop
>> -subnqn:  blktests-subsystem-1
>>   Test complete
>> diff --git a/tests/nvme/017 b/tests/nvme/017
>> index e167450..3dbb7c1 100755
>> --- a/tests/nvme/017
>> +++ b/tests/nvme/017
>> @@ -27,6 +27,8 @@ test() {
>>   
>>   	truncate -s "${nvme_img_size}" "${file_path}"
>>   
>> +	local genctr=1
>> +
>>   	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
>>   		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
>>   
>> @@ -37,7 +39,8 @@ test() {
>>   	port="$(_create_nvmet_port "${nvme_trtype}")"
>>   	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
>>   
>> -	_nvme_discover loop | _filter_discovery
>> +	genctr=$(_check_genctr "${genctr}" "adding a subsystem to a port")
>> +
>>   	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
>>   	_remove_nvmet_port "${port}"
>>   
>> diff --git a/tests/nvme/017.out b/tests/nvme/017.out
>> index 12787f7..6ce9a80 100644
>> --- a/tests/nvme/017.out
>> +++ b/tests/nvme/017.out
>> @@ -1,9 +1,2 @@
>>   Running nvme/017
>> -Discovery Log Number of Records 2, Generation counter X
>> -=====Discovery Log Entry 0======
>> -trtype:  loop
>> -subnqn:  nqn.2014-08.org.nvmexpress.discovery
>> -=====Discovery Log Entry 1======
>> -trtype:  loop
>> -subnqn:  blktests-subsystem-1
>>   Test complete
>> -- 
>> 2.39.1

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Shinichiro Kawasaki]

On May 31, 2023 / 05:40, Yang Xu (Fujitsu) wrote:
> 
> 
> on 2023/05/31 13:36, Shinichiro Kawasaki wrote:
> > CC+: linux-nvme
> > 
> > On May 31, 2023 / 09:07, Yang Xu wrote:
> >> Since commit 328943e3 ("Update tests for discovery log page changes"),
> >> blktests also include the discovery subsystem itself. But it
> >> will lead these cases fails on older nvme-cli system.
> > 
> > Thanks for this report. What is the nvme-cli version with the issue?
> 
> I used nvme-cli-1.16-7.el8.x86_64.

Thanks. I compiled nvme-cli version 1.16 on my test system, but it did not
recreate the failure of nvme/016 or nvme/017. I ran them on kernel v6.4-rc4 with
Fedora 37, so the failure may depend on not only nvme-cli version, but also
kernel version.

Anyway, let's wait for comments by nvme experts.

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Sagi Grimberg]

> On May 31, 2023 / 09:07, Yang Xu wrote:
>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>> blktests also include the discovery subsystem itself. But it
>> will lead these cases fails on older nvme-cli system.
> 
> Thanks for this report. What is the nvme-cli version with the issue?
> 
>>
>> To avoid this, like nvme/002, use _check_genctr to check instead of
>> comparing many discovery Log Entry output.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> 
> The change looks fine to me, but I'd wait for comments by nvme developers.

I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
the full log-page output...

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Chaitanya Kulkarni]

On 6/5/2023 4:18 PM, Sagi Grimberg wrote:
> 
>> On May 31, 2023 / 09:07, Yang Xu wrote:
>>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>>> blktests also include the discovery subsystem itself. But it
>>> will lead these cases fails on older nvme-cli system.
>>
>> Thanks for this report. What is the nvme-cli version with the issue?
>>
>>>
>>> To avoid this, like nvme/002, use _check_genctr to check instead of
>>> comparing many discovery Log Entry output.
>>>
>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>
>> The change looks fine to me, but I'd wait for comments by nvme 
>> developers.
> 
> I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
> the full log-page output...

the original testcase was designed to validate the log page internals
and  that correctness cannot be established without looking into the log
page.

but given that how much churn this is generating eveytime something 
changes in nvme-cli or in kernel implementation I'm really wondering is
that worth everyone's time ?

Sagi/Shinichiro any thoughts ?

-ck

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Shinichiro Kawasaki]

On Jun 06, 2023 / 03:20, Chaitanya Kulkarni wrote:
> On 6/5/2023 4:18 PM, Sagi Grimberg wrote:
> > 
> >> On May 31, 2023 / 09:07, Yang Xu wrote:
> >>> Since commit 328943e3 ("Update tests for discovery log page changes"),
> >>> blktests also include the discovery subsystem itself. But it
> >>> will lead these cases fails on older nvme-cli system.
> >>
> >> Thanks for this report. What is the nvme-cli version with the issue?
> >>
> >>>
> >>> To avoid this, like nvme/002, use _check_genctr to check instead of
> >>> comparing many discovery Log Entry output.
> >>>
> >>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> >>
> >> The change looks fine to me, but I'd wait for comments by nvme 
> >> developers.
> > 
> > I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
> > the full log-page output...
> 
> the original testcase was designed to validate the log page internals
> and  that correctness cannot be established without looking into the log
> page.
> 
> but given that how much churn this is generating eveytime something 
> changes in nvme-cli or in kernel implementation I'm really wondering is
> that worth everyone's time ?
> 
> Sagi/Shinichiro any thoughts ?

I don't have future view about the stability of the log page. I would like to
hear call by Sagi and/or nvme developers about it. If we expect more log page
changes, Yang's change in blktests sounds reasonable.

If we expect no more log page changes in the future, we can think of another
solution: skip the test cases on older kernel (or nvme-cli). I think the
blktests commit 328943e3 ("Update tests for discovery log page changes")
corresponds to the kernel commit e5ea42faa773 ("nvme: display correct subsystem
NQN"), applied in the kernel version 5.16. So "_have_kver 5 16" for the test
cases will avoid the failure Yang observes.

Yang, may I confirm the kernel version you use? If you use RHEL 8 based OS, I
think it is v4.18.

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Yang Xu (Fujitsu)]

on 2023/06/06 13:14, Shinichiro Kawasaki wrote:
> On Jun 06, 2023 / 03:20, Chaitanya Kulkarni wrote:
>> On 6/5/2023 4:18 PM, Sagi Grimberg wrote:
>>>
>>>> On May 31, 2023 / 09:07, Yang Xu wrote:
>>>>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>>>>> blktests also include the discovery subsystem itself. But it
>>>>> will lead these cases fails on older nvme-cli system.
>>>>
>>>> Thanks for this report. What is the nvme-cli version with the issue?
>>>>
>>>>>
>>>>> To avoid this, like nvme/002, use _check_genctr to check instead of
>>>>> comparing many discovery Log Entry output.
>>>>>
>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>>>
>>>> The change looks fine to me, but I'd wait for comments by nvme
>>>> developers.
>>>
>>> I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
>>> the full log-page output...
>>
>> the original testcase was designed to validate the log page internals
>> and  that correctness cannot be established without looking into the log
>> page.
>>
>> but given that how much churn this is generating eveytime something
>> changes in nvme-cli or in kernel implementation I'm really wondering is
>> that worth everyone's time ?
>>
>> Sagi/Shinichiro any thoughts ?
> 
> I don't have future view about the stability of the log page. I would like to
> hear call by Sagi and/or nvme developers about it. If we expect more log page
> changes, Yang's change in blktests sounds reasonable.
> 
> If we expect no more log page changes in the future, we can think of another
> solution: skip the test cases on older kernel (or nvme-cli). I think the
> blktests commit 328943e3 ("Update tests for discovery log page changes")
> corresponds to the kernel commit e5ea42faa773 ("nvme: display correct subsystem
> NQN"), applied in the kernel version 5.16. So "_have_kver 5 16" for the test
> cases will avoid the failure Yang observes.
> 
> Yang, may I confirm the kernel version you use? If you use RHEL 8 based OS, I
> think it is v4.18.

Yes, I used RHEL8 and don't introduce this kernel patch.

Best Regards
Yang Xu

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Sagi Grimberg]

>>> On May 31, 2023 / 09:07, Yang Xu wrote:
>>>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>>>> blktests also include the discovery subsystem itself. But it
>>>> will lead these cases fails on older nvme-cli system.
>>>
>>> Thanks for this report. What is the nvme-cli version with the issue?
>>>
>>>>
>>>> To avoid this, like nvme/002, use _check_genctr to check instead of
>>>> comparing many discovery Log Entry output.
>>>>
>>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>>
>>> The change looks fine to me, but I'd wait for comments by nvme
>>> developers.
>>
>> I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
>> the full log-page output...
> 
> the original testcase was designed to validate the log page internals
> and  that correctness cannot be established without looking into the log
> page.
> 
> but given that how much churn this is generating eveytime something
> changes in nvme-cli or in kernel implementation I'm really wondering is
> that worth everyone's time ?
> 
> Sagi/Shinichiro any thoughts ?

Also back then I thought it'd create churn because the log page output
is not an interface.

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Shinichiro Kawasaki]

On Jun 06, 2023 / 09:45, Sagi Grimberg wrote:
> 
> > > > On May 31, 2023 / 09:07, Yang Xu wrote:
> > > > > Since commit 328943e3 ("Update tests for discovery log page changes"),
> > > > > blktests also include the discovery subsystem itself. But it
> > > > > will lead these cases fails on older nvme-cli system.
> > > > 
> > > > Thanks for this report. What is the nvme-cli version with the issue?
> > > > 
> > > > > 
> > > > > To avoid this, like nvme/002, use _check_genctr to check instead of
> > > > > comparing many discovery Log Entry output.
> > > > > 
> > > > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> > > > 
> > > > The change looks fine to me, but I'd wait for comments by nvme
> > > > developers.
> > > 
> > > I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
> > > the full log-page output...
> > 
> > the original testcase was designed to validate the log page internals
> > and  that correctness cannot be established without looking into the log
> > page.

I think nvme/016 and 017 still have value even without the log-page output
checks. They exercise namespace creations and deletions which other test
cases don't.

> > 
> > but given that how much churn this is generating eveytime something
> > changes in nvme-cli or in kernel implementation I'm really wondering is
> > that worth everyone's time ?
> > 
> > Sagi/Shinichiro any thoughts ?
> 
> Also back then I thought it'd create churn because the log page output
> is not an interface.

So, we should drop the log page output check, and it means that Yang's patch is
the choice.

Chaitanya, is it ok for you?

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Chaitanya Kulkarni]

On 6/6/2023 7:14 PM, Shinichiro Kawasaki wrote:
> On Jun 06, 2023 / 09:45, Sagi Grimberg wrote:
>>
>>>>> On May 31, 2023 / 09:07, Yang Xu wrote:
>>>>>> Since commit 328943e3 ("Update tests for discovery log page changes"),
>>>>>> blktests also include the discovery subsystem itself. But it
>>>>>> will lead these cases fails on older nvme-cli system.
>>>>>
>>>>> Thanks for this report. What is the nvme-cli version with the issue?
>>>>>
>>>>>>
>>>>>> To avoid this, like nvme/002, use _check_genctr to check instead of
>>>>>> comparing many discovery Log Entry output.
>>>>>>
>>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>>>>
>>>>> The change looks fine to me, but I'd wait for comments by nvme
>>>>> developers.
>>>>
>>>> I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
>>>> the full log-page output...
>>>
>>> the original testcase was designed to validate the log page internals
>>> and  that correctness cannot be established without looking into the log
>>> page.
> 
> I think nvme/016 and 017 still have value even without the log-page output
> checks. They exercise namespace creations and deletions which other test
> cases don't.
> 
>>>
>>> but given that how much churn this is generating eveytime something
>>> changes in nvme-cli or in kernel implementation I'm really wondering is
>>> that worth everyone's time ?
>>>
>>> Sagi/Shinichiro any thoughts ?
>>
>> Also back then I thought it'd create churn because the log page output
>> is not an interface.
> 
> So, we should drop the log page output check, and it means that Yang's patch is
> the choice.
> 
> Chaitanya, is it ok for you?

I think it is reasonable to drop this check, also it will be great if
can we can drop any other such checks in the nvme test category to save
everyone's time.

-ck

Re: [PATCH blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery

[Shinichiro Kawasaki]

On Jun 07, 2023 / 03:35, Chaitanya Kulkarni wrote:
> On 6/6/2023 7:14 PM, Shinichiro Kawasaki wrote:
> > On Jun 06, 2023 / 09:45, Sagi Grimberg wrote:
> >>
> >>>>> On May 31, 2023 / 09:07, Yang Xu wrote:
> >>>>>> Since commit 328943e3 ("Update tests for discovery log page changes"),
> >>>>>> blktests also include the discovery subsystem itself. But it
> >>>>>> will lead these cases fails on older nvme-cli system.
> >>>>>
> >>>>> Thanks for this report. What is the nvme-cli version with the issue?
> >>>>>
> >>>>>>
> >>>>>> To avoid this, like nvme/002, use _check_genctr to check instead of
> >>>>>> comparing many discovery Log Entry output.
> >>>>>>
> >>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> >>>>>
> >>>>> The change looks fine to me, but I'd wait for comments by nvme
> >>>>> developers.
> >>>>
> >>>> I'm ok with this change, but IIRC Chaitanya wanted that we keep checking
> >>>> the full log-page output...
> >>>
> >>> the original testcase was designed to validate the log page internals
> >>> and  that correctness cannot be established without looking into the log
> >>> page.
> > 
> > I think nvme/016 and 017 still have value even without the log-page output
> > checks. They exercise namespace creations and deletions which other test
> > cases don't.
> > 
> >>>
> >>> but given that how much churn this is generating eveytime something
> >>> changes in nvme-cli or in kernel implementation I'm really wondering is
> >>> that worth everyone's time ?
> >>>
> >>> Sagi/Shinichiro any thoughts ?
> >>
> >> Also back then I thought it'd create churn because the log page output
> >> is not an interface.
> > 
> > So, we should drop the log page output check, and it means that Yang's patch is
> > the choice.
> > 
> > Chaitanya, is it ok for you?
> 
> I think it is reasonable to drop this check,

Okay, I have applied Yang's patch (Thanks Yang!). And I also added a follow-up
commit to remove the helper fucntion _filter_discovery, which is no longer used.

> also it will be great if
> can we can drop any other such checks in the nvme test category to save
> everyone's time.

Another commit 93ae757d4ca2("nvme/{002,030}: Move discovery generation counter
code to rc") removed the check from nvme/002. So, there is no check for
Discovery Log Page outputs in the nvme group at this moment. As far as I see,
there seems no other Log Page checks.