[PATCH blktests] nvme/043,044,045: load dh_generic module

[PATCH blktests] nvme/043,044,045: load dh_generic module

[Shin'ichiro Kawasaki]

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
+
 	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"
 }
-- 
2.37.1

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Chaitanya Kulkarni]

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>

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Chaitanya Kulkarni]

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

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Sagi Grimberg]

> 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.

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Sagi Grimberg]

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...

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Shinichiro Kawasaki]

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.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme/043,044,045: load dh_generic module

[Shinichiro Kawasaki]

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 :)

-- 
Shin'ichiro Kawasaki