[PATCH BLKTESTS V2 0/3] nvme: fix output formats

[PATCH BLKTESTS V2 0/3] nvme: fix output formats

[Chaitanya Kulkarni]

Hi Omar,

This is a small patch-series fixes output formats for the the discovery related
testcases. Due to change in the generation counter on the target side
and nvme-cli output we ended up fixing these cases.

-Regards,
Chaitanya

Changes since V1:-
1. Keep the nvme-cli return value check consistent with other blktests.

Chaitanya Kulkarni (3):
  nvme/002: fix output format
  nvme/016: fix output format
  nvme/017: fix output format

 tests/nvme/002.out | 4996 ++++++++++++++++++++++++++--------------------------
 tests/nvme/016     |    4 +-
 tests/nvme/016.out |   11 -
 tests/nvme/017     |    4 +-
 tests/nvme/017.out |   11 -
 5 files changed, 2504 insertions(+), 2522 deletions(-)

-- 
1.8.3.1

[PATCH BLKTESTS V2 2/3] nvme/016: fix output format

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 tests/nvme/016     |  4 +++-
 tests/nvme/016.out | 11 -----------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/tests/nvme/016 b/tests/nvme/016
index 966d5dc..4269ef1 100755
--- a/tests/nvme/016
+++ b/tests/nvme/016
@@ -34,7 +34,9 @@ test() {
 	port="$(_create_nvmet_port "loop")"
 	_add_nvmet_subsys_to_port "$port" "${subsys_nqn}"
 
-	nvme discover -t loop | sed -r -e "s/portid:  [0-9]+/portid:  X/"
+	if ! nvme discover -t loop | grep -q "${subsys_nqn}" ; then
+		echo "nvme discover command failed"
+	fi
 	_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 59bd293..fd244d5 100644
--- a/tests/nvme/016.out
+++ b/tests/nvme/016.out
@@ -1,13 +1,2 @@
 Running nvme/016
-
-Discovery Log Number of Records 1, Generation counter 1
-=====Discovery Log Entry 0======
-trtype:  loop
-adrfam:  pci
-subtype: nvme subsystem
-treq:    not specified
-portid:  X
-trsvcid: 
-subnqn:  blktests-subsystem-1
-traddr:  
 Test complete
-- 
1.8.3.1

[PATCH BLKTESTS V2 3/3] nvme/017: fix output format

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 tests/nvme/017     |  4 +++-
 tests/nvme/017.out | 11 -----------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/tests/nvme/017 b/tests/nvme/017
index 0b86bec..a0d258d 100755
--- a/tests/nvme/017
+++ b/tests/nvme/017
@@ -37,7 +37,9 @@ test() {
 	port="$(_create_nvmet_port "loop")"
 	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
 
-	nvme discover -t loop | sed -r -e "s/portid:  [0-9]+/portid:  X/"
+	if ! nvme discover -t loop | grep -q "${subsys_nqn}" ; then
+		echo "nvme discover command failed"
+	fi
 	_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 4b0877a..6ce9a80 100644
--- a/tests/nvme/017.out
+++ b/tests/nvme/017.out
@@ -1,13 +1,2 @@
 Running nvme/017
-
-Discovery Log Number of Records 1, Generation counter 1
-=====Discovery Log Entry 0======
-trtype:  loop
-adrfam:  pci
-subtype: nvme subsystem
-treq:    not specified
-portid:  X
-trsvcid: 
-subnqn:  blktests-subsystem-1
-traddr:  
 Test complete
-- 
1.8.3.1

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Omar Sandoval]

On Tue, Feb 19, 2019 at 12:23:53PM -0800, Chaitanya Kulkarni wrote:
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  tests/nvme/002.out | 4996 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 2498 insertions(+), 2498 deletions(-)
> 
> diff --git a/tests/nvme/002.out b/tests/nvme/002.out
> index aa71d8f..abecda5 100644
> --- a/tests/nvme/002.out
> +++ b/tests/nvme/002.out
> @@ -1,11 +1,11 @@
>  Running nvme/002
>  
> -Discovery Log Number of Records 1000, Generation counter 1000
> +Discovery Log Number of Records 1000, Generation counter 2000
>  =====Discovery Log Entry 0======
>  trtype:  loop
>  adrfam:  pci
>  subtype: nvme subsystem
> -treq:    not specified
> +treq:    not specified, sq flow control disable supported
>  portid:  X
>  trsvcid: 
>  subnqn:  blktests-subsystem-0

[snip]

Why did this output change? An nvme-cli change, or a kernel change, or
something else? And will this now fail with older versions of nvme-cli
or the kernel instead?

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Chaitanya Kulkarni]

