| Message ID | 1685495221-4598-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] nvme/{016,017}: use _check_genctr instead of _filter_discovery | expand |
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 >
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
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.
> 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...
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
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.
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
>>> 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.
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?
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
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.
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
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. 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> --- 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(-)