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

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

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