On 02/20/2019 10:04 AM, Omar Sandoval wrote:
> On Tue, Feb 19, 2019 at 12:23:53PM -0800, Chaitanya Kulkarni wrote:
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>   tests/nvme/002.out | 4996 ++++++++++++++++++++++++++--------------------------
>>   1 file changed, 2498 insertions(+), 2498 deletions(-)
>>
>> diff --git a/tests/nvme/002.out b/tests/nvme/002.out
>> index aa71d8f..abecda5 100644
>> --- a/tests/nvme/002.out
>> +++ b/tests/nvme/002.out
>> @@ -1,11 +1,11 @@
>>   Running nvme/002
>>
>> -Discovery Log Number of Records 1000, Generation counter 1000
>> +Discovery Log Number of Records 1000, Generation counter 2000
>>   =====Discovery Log Entry 0======
>>   trtype:  loop
>>   adrfam:  pci
>>   subtype: nvme subsystem
>> -treq:    not specified
>> +treq:    not specified, sq flow control disable supported
>>   portid:  X
>>   trsvcid:
>>   subnqn:  blktests-subsystem-0
>
> [snip]
>
> Why did this output change? An nvme-cli change, or a kernel change, or
> something else? And will this now fail with older versions of nvme-cli
> or the kernel instead?
>

This change is specific to nvme-cli only.

It should fail on older cli versions, we want latest cli/tools to be 
used when running these testcases so that all the latest tools are
also been tested for the compliance, else we might be running tests
with any bugs with older version(s) which are fixed already.

Actually I'd like to add quick nvme-cli version check at the start of 
each test that will certainly help/force user to have a latest version.

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Omar Sandoval]

On Wed, Feb 20, 2019 at 06:28:50PM +0000, Chaitanya Kulkarni wrote:
> On 02/20/2019 10:04 AM, Omar Sandoval wrote:
> > On Tue, Feb 19, 2019 at 12:23:53PM -0800, Chaitanya Kulkarni wrote:
> >> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> >> ---
> >>   tests/nvme/002.out | 4996 ++++++++++++++++++++++++++--------------------------
> >>   1 file changed, 2498 insertions(+), 2498 deletions(-)
> >>
> >> diff --git a/tests/nvme/002.out b/tests/nvme/002.out
> >> index aa71d8f..abecda5 100644
> >> --- a/tests/nvme/002.out
> >> +++ b/tests/nvme/002.out
> >> @@ -1,11 +1,11 @@
> >>   Running nvme/002
> >>
> >> -Discovery Log Number of Records 1000, Generation counter 1000
> >> +Discovery Log Number of Records 1000, Generation counter 2000
> >>   =====Discovery Log Entry 0======
> >>   trtype:  loop
> >>   adrfam:  pci
> >>   subtype: nvme subsystem
> >> -treq:    not specified
> >> +treq:    not specified, sq flow control disable supported
> >>   portid:  X
> >>   trsvcid:
> >>   subnqn:  blktests-subsystem-0
> >
> > [snip]
> >
> > Why did this output change? An nvme-cli change, or a kernel change, or
> > something else? And will this now fail with older versions of nvme-cli
> > or the kernel instead?
> >
> 
> This change is specific to nvme-cli only.
> 
> It should fail on older cli versions, we want latest cli/tools to be 
> used when running these testcases so that all the latest tools are
> also been tested for the compliance, else we might be running tests
> with any bugs with older version(s) which are fixed already.
> 
> Actually I'd like to add quick nvme-cli version check at the start of 
> each test that will certainly help/force user to have a latest version.

Perfect, I was going to suggest adding a check for the minimum version
needed for each test.

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Theodore Y. Ts'o]

On Wed, Feb 20, 2019 at 11:35:00AM -0800, Omar Sandoval wrote:
> > It should fail on older cli versions, we want latest cli/tools to be 
> > used when running these testcases so that all the latest tools are
> > also been tested for the compliance, else we might be running tests
> > with any bugs with older version(s) which are fixed already.
> > 
> > Actually I'd like to add quick nvme-cli version check at the start of 
> > each test that will certainly help/force user to have a latest version.
> 
> Perfect, I was going to suggest adding a check for the minimum version
> needed for each test.

It would be great if we could document what is (a) the minimum version
for things to work at all, and (b) the ideal version to use so that
tests aren't being skipped due to version constraints and so we are
testing the kernel as much as possible.

Ideally, tests should either be designed primarily to test userspace,
or to test the kernel, so that people who want to use blktests aren't
having to build the latest versions of userspace.

