| Message ID | 20220831052729.202997-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] nvme/043,044,045: load dh_generic module | expand |
On 8/30/22 22:27, Shin'ichiro Kawasaki wrote: > Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic > module. When the module is built as a loadable module, the test cases > fail since the module is not loaded at test case runs. > > To avoid the failures, load the dh_generic module at the preparation > step of the test cases. Also unload it at test end for clean up. > > Reported-by: Sagi Grimberg <sagi@grimberg.me> > Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for authenticated connections") > Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") > Fixes: 7640176ef7cc ("nvme/045: test re-authentication") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
On 8/30/22 22:27, Shin'ichiro Kawasaki wrote: > Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic > module. When the module is built as a loadable module, the test cases > fail since the module is not loaded at test case runs. > > To avoid the failures, load the dh_generic module at the preparation > step of the test cases. Also unload it at test end for clean up. > > Reported-by: Sagi Grimberg <sagi@grimberg.me> > Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for authenticated connections") > Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") > Fixes: 7640176ef7cc ("nvme/045: test re-authentication") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ > --- > tests/nvme/043 | 4 ++++ > tests/nvme/044 | 4 ++++ > tests/nvme/045 | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/tests/nvme/043 b/tests/nvme/043 > index 381ae75..dbe9d3f 100755 > --- a/tests/nvme/043 > +++ b/tests/nvme/043 > @@ -40,6 +40,8 @@ test() { > > _setup_nvmet > > + modprobe -q dh_generic > + > truncate -s 512M "${file_path}" > > _create_nvmet_subsystem "${subsys_name}" "${file_path}" it actually makes sense to move above calls into common helper called nvme_auth_test_setup() and similar code for the teardown nvme_auth_teardown() for nvme-auth testcases instead of duplicating the code. -ck
> Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic > module. When the module is built as a loadable module, the test cases > fail since the module is not loaded at test case runs. > > To avoid the failures, load the dh_generic module at the preparation > step of the test cases. Also unload it at test end for clean up. > > Reported-by: Sagi Grimberg <sagi@grimberg.me> > Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for authenticated connections") > Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") > Fixes: 7640176ef7cc ("nvme/045: test re-authentication") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ > --- > tests/nvme/043 | 4 ++++ > tests/nvme/044 | 4 ++++ > tests/nvme/045 | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/tests/nvme/043 b/tests/nvme/043 > index 381ae75..dbe9d3f 100755 > --- a/tests/nvme/043 > +++ b/tests/nvme/043 > @@ -40,6 +40,8 @@ test() { > > _setup_nvmet > > + modprobe -q dh_generic > + > truncate -s 512M "${file_path}" > > _create_nvmet_subsystem "${subsys_name}" "${file_path}" > @@ -88,5 +90,7 @@ test() { > > rm "${file_path}" > > + modprobe -qr dh_generic You should not do this, dh_generic might have been loaded unrelated to this test, you shouldn't just blindly unload it.
On 8/31/22 16:32, Sagi Grimberg wrote: > >> Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic >> module. When the module is built as a loadable module, the test cases >> fail since the module is not loaded at test case runs. >> >> To avoid the failures, load the dh_generic module at the preparation >> step of the test cases. Also unload it at test end for clean up. >> >> Reported-by: Sagi Grimberg <sagi@grimberg.me> >> Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for >> authenticated connections") >> Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") >> Fixes: 7640176ef7cc ("nvme/045: test re-authentication") >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> >> Link: >> https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ >> >> --- >> tests/nvme/043 | 4 ++++ >> tests/nvme/044 | 4 ++++ >> tests/nvme/045 | 4 ++++ >> 3 files changed, 12 insertions(+) >> >> diff --git a/tests/nvme/043 b/tests/nvme/043 >> index 381ae75..dbe9d3f 100755 >> --- a/tests/nvme/043 >> +++ b/tests/nvme/043 >> @@ -40,6 +40,8 @@ test() { >> _setup_nvmet >> + modprobe -q dh_generic >> + >> truncate -s 512M "${file_path}" >> _create_nvmet_subsystem "${subsys_name}" "${file_path}" >> @@ -88,5 +90,7 @@ test() { >> rm "${file_path}" >> + modprobe -qr dh_generic > > You should not do this, dh_generic might have been > loaded unrelated to this test, you shouldn't just > blindly unload it. btw, even failing with a reasonable error message is fine rather than loading dh_generic, the problem is that now it will fail the test in a way that. requires to open the code in order to understand what happened. Perhaps the original commit is better...
On Aug 31, 2022 / 16:37, Sagi Grimberg wrote: > > > On 8/31/22 16:32, Sagi Grimberg wrote: > > > > > Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic > > > module. When the module is built as a loadable module, the test cases > > > fail since the module is not loaded at test case runs. > > > > > > To avoid the failures, load the dh_generic module at the preparation > > > step of the test cases. Also unload it at test end for clean up. > > > > > > Reported-by: Sagi Grimberg <sagi@grimberg.me> > > > Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations > > > for authenticated connections") > > > Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") > > > Fixes: 7640176ef7cc ("nvme/045: test re-authentication") > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ > > > > > > --- > > > tests/nvme/043 | 4 ++++ > > > tests/nvme/044 | 4 ++++ > > > tests/nvme/045 | 4 ++++ > > > 3 files changed, 12 insertions(+) > > > > > > diff --git a/tests/nvme/043 b/tests/nvme/043 > > > index 381ae75..dbe9d3f 100755 > > > --- a/tests/nvme/043 > > > +++ b/tests/nvme/043 > > > @@ -40,6 +40,8 @@ test() { > > > _setup_nvmet > > > + modprobe -q dh_generic > > > + > > > truncate -s 512M "${file_path}" > > > _create_nvmet_subsystem "${subsys_name}" "${file_path}" > > > @@ -88,5 +90,7 @@ test() { > > > rm "${file_path}" > > > + modprobe -qr dh_generic > > > > You should not do this, dh_generic might have been > > loaded unrelated to this test, you shouldn't just > > blindly unload it. > > btw, even failing with a reasonable error message is fine rather > than loading dh_generic, the problem is that now it will fail the test > in a way that. requires to open the code in order to understand what > happened. > > Perhaps the original commit is better... Thanks for the comments. This discussion made me rethink the commit 06a0ba866d90 ("common/rc: avoid module load in _have_driver()"). It avoided module load in _have_driver() to fix an issue, but this approach may not be the best. Fortunately, I've come across another approach for the issue. It will load modules in _have_driver(), but unload them after each test case if the modules were not loaded before the test case start. I'll post another series for this idea. With this approach, your original commit to add _have_driver() calls should work.
On Aug 31, 2022 / 10:45, Chaitanya Kulkarni wrote: > On 8/30/22 22:27, Shin'ichiro Kawasaki wrote: > > Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic > > module. When the module is built as a loadable module, the test cases > > fail since the module is not loaded at test case runs. > > > > To avoid the failures, load the dh_generic module at the preparation > > step of the test cases. Also unload it at test end for clean up. > > > > Reported-by: Sagi Grimberg <sagi@grimberg.me> > > Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for authenticated connections") > > Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") > > Fixes: 7640176ef7cc ("nvme/045: test re-authentication") > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ > > --- > > tests/nvme/043 | 4 ++++ > > tests/nvme/044 | 4 ++++ > > tests/nvme/045 | 4 ++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/tests/nvme/043 b/tests/nvme/043 > > index 381ae75..dbe9d3f 100755 > > --- a/tests/nvme/043 > > +++ b/tests/nvme/043 > > @@ -40,6 +40,8 @@ test() { > > > > _setup_nvmet > > > > + modprobe -q dh_generic > > + > > truncate -s 512M "${file_path}" > > > > _create_nvmet_subsystem "${subsys_name}" "${file_path}" > > it actually makes sense to move above calls into common helper > called nvme_auth_test_setup() and similar code for the teardown > nvme_auth_teardown() for nvme-auth testcases instead of > duplicating the code. Thanks for the review, but I will drop this patch based on other comments by Sagi. Your comment above inspired me to think about better solution :)
diff --git a/tests/nvme/043 b/tests/nvme/043 index 381ae75..dbe9d3f 100755 --- a/tests/nvme/043 +++ b/tests/nvme/043 @@ -40,6 +40,8 @@ test() { _setup_nvmet + modprobe -q dh_generic + truncate -s 512M "${file_path}" _create_nvmet_subsystem "${subsys_name}" "${file_path}" @@ -88,5 +90,7 @@ test() { rm "${file_path}" + modprobe -qr dh_generic + echo "Test complete" } diff --git a/tests/nvme/044 b/tests/nvme/044 index 0465531..5ef6160 100755 --- a/tests/nvme/044 +++ b/tests/nvme/044 @@ -51,6 +51,8 @@ test() { _setup_nvmet + modprobe -q dh_generic + truncate -s 512M "${file_path}" _create_nvmet_subsystem "${subsys_name}" "${file_path}" @@ -118,5 +120,7 @@ test() { rm "${file_path}" + modprobe -qr dh_generic + echo "Test complete" } diff --git a/tests/nvme/045 b/tests/nvme/045 index b60f18f..1d44c55 100755 --- a/tests/nvme/045 +++ b/tests/nvme/045 @@ -53,6 +53,8 @@ test() { _setup_nvmet + modprobe -q dh_generic + truncate -s 512M "${file_path}" _create_nvmet_subsystem "${subsys_name}" "${file_path}" @@ -130,5 +132,7 @@ test() { rm "${file_path}" + modprobe -qr dh_generic + echo "Test complete" }
Test cases nvme/043, 044 and 045 use DH group which relies on dh_generic module. When the module is built as a loadable module, the test cases fail since the module is not loaded at test case runs. To avoid the failures, load the dh_generic module at the preparation step of the test cases. Also unload it at test end for clean up. Reported-by: Sagi Grimberg <sagi@grimberg.me> Fixes: 38d7c5e8400f ("nvme/043: test hash and dh group variations for authenticated connections") Fixes: 63bdf9c16b19 ("nvme/044: test bi-directional authentication") Fixes: 7640176ef7cc ("nvme/045: test re-authentication") Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Link: https://lore.kernel.org/linux-block/a5c3c8e7-4b0a-9930-8f90-e534d2a82bdf@grimberg.me/ --- tests/nvme/043 | 4 ++++ tests/nvme/044 | 4 ++++ tests/nvme/045 | 4 ++++ 3 files changed, 12 insertions(+)