With the attempt to get the fc transport running for blktest I found a couple of
issues in the current cleanup code path for fc. With these fixes and the kernel
patches and configuration from
https://lore.kernel.org/linux-nvme/[email protected]/
it starts to work nicely, incl finding some bugs in fc I think.
Daniel Wagner (2):
nvme-rc: Cleanup fc ports in reverse order
nvme-rc: Cleanup fc resource before module unloading
tests/nvme/rc | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
--
2.40.0
We need to free the resources in the opposite order as we allocate them.
Signed-off-by: Daniel Wagner <[email protected]>
---
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() {
--
2.40.0
Before we unload the module we should cleanup the fc resources first,
basically reorder the shutdown sequence to be in reverse order of the
setup path.
Also unload the nvme-fcloop after usage.
While at it also update the rdma stop_soft_rdma before the module
unloading for the same reasoning.
Signed-off-by: Daniel Wagner <[email protected]>
---
tests/nvme/rc | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/tests/nvme/rc b/tests/nvme/rc
index ec0cc2d8d8cc..41f196b037d6 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -260,18 +260,20 @@ _cleanup_nvmet() {
shopt -u nullglob
trap SIGINT
- modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
- if [[ "${nvme_trtype}" != "loop" ]]; then
- modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
- fi
- modprobe -rq nvmet 2>/dev/null
if [[ "${nvme_trtype}" == "rdma" ]]; then
stop_soft_rdma
fi
if [[ "${nvme_trtype}" == "fc" ]]; then
_cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
"${def_remote_wwnn}" "${def_remote_wwpn}"
+ modprobe -rq nvme-fcloop
fi
+
+ modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
+ if [[ "${nvme_trtype}" != "loop" ]]; then
+ modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
+ fi
+ modprobe -rq nvmet 2>/dev/null
}
_setup_nvmet() {
--
2.40.0
> Before we unload the module we should cleanup the fc resources first,
> basically reorder the shutdown sequence to be in reverse order of the
> setup path.
If this triggers a bug, then I think it is a good idea to have a
dedicated test that reproduces it if we are changing the default
behavior.
>
> Also unload the nvme-fcloop after usage.
>
> While at it also update the rdma stop_soft_rdma before the module
> unloading for the same reasoning.
Why? it creates the wrong reverse ordering.
1. setup soft-rdma
2. setup nvme-rdma
2. teardown nvme-rdma
1. teardown soft-rdma
I don't think we need this change. I mean it is a good test
to have that the rdma device goes away underneath nvme-rdma
but it is good for a dedicated test.
>
> Signed-off-by: Daniel Wagner <[email protected]>
> ---
> tests/nvme/rc | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index ec0cc2d8d8cc..41f196b037d6 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -260,18 +260,20 @@ _cleanup_nvmet() {
> shopt -u nullglob
> trap SIGINT
>
> - modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
> - if [[ "${nvme_trtype}" != "loop" ]]; then
> - modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
> - fi
> - modprobe -rq nvmet 2>/dev/null
> if [[ "${nvme_trtype}" == "rdma" ]]; then
> stop_soft_rdma
> fi
> if [[ "${nvme_trtype}" == "fc" ]]; then
> _cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
> "${def_remote_wwnn}" "${def_remote_wwpn}"
> + modprobe -rq nvme-fcloop
> fi
> +
> + modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
> + if [[ "${nvme_trtype}" != "loop" ]]; then
> + modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
> + fi
> + modprobe -rq nvmet 2>/dev/null
> }
>
> _setup_nvmet() {
On 4/19/23 01:47, Daniel Wagner wrote:
> Before we unload the module we should cleanup the fc resources first,
> basically reorder the shutdown sequence to be in reverse order of the
> setup path.
>
> Also unload the nvme-fcloop after usage.
>
> While at it also update the rdma stop_soft_rdma before the module
> unloading for the same reasoning.
>
> Signed-off-by: Daniel Wagner <[email protected]>
> ---
> tests/nvme/rc | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index ec0cc2d8d8cc..41f196b037d6 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -260,18 +260,20 @@ _cleanup_nvmet() {
> shopt -u nullglob
> trap SIGINT
>
> - modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
> - if [[ "${nvme_trtype}" != "loop" ]]; then
> - modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
> - fi
> - modprobe -rq nvmet 2>/dev/null
> if [[ "${nvme_trtype}" == "rdma" ]]; then
> stop_soft_rdma
> fi
> if [[ "${nvme_trtype}" == "fc" ]]; then
> _cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
> "${def_remote_wwnn}" "${def_remote_wwpn}"
> + modprobe -rq nvme-fcloop
> fi
> +
> + modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
> + if [[ "${nvme_trtype}" != "loop" ]]; then
> + modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
> + fi
> + modprobe -rq nvmet 2>/dev/null
> }
>
> _setup_nvmet() {
were you able to test this with RDMA ?
just want to make sure we are not breaking anything since we are changing
the order of module unload and stop_soft_rdma() in this patch ...
-ck
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 <[email protected]>
> ---
> 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 Wed, Apr 19, 2023 at 12:44:42PM +0300, Sagi Grimberg wrote:
>
> > Before we unload the module we should cleanup the fc resources first,
> > basically reorder the shutdown sequence to be in reverse order of the
> > setup path.
>
> If this triggers a bug, then I think it is a good idea to have a
> dedicated test that reproduces it if we are changing the default
> behavior.
Right, though I would like to tackle one problem after the other, first get fc
working with the 'correct' order.
> > While at it also update the rdma stop_soft_rdma before the module
> > unloading for the same reasoning.
>
> Why? it creates the wrong reverse ordering.
>
> 1. setup soft-rdma
> 2. setup nvme-rdma
>
> 2. teardown nvme-rdma
> 1. teardown soft-rdma
>
> I don't think we need this change. I mean it is a good test
> to have that the rdma device goes away underneath nvme-rdma
> but it is good for a dedicated test.
I was woried about this setup sequence here:
modprobe -q nvme-"${nvme_trtype}"
if [[ "${nvme_trtype}" == "rdma" ]]; then
start_soft_rdma
The module is loaded before start_soft_rdma is started, thus I thought we should
do the reverse, first call stop_soft_rdma and the unload the module.
On Wed, Apr 19, 2023 at 09:41:28AM +0000, Chaitanya Kulkarni wrote:
> were you able to test this with RDMA ?
Yes, I've tested it with all transports (loop, tcp, rdma, fc)
> just want to make sure we are not breaking anything since we are changing
> the order of module unload and stop_soft_rdma() in this patch ...
Sure thing
>>> Before we unload the module we should cleanup the fc resources first,
>>> basically reorder the shutdown sequence to be in reverse order of the
>>> setup path.
>>
>> If this triggers a bug, then I think it is a good idea to have a
>> dedicated test that reproduces it if we are changing the default
>> behavior.
>
> Right, though I would like to tackle one problem after the other, first get fc
> working with the 'correct' order.
>
>>> While at it also update the rdma stop_soft_rdma before the module
>>> unloading for the same reasoning.
>>
>> Why? it creates the wrong reverse ordering.
>>
>> 1. setup soft-rdma
>> 2. setup nvme-rdma
>>
>> 2. teardown nvme-rdma
>> 1. teardown soft-rdma
>>
>> I don't think we need this change. I mean it is a good test
>> to have that the rdma device goes away underneath nvme-rdma
>> but it is good for a dedicated test.
>
> I was woried about this setup sequence here:
>
> modprobe -q nvme-"${nvme_trtype}"
> if [[ "${nvme_trtype}" == "rdma" ]]; then
> start_soft_rdma
>
> The module is loaded before start_soft_rdma is started, thus I thought we should
> do the reverse, first call stop_soft_rdma and the unload the module.
They should be unrelated. the safe route is to first remove the uld and
then the device.
On 4/19/23 02:44, Sagi Grimberg wrote:
>
>> Before we unload the module we should cleanup the fc resources first,
>> basically reorder the shutdown sequence to be in reverse order of the
>> setup path.
>
> If this triggers a bug, then I think it is a good idea to have a
> dedicated test that reproduces it if we are changing the default
> behavior.
>
+1
-ck
On Apr 19, 2023 / 21:15, Chaitanya Kulkarni wrote:
> On 4/19/23 02:44, Sagi Grimberg wrote:
> >
> >> Before we unload the module we should cleanup the fc resources first,
> >> basically reorder the shutdown sequence to be in reverse order of the
> >> setup path.
> >
> > If this triggers a bug, then I think it is a good idea to have a
> > dedicated test that reproduces it if we are changing the default
> > behavior.
> >
>
> +1
Agreed. Patch post for the new test case will be appreciated. Not to forget this
work, I will open a github issue later.
On Apr 19, 2023 / 12:36, Daniel Wagner wrote:
> On Wed, Apr 19, 2023 at 09:41:28AM +0000, Chaitanya Kulkarni wrote:
> > were you able to test this with RDMA ?
>
> Yes, I've tested it with all transports (loop, tcp, rdma, fc)
>
> > just want to make sure we are not breaking anything since we are changing
> > the order of module unload and stop_soft_rdma() in this patch ...
>
> Sure thing
I also tested, and observed no result change by these two patches. Only one
failure I observed is nvme/003 due to lockdep WARN, but it happens regardless
of the patches.
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 <[email protected]>
> > ---
> > 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 Apr 19, 2023 / 13:45, Sagi Grimberg wrote:
>
> > > > Before we unload the module we should cleanup the fc resources first,
> > > > basically reorder the shutdown sequence to be in reverse order of the
> > > > setup path.
> > >
> > > If this triggers a bug, then I think it is a good idea to have a
> > > dedicated test that reproduces it if we are changing the default
> > > behavior.
> >
> > Right, though I would like to tackle one problem after the other, first get fc
> > working with the 'correct' order.
> >
> > > > While at it also update the rdma stop_soft_rdma before the module
> > > > unloading for the same reasoning.
> > >
> > > Why? it creates the wrong reverse ordering.
> > >
> > > 1. setup soft-rdma
> > > 2. setup nvme-rdma
> > >
> > > 2. teardown nvme-rdma
> > > 1. teardown soft-rdma
> > >
> > > I don't think we need this change. I mean it is a good test
> > > to have that the rdma device goes away underneath nvme-rdma
> > > but it is good for a dedicated test.
I agree that the new test case is good.
> >
> > I was woried about this setup sequence here:
> >
> > modprobe -q nvme-"${nvme_trtype}"
> > if [[ "${nvme_trtype}" == "rdma" ]]; then
> > start_soft_rdma
> >
> > The module is loaded before start_soft_rdma is started, thus I thought we should
> > do the reverse, first call stop_soft_rdma and the unload the module.
>
> They should be unrelated. the safe route is to first remove the uld and
> then the device.
Sagi, this comment above was not clear for me. Is Daniel's patch ok for you?
IMO, it is reasonable to "do clean-up in reverse order as setup" as a general
guide. It will reduce the chance to see module related failures when the test
cases do not expect such failures. Instead, we can have dedicated test cases for
the module load/unload order related failures. start_soft_rdma and
stop_soft_rdma do module load and unload. So I think the guide is good for those
helper functions also.
On 4/30/23 13:34, Shinichiro Kawasaki wrote:
> On Apr 19, 2023 / 13:45, Sagi Grimberg wrote:
>>
>>>>> Before we unload the module we should cleanup the fc resources first,
>>>>> basically reorder the shutdown sequence to be in reverse order of the
>>>>> setup path.
>>>>
>>>> If this triggers a bug, then I think it is a good idea to have a
>>>> dedicated test that reproduces it if we are changing the default
>>>> behavior.
>>>
>>> Right, though I would like to tackle one problem after the other, first get fc
>>> working with the 'correct' order.
>>>
>>>>> While at it also update the rdma stop_soft_rdma before the module
>>>>> unloading for the same reasoning.
>>>>
>>>> Why? it creates the wrong reverse ordering.
>>>>
>>>> 1. setup soft-rdma
>>>> 2. setup nvme-rdma
>>>>
>>>> 2. teardown nvme-rdma
>>>> 1. teardown soft-rdma
>>>>
>>>> I don't think we need this change. I mean it is a good test
>>>> to have that the rdma device goes away underneath nvme-rdma
>>>> but it is good for a dedicated test.
>
> I agree that the new test case is good.
>
>>>
>>> I was woried about this setup sequence here:
>>>
>>> modprobe -q nvme-"${nvme_trtype}"
>>> if [[ "${nvme_trtype}" == "rdma" ]]; then
>>> start_soft_rdma
>>>
>>> The module is loaded before start_soft_rdma is started, thus I thought we should
>>> do the reverse, first call stop_soft_rdma and the unload the module.
>>
>> They should be unrelated. the safe route is to first remove the uld and
>> then the device.
>
> Sagi, this comment above was not clear for me. Is Daniel's patch ok for you?
>
> IMO, it is reasonable to "do clean-up in reverse order as setup" as a general
> guide. It will reduce the chance to see module related failures when the test
> cases do not expect such failures. Instead, we can have dedicated test cases for
> the module load/unload order related failures. start_soft_rdma and
> stop_soft_rdma do module load and unload. So I think the guide is good for those
> helper functions also.
As I mentioned here, this change exercises a code path in the driver
that is a surprise unplug of the rdma device. It is equivalent to
triggering a surprise removal of the pci device normally during
nvme-pci test teardown. While this is worth testing, I'm not sure we
want the default behavior to do that, but rather add dedicated tests for
it.
Hence, my suggestion was to leave nvme-rdma as is.
On May 01, 2023 / 17:10, Sagi Grimberg wrote:
> On 4/30/23 13:34, Shinichiro Kawasaki wrote:
[...]
> > Sagi, this comment above was not clear for me. Is Daniel's patch ok for you?
> >
> > IMO, it is reasonable to "do clean-up in reverse order as setup" as a general
> > guide. It will reduce the chance to see module related failures when the test
> > cases do not expect such failures. Instead, we can have dedicated test cases for
> > the module load/unload order related failures. start_soft_rdma and
> > stop_soft_rdma do module load and unload. So I think the guide is good for those
> > helper functions also.
>
> As I mentioned here, this change exercises a code path in the driver
> that is a surprise unplug of the rdma device. It is equivalent to
> triggering a surprise removal of the pci device normally during
> nvme-pci test teardown. While this is worth testing, I'm not sure we
> want the default behavior to do that, but rather add dedicated tests for
> it.
>
> Hence, my suggestion was to leave nvme-rdma as is.
Thanks for the clarification. I assume that stop_soft_rdma is the "surprise
unplug of the rdma device". If I understand it correctly, the change for nvme-fc
will be like this:
diff --git a/tests/nvme/rc b/tests/nvme/rc
index ec0cc2d..24803af 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -260,6 +260,11 @@ _cleanup_nvmet() {
shopt -u nullglob
trap SIGINT
+ if [[ "${nvme_trtype}" == "fc" ]]; then
+ _cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
+ "${def_remote_wwnn}" "${def_remote_wwpn}"
+ modprobe -rq nvme-fcloop
+ fi
modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
if [[ "${nvme_trtype}" != "loop" ]]; then
modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
@@ -268,10 +273,6 @@ _cleanup_nvmet() {
if [[ "${nvme_trtype}" == "rdma" ]]; then
stop_soft_rdma
fi
- if [[ "${nvme_trtype}" == "fc" ]]; then
- _cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
- "${def_remote_wwnn}" "${def_remote_wwpn}"
- fi
}
_setup_nvmet() {
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 <[email protected]>
> > > ---
> > > 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.