[PATCH v2 0/2] Fix missing AENs when discovery controllers are disconnected

[PATCH v2 0/2] Fix missing AENs when discovery controllers are disconnected

[Sagi Grimberg]

When a discovery controller is disconnected, no AENs will
arrive to notify the host about discovery log-page changes.

Fix this with  trapping a new dedicated kernel udev event for persistent
discovery controllers that reconnect. Prior attempt tried to use
"connected" event already sent by the kernel however this also applied
on the first connected, causing undesired side-effects when issuing
a simple 'discover' command.

The patchset includes the nvme-cli counter-part as well.

Sagi Grimberg (2):
  nvme: enumerate controller flags
  nvme: send a rediscover uevent when a persistent discovery controller
    reconnects

 drivers/nvme/host/core.c | 10 ++++++++++
 drivers/nvme/host/nvme.h |  8 ++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] nvme: enumerate controller flags

[Sagi Grimberg]

We expect to grow a few of these flags for various purposes
so make them a proper enumeration.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/nvme.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bdc0ff7ed9ab..fcf891467e68 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -233,6 +233,11 @@ struct nvme_fault_inject {
 #endif
 };
 
+enum nvme_ctrl_flags {
+	NVME_CTRL_FAILFAST_EXPIRED	= (1 << 0),
+	NVME_CTRL_ADMIN_Q_STOPPED	= (1 << 1),
+};
+
 struct nvme_ctrl {
 	bool comp_seen;
 	enum nvme_ctrl_state state;
@@ -354,8 +359,6 @@ struct nvme_ctrl {
 	u16 maxcmd;
 	int nr_reconnects;
 	unsigned long flags;
-#define NVME_CTRL_FAILFAST_EXPIRED	0
-#define NVME_CTRL_ADMIN_Q_STOPPED	1
 	struct nvmf_ctrl_options *opts;
 
 	struct page *discard_page;
-- 
2.34.1

Re: [PATCH v2 1/2] nvme: enumerate controller flags

[Daniel Wagner]

On Thu, Sep 22, 2022 at 11:15:36AM +0300, Sagi Grimberg wrote:
> We expect to grow a few of these flags for various purposes
> so make them a proper enumeration.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH v2 1/2] nvme: enumerate controller flags

[James Smart]

On 9/22/2022 1:15 AM, Sagi Grimberg wrote:
> We expect to grow a few of these flags for various purposes
> so make them a proper enumeration.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/nvme.h | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: James Smart <jsmart2021@gmail.com>

-- james

Re: [PATCH v2 1/2] nvme: enumerate controller flags

[Keith Busch]

On Thu, Sep 22, 2022 at 11:15:36AM +0300, Sagi Grimberg wrote:
> We expect to grow a few of these flags for various purposes
> so make them a proper enumeration.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/nvme.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bdc0ff7ed9ab..fcf891467e68 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -233,6 +233,11 @@ struct nvme_fault_inject {
>  #endif
>  };
>  
> +enum nvme_ctrl_flags {
> +	NVME_CTRL_FAILFAST_EXPIRED	= (1 << 0),
> +	NVME_CTRL_ADMIN_Q_STOPPED	= (1 << 1),
> +};

Don't shift these. They are used with set/clear/test_bit() APIs, which take the
bit position rather than the value.

> +
>  struct nvme_ctrl {
>  	bool comp_seen;
>  	enum nvme_ctrl_state state;
> @@ -354,8 +359,6 @@ struct nvme_ctrl {
>  	u16 maxcmd;
>  	int nr_reconnects;
>  	unsigned long flags;
> -#define NVME_CTRL_FAILFAST_EXPIRED	0
> -#define NVME_CTRL_ADMIN_Q_STOPPED	1
>  	struct nvmf_ctrl_options *opts;
>  
>  	struct page *discard_page;

[PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[Sagi Grimberg]

When a discovery controller is disconnected, no AENs will arrive to
notify the host about discovery log change events.

In order to solve this, send a uevent notification when a
persistent discovery controller reconnects. We add a new ctrl
flag NVME_CTRL_STARTED_ONCE that will be set on the first
start, and consecutive calls will find it set, and send the
event to userspace if the controller is a discovery controller.

Upon the event reception, userspace will re-read the discovery
log page and will act upon changes as it sees fit.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 10 ++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 70ebf27ad10e..07a21e0b9cf7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4822,6 +4822,16 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 
 	nvme_enable_aen(ctrl);
 
+	/*
+	 * persistent discovery controllers need to send indication to userspace
+	 * to re-read the discovery log page to learn about possible changes
+	 * that were missed. We identify persistent discovery controllers by
+	 * checking that they started once before, hence are reconnecting back.
+	 */
+	if (test_and_set_bit(NVME_CTRL_STARTED_ONCE, &ctrl->flags) &&
+	    nvme_discovery_ctrl(ctrl))
+		nvme_change_uevent(ctrl, "NVME_EVENT=rediscover");
+
 	if (ctrl->queue_count > 1) {
 		nvme_queue_scan(ctrl);
 		nvme_start_queues(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index fcf891467e68..2d5d44a73f26 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -236,6 +236,7 @@ struct nvme_fault_inject {
 enum nvme_ctrl_flags {
 	NVME_CTRL_FAILFAST_EXPIRED	= (1 << 0),
 	NVME_CTRL_ADMIN_Q_STOPPED	= (1 << 1),
+	NVME_CTRL_STARTED_ONCE		= (1 << 2),
 };
 
 struct nvme_ctrl {
-- 
2.34.1

Re: [PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[Daniel Wagner]

On Thu, Sep 22, 2022 at 11:15:37AM +0300, Sagi Grimberg wrote:
> When a discovery controller is disconnected, no AENs will arrive to
> notify the host about discovery log change events.
> 
> In order to solve this, send a uevent notification when a
> persistent discovery controller reconnects. We add a new ctrl
> flag NVME_CTRL_STARTED_ONCE that will be set on the first
> start, and consecutive calls will find it set, and send the
> event to userspace if the controller is a discovery controller.
> 
> Upon the event reception, userspace will re-read the discovery
> log page and will act upon changes as it sees fit.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 10 ++++++++++
>  drivers/nvme/host/nvme.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 70ebf27ad10e..07a21e0b9cf7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4822,6 +4822,16 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)

Stupid question: nvme_start_ctrl() is obviously also used in pci.c. Is
it expected that a PCI device also sends out the rediscover event?

Re: [PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[Sagi Grimberg]

>> When a discovery controller is disconnected, no AENs will arrive to
>> notify the host about discovery log change events.
>>
>> In order to solve this, send a uevent notification when a
>> persistent discovery controller reconnects. We add a new ctrl
>> flag NVME_CTRL_STARTED_ONCE that will be set on the first
>> start, and consecutive calls will find it set, and send the
>> event to userspace if the controller is a discovery controller.
>>
>> Upon the event reception, userspace will re-read the discovery
>> log page and will act upon changes as it sees fit.
>>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/host/core.c | 10 ++++++++++
>>   drivers/nvme/host/nvme.h |  1 +
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 70ebf27ad10e..07a21e0b9cf7 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -4822,6 +4822,16 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
> 
> Stupid question: nvme_start_ctrl() is obviously also used in pci.c. Is
> it expected that a PCI device also sends out the rediscover event?

It won't pass the nvme_discovery_ctrl test.

Re: [PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[Daniel Wagner]

On Thu, Sep 22, 2022 at 10:30:25AM +0200, Daniel Wagner wrote:
> Stupid question: nvme_start_ctrl() is obviously also used in pci.c. Is
> it expected that a PCI device also sends out the rediscover event?

Forget it. I missed the nvme_discovery_ctrl() part.

Re: [PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[Daniel Wagner]

On Thu, Sep 22, 2022 at 11:15:37AM +0300, Sagi Grimberg wrote:
> When a discovery controller is disconnected, no AENs will arrive to
> notify the host about discovery log change events.
> 
> In order to solve this, send a uevent notification when a
> persistent discovery controller reconnects. We add a new ctrl
> flag NVME_CTRL_STARTED_ONCE that will be set on the first
> start, and consecutive calls will find it set, and send the
> event to userspace if the controller is a discovery controller.
> 
> Upon the event reception, userspace will re-read the discovery
> log page and will act upon changes as it sees fit.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH v2 2/2] nvme: send a rediscover uevent when a persistent discovery controller reconnects

[James Smart]

On 9/22/2022 1:15 AM, Sagi Grimberg wrote:
> When a discovery controller is disconnected, no AENs will arrive to
> notify the host about discovery log change events.
> 
> In order to solve this, send a uevent notification when a
> persistent discovery controller reconnects. We add a new ctrl
> flag NVME_CTRL_STARTED_ONCE that will be set on the first
> start, and consecutive calls will find it set, and send the
> event to userspace if the controller is a discovery controller.
> 
> Upon the event reception, userspace will re-read the discovery
> log page and will act upon changes as it sees fit.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 10 ++++++++++
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c


Reviewed-by: James Smart <jsmart2021@gmail.com>

-- james

[PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Sagi Grimberg]

When using persistent discovery controllers, if the discovery controller
loses connectivity and manage to reconnect after a while, we need to
retrieve again the discovery log page in order to learn about possible
changes that may have occurred during this time as discovery log change
events were lost.

Upon reception of a udev EVENT=rediscover we can kickstart discovery on
the existing discovery controller device node that generated the event.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
index 434cc080ffe3..93e438863672 100644
--- a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
+++ b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
@@ -17,3 +17,10 @@ ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="0x70f002",\
 ACTION=="change", SUBSYSTEM=="fc", ENV{FC_EVENT}=="nvmediscovery", \
   ENV{NVMEFC_HOST_TRADDR}=="*",  ENV{NVMEFC_TRADDR}=="*", \
   RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--device=none\t--transport=fc\t--traddr=$env{NVMEFC_TRADDR}\t--trsvcid=none\t--host-traddr=$env{NVMEFC_HOST_TRADDR}.service"
+
+# A discovery controller just (re)connected, re-read the discovery log change to
+# check if there were any changes since it was last connected.
+ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_EVENT}=="rediscover", ATTR{cntrltype}=="discovery", \
+  ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
+  ENV{NVME_TRSVCID}=="*", ENV{NVME_HOST_TRADDR}=="*", \
+  RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--device=$kernel\t--transport=$env{NVME_TRTYPE}\t--traddr=$env{NVME_TRADDR}\t--trsvcid=$env{NVME_TRSVCID}\t--host-traddr=$env{NVME_HOST_TRADDR}.service"
-- 
2.34.1

Re: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Daniel Wagner]

On Thu, Sep 22, 2022 at 11:15:38AM +0300, Sagi Grimberg wrote:
> When using persistent discovery controllers, if the discovery controller
> loses connectivity and manage to reconnect after a while, we need to
> retrieve again the discovery log page in order to learn about possible
> changes that may have occurred during this time as discovery log change
> events were lost.
> 
> Upon reception of a udev EVENT=rediscover we can kickstart discovery on
> the existing discovery controller device node that generated the event.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[James Smart]

On 9/22/2022 1:15 AM, Sagi Grimberg wrote:
> When using persistent discovery controllers, if the discovery controller
> loses connectivity and manage to reconnect after a while, we need to
> retrieve again the discovery log page in order to learn about possible
> changes that may have occurred during this time as discovery log change
> events were lost.
> 
> Upon reception of a udev EVENT=rediscover we can kickstart discovery on
> the existing discovery controller device node that generated the event.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in | 7 +++++++
>   1 file changed, 7 insertions(+)
> 

Reviewed-by: James Smart <jsmart2021@gmail.com>

-- james

Re: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Daniel Wagner]

On Thu, Sep 22, 2022 at 11:15:38AM +0300, Sagi Grimberg wrote:
> When using persistent discovery controllers, if the discovery controller
> loses connectivity and manage to reconnect after a while, we need to
> retrieve again the discovery log page in order to learn about possible
> changes that may have occurred during this time as discovery log change
> events were lost.
> 
> Upon reception of a udev EVENT=rediscover we can kickstart discovery on
> the existing discovery controller device node that generated the event.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> index 434cc080ffe3..93e438863672 100644
> --- a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> +++ b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> @@ -17,3 +17,10 @@ ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="0x70f002",\
>  ACTION=="change", SUBSYSTEM=="fc", ENV{FC_EVENT}=="nvmediscovery", \
>    ENV{NVMEFC_HOST_TRADDR}=="*",  ENV{NVMEFC_TRADDR}=="*", \
>    RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--device=none\t--transport=fc\t--traddr=$env{NVMEFC_TRADDR}\t--trsvcid=none\t--host-traddr=$env{NVMEFC_HOST_TRADDR}.service"
> +
> +# A discovery controller just (re)connected, re-read the discovery log change to
> +# check if there were any changes since it was last connected.
> +ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_EVENT}=="rediscover", ATTR{cntrltype}=="discovery", \
> +  ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
> +  ENV{NVME_TRSVCID}=="*", ENV{NVME_HOST_TRADDR}=="*", \
> +  RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--device=$kernel\t--transport=$env{NVME_TRTYPE}\t--traddr=$env{NVME_TRADDR}\t--trsvcid=$env{NVME_TRSVCID}\t--host-traddr=$env{NVME_HOST_TRADDR}.service"

I was about to apply the change. Though I just added the change from
Martin where the --host_iface argument is added to the first nvme
rule.

https://github.com/linux-nvme/nvme-cli/pull/1675

I wonder to we need to it to append the --host_iface argument as well.

Daniel

RE: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Belanger, Martin]

> On Thu, Sep 22, 2022 at 11:15:38AM +0300, Sagi Grimberg wrote:
> > When using persistent discovery controllers, if the discovery
> > controller loses connectivity and manage to reconnect after a while,
> > we need to retrieve again the discovery log page in order to learn
> > about possible changes that may have occurred during this time as
> > discovery log change events were lost.
> >
> > Upon reception of a udev EVENT=rediscover we can kickstart discovery
> > on the existing discovery controller device node that generated the event.
> >
> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > ---
> >  nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> > b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> > index 434cc080ffe3..93e438863672 100644
> > --- a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> > +++ b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules.in
> > @@ -17,3 +17,10 @@ ACTION=="change", SUBSYSTEM=="nvme",
> > ENV{NVME_AEN}=="0x70f002",\  ACTION=="change", SUBSYSTEM=="fc",
> ENV{FC_EVENT}=="nvmediscovery", \
> >    ENV{NVMEFC_HOST_TRADDR}=="*",  ENV{NVMEFC_TRADDR}=="*", \
> >    RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--device=none\t--
> transport=fc\t--traddr=$env{NVMEFC_TRADDR}\t--trsvcid=none\t--host-
> traddr=$env{NVMEFC_HOST_TRADDR}.service"
> > +
> > +# A discovery controller just (re)connected, re-read the discovery
> > +log change to # check if there were any changes since it was last
> connected.
> > +ACTION=="change", SUBSYSTEM=="nvme",
> ENV{NVME_EVENT}=="rediscover",
> > +ATTR{cntrltype}=="discovery", \
> > +  ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
> > +  ENV{NVME_TRSVCID}=="*", ENV{NVME_HOST_TRADDR}=="*", \
> > +  RUN+="@SYSTEMCTL@ --no-block start nvmf-connect@--
> device=$kernel\t--transport=$env{NVME_TRTYPE}\t--
> traddr=$env{NVME_TRADDR}\t--trsvcid=$env{NVME_TRSVCID}\t--host-
> traddr=$env{NVME_HOST_TRADDR}.service"
> 
> I was about to apply the change. Though I just added the change from Martin
> where the --host_iface argument is added to the first nvme rule.
> 
> https://urldefense.com/v3/__https://github.com/linux-nvme/nvme-
> cli/pull/1675__;!!LpKI!kH_mYipHgbqgVMaCaU3Jo03OOkaawd03WAxINmKpN
> 1LsV_CQAbAXkX24oxXLrHDNUfwUuQLFcN6KriuoJuY$  [github[.]com]
> 
> I wonder to we need to it to append the --host_iface argument as well.
Yes, the --host-iface should be added for this case too. 
Martin

> 
> Daniel

Re: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Daniel Wagner]

On Tue, Sep 27, 2022 at 10:50:08AM +0000, Belanger, Martin wrote:
> > I wonder to we need to it to append the --host_iface argument as well.
> Yes, the --host-iface should be added for this case too. 

Okay, I'll add them.

Re: [PATCH v2 3/2 nvme-cli] fabrics: re-read the discovery log page when a discovery controller reconnected

[Daniel Wagner]

On Tue, Sep 27, 2022 at 01:22:54PM +0200, Daniel Wagner wrote:
> On Tue, Sep 27, 2022 at 10:50:08AM +0000, Belanger, Martin wrote:
> > > I wonder to we need to it to append the --host_iface argument as well.
> > Yes, the --host-iface should be added for this case too. 
> 
> Okay, I'll add them.

Patch applied. Thanks!

Re: [PATCH v2 0/2] Fix missing AENs when discovery controllers are disconnected

[Christoph Hellwig]

Thanks,

applied to nvme-6.1 after fixing up the bit positions.

Re: [PATCH v2 0/2] Fix missing AENs when discovery controllers are disconnected

[Sagi Grimberg]

> Thanks,
> 
> applied to nvme-6.1 after fixing up the bit positions.

Thanks Christoph