| Message ID | 20230419084757.24846-2-dwagner@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvme_trtype=fc fixes | expand |
On 4/19/23 11:47, Daniel Wagner wrote: > We need to free the resources in the opposite order as we allocate them. > > Signed-off-by: Daniel Wagner <dwagner@suse.de> > --- > tests/nvme/rc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/nvme/rc b/tests/nvme/rc > index b44239446dcf..ec0cc2d8d8cc 100644 > --- a/tests/nvme/rc > +++ b/tests/nvme/rc > @@ -204,10 +204,10 @@ _cleanup_fcloop() { > local remote_wwnn="${3:-$def_remote_wwnn}" > local remote_wwpn="${4:-$def_remote_wwpn}" > > - _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > - "${remote_wwnn}" "${remote_wwpn}" > _nvme_fcloop_del_tport "${remote_wwnn}" "${remote_wwpn}" > _nvme_fcloop_del_lport "${local_wwnn}" "${local_wwpn}" > + _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > + "${remote_wwnn}" "${remote_wwpn}" > } > > _cleanup_nvmet() { Does this fix a bug? if it does, than it should probably be documented that there is a driver bug because userspace teardown ordering should not trigger a driver bug.
On Apr 19, 2023 / 12:41, Sagi Grimberg wrote: > > > On 4/19/23 11:47, Daniel Wagner wrote: > > We need to free the resources in the opposite order as we allocate them. > > > > Signed-off-by: Daniel Wagner <dwagner@suse.de> > > --- > > tests/nvme/rc | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/nvme/rc b/tests/nvme/rc > > index b44239446dcf..ec0cc2d8d8cc 100644 > > --- a/tests/nvme/rc > > +++ b/tests/nvme/rc > > @@ -204,10 +204,10 @@ _cleanup_fcloop() { > > local remote_wwnn="${3:-$def_remote_wwnn}" > > local remote_wwpn="${4:-$def_remote_wwpn}" > > - _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > > - "${remote_wwnn}" "${remote_wwpn}" > > _nvme_fcloop_del_tport "${remote_wwnn}" "${remote_wwpn}" > > _nvme_fcloop_del_lport "${local_wwnn}" "${local_wwpn}" > > + _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > > + "${remote_wwnn}" "${remote_wwpn}" > > } > > _cleanup_nvmet() { > > Does this fix a bug? if it does, than it should probably be documented > that there is a driver bug because userspace teardown ordering should > not trigger a driver bug. I think this fixes a bug, and it can be a left work to add another new test case. Daniel, what do you think?
On Sun, Apr 30, 2023 at 10:07:06AM +0000, Shinichiro Kawasaki wrote: > On Apr 19, 2023 / 12:41, Sagi Grimberg wrote: > > > > > > On 4/19/23 11:47, Daniel Wagner wrote: > > > We need to free the resources in the opposite order as we allocate them. > > > > > > Signed-off-by: Daniel Wagner <dwagner@suse.de> > > > --- > > > tests/nvme/rc | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/nvme/rc b/tests/nvme/rc > > > index b44239446dcf..ec0cc2d8d8cc 100644 > > > --- a/tests/nvme/rc > > > +++ b/tests/nvme/rc > > > @@ -204,10 +204,10 @@ _cleanup_fcloop() { > > > local remote_wwnn="${3:-$def_remote_wwnn}" > > > local remote_wwpn="${4:-$def_remote_wwpn}" > > > - _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > > > - "${remote_wwnn}" "${remote_wwpn}" > > > _nvme_fcloop_del_tport "${remote_wwnn}" "${remote_wwpn}" > > > _nvme_fcloop_del_lport "${local_wwnn}" "${local_wwpn}" > > > + _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ > > > + "${remote_wwnn}" "${remote_wwpn}" > > > } > > > _cleanup_nvmet() { > > > > Does this fix a bug? if it does, than it should probably be documented > > that there is a driver bug because userspace teardown ordering should > > not trigger a driver bug. > > I think this fixes a bug, and it can be a left work to add another new test > case. Daniel, what do you think? Initially I thought this fixes a bug when unloading the fc module. But this change was just really fixing. So stringly speaking I don't think it really workarounds a bug in the fc module unloading. I left the change in the series as I though it makes sense to do the operation in reverse order. So in short it's really just a cosmetic fix for blktests.
diff --git a/tests/nvme/rc b/tests/nvme/rc index b44239446dcf..ec0cc2d8d8cc 100644 --- a/tests/nvme/rc +++ b/tests/nvme/rc @@ -204,10 +204,10 @@ _cleanup_fcloop() { local remote_wwnn="${3:-$def_remote_wwnn}" local remote_wwpn="${4:-$def_remote_wwpn}" - _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ - "${remote_wwnn}" "${remote_wwpn}" _nvme_fcloop_del_tport "${remote_wwnn}" "${remote_wwpn}" _nvme_fcloop_del_lport "${local_wwnn}" "${local_wwpn}" + _nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \ + "${remote_wwnn}" "${remote_wwpn}" } _cleanup_nvmet() {
We need to free the resources in the opposite order as we allocate them. Signed-off-by: Daniel Wagner <dwagner@suse.de> --- tests/nvme/rc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)