(As an aside, Bart has told me that the failures I'm seeing with
srp/002 and srp/011 are due to the fact that I'm using a version of
multipathd which is too old.  Apparently both the version of
multipathd in Debian stable *and* Debian unstable are still too old,
since they are still failing.  Next stop, building multipathd from
the git repo....   Grrr......)

						- Ted

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Chaitanya Kulkarni]

On 02/21/2019 07:04 AM, Theodore Y. Ts'o wrote:
> On Wed, Feb 20, 2019 at 11:35:00AM -0800, Omar Sandoval wrote:
>>> It should fail on older cli versions, we want latest cli/tools to be
>>> used when running these testcases so that all the latest tools are
>>> also been tested for the compliance, else we might be running tests
>>> with any bugs with older version(s) which are fixed already.
>>>
>>> Actually I'd like to add quick nvme-cli version check at the start of
>>> each test that will certainly help/force user to have a latest version.
>>
>> Perfect, I was going to suggest adding a check for the minimum version
>> needed for each test.
>
> It would be great if we could document what is (a) the minimum version
> for things to work at all,
Actually the latest version or the HEAD of the git repo should work all
the time if it is breaking test or the tool or driver we need to add
a fix respectively.

  and (b) the ideal version to use so that
> tests aren't being skipped due to version constraints and so we are
> testing the kernel as much as possible.
>
Agree.
> Ideally, tests should either be designed primarily to test userspace,
> or to test the kernel, so that people who want to use blktests aren't
> having to build the latest versions of userspace.
>
At least with the current NVMe infrastructure we need latest tools
(git HEAD) to be compliant all the time with the tests, since it
guarantees the tools, driver and kernel compatibility.

We can a little install script to build and install the
cli if desired version is not present.

> (As an aside, Bart has told me that the failures I'm seeing with
> srp/002 and srp/011 are due to the fact that I'm using a version of
> multipathd which is too old.  Apparently both the version of
> multipathd in Debian stable *and* Debian unstable are still too old,
> since they are still failing.  Next stop, building multipathd from
> the git repo....   Grrr......)
This is exact reason why tools needs to be built and check with latest
HEAD.
>
> 						- Ted
>

Re: [PATCH BLKTESTS V2 1/3] nvme/002: fix output format

[Omar Sandoval]

On Fri, Feb 22, 2019 at 12:00:43AM +0000, Chaitanya Kulkarni wrote:
> On 02/21/2019 07:04 AM, Theodore Y. Ts'o wrote:
> > On Wed, Feb 20, 2019 at 11:35:00AM -0800, Omar Sandoval wrote:
> >>> It should fail on older cli versions, we want latest cli/tools to be
> >>> used when running these testcases so that all the latest tools are
> >>> also been tested for the compliance, else we might be running tests
> >>> with any bugs with older version(s) which are fixed already.
> >>>
> >>> Actually I'd like to add quick nvme-cli version check at the start of
> >>> each test that will certainly help/force user to have a latest version.
> >>
> >> Perfect, I was going to suggest adding a check for the minimum version
> >> needed for each test.
> >
> > It would be great if we could document what is (a) the minimum version
> > for things to work at all,
> Actually the latest version or the HEAD of the git repo should work all
> the time if it is breaking test or the tool or driver we need to add
> a fix respectively.
> 
>   and (b) the ideal version to use so that
> > tests aren't being skipped due to version constraints and so we are
> > testing the kernel as much as possible.
> >
> Agree.
> > Ideally, tests should either be designed primarily to test userspace,
> > or to test the kernel, so that people who want to use blktests aren't
> > having to build the latest versions of userspace.
> >
> At least with the current NVMe infrastructure we need latest tools
> (git HEAD) to be compliant all the time with the tests, since it
> guarantees the tools, driver and kernel compatibility.
> 
> We can a little install script to build and install the
> cli if desired version is not present.
> 
> > (As an aside, Bart has told me that the failures I'm seeing with
> > srp/002 and srp/011 are due to the fact that I'm using a version of
> > multipathd which is too old.  Apparently both the version of
> > multipathd in Debian stable *and* Debian unstable are still too old,
> > since they are still failing.  Next stop, building multipathd from
> > the git repo....   Grrr......)
> This is exact reason why tools needs to be built and check with latest
> HEAD.

I don't think we should be _requiring_ that the tests are run with the
latest tools from git. That just adds another annoying step to someone
trying to get the tests up and running. Like I said, each test should
check for the minimum version it needs, and we should avoid making the
golden output too dependent on version.

However, I do think it's a reasonable expectation that if you want
maximum test coverage, you should build the tools from git.