[PATCH 0/3 rfc] Support discovery log change events

[PATCH 0/3 rfc] Support discovery log change events

[Sagi Grimberg]

We want to be able to support discovery log change events automatically
without user intervention.

The definition of discovery log change events applies on "persistent" long
lived controllers, so first we need to have discovery controllers to stay
for a long time and accept kato value.

Then when we do happen to get a discovery log change event on the persistent
discovery controller, we simply fire a udev event to user-space to re-query
the discovery log page and connect to new subsystems in the fabric.

Review and feedback is welcome.

Sagi Grimberg (3):
  nvme-fabrics: allow discovery subsystems accept a kato
  nvme: enable aen also for discovery controllers
  nvme: fire discovery log page change events to userspace

 drivers/nvme/host/core.c    | 37 ++++++++++++++++++++++++++++++++++---
 drivers/nvme/host/fabrics.c | 12 ++----------
 2 files changed, 36 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH 1/3 rfc] nvme-fabrics: allow discovery subsystems accept a kato

[Sagi Grimberg]

This modifies the behavior of discovery subsystems to accept
a kato as a preparation to support discovery log change
events. This also means that now every discovery controller
will have a default kato value, and for non-persistent connections
the host needs to pass in a zero kato value (keep_alive_tmo=0).

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/fabrics.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 70c09abcfcbf..9d53ea4939cd 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -389,8 +389,8 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	 * Set keep-alive timeout in seconds granularity (ms * 1000)
 	 * and add a grace period for controller kato enforcement
 	 */
-	cmd.connect.kato = ctrl->opts->discovery_nqn ? 0 :
-		cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000);
+	cmd.connect.kato = ctrl->kato ?
+		cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000) : 0;
 
 	if (ctrl->opts->disable_sqflow)
 		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
@@ -746,13 +746,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				pr_warn("keep_alive_tmo 0 won't execute keep alives!!!\n");
 			}
 			opts->kato = token;
-
-			if (opts->discovery_nqn && opts->kato) {
-				pr_err("Discovery controllers cannot accept KATO != 0\n");
-				ret = -EINVAL;
-				goto out;
-			}
-
 			break;
 		case NVMF_OPT_CTRL_LOSS_TMO:
 			if (match_int(args, &token)) {
@@ -873,7 +866,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	}
 
 	if (opts->discovery_nqn) {
-		opts->kato = 0;
 		opts->nr_io_queues = 0;
 		opts->nr_write_queues = 0;
 		opts->nr_poll_queues = 0;
-- 
2.17.1

[PATCH 2/3 rfc] nvme: enable aen also for discovery controllers

[Sagi Grimberg]

If the controller supports discovery log page change events,
we want to enable it on the host side as well.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9ec88253ebcd..348af75583e5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1178,6 +1178,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 	if (!supported_aens)
 		return;
 
+	queue_work(nvme_wq, &ctrl->async_event_work);
+
 	status = nvme_set_features(ctrl, NVME_FEAT_ASYNC_EVENT, supported_aens,
 			NULL, 0, &result);
 	if (status)
@@ -3662,10 +3664,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 	if (ctrl->kato)
 		nvme_start_keep_alive(ctrl);
 
+	if (ctrl->queue_count > 1 ||
+	    (ctrl->ops->flags & NVME_F_FABRICS &&
+	     ctrl->opts->discovery_nqn))
+		nvme_enable_aen(ctrl);
+
 	if (ctrl->queue_count > 1) {
 		nvme_queue_scan(ctrl);
-		nvme_enable_aen(ctrl);
-		queue_work(nvme_wq, &ctrl->async_event_work);
 		nvme_start_queues(ctrl);
 	}
 }
-- 
2.17.1

[PATCH 3/3 rfc] nvme: fire discovery log page change events to userspace

[Sagi Grimberg]

Provide userspace with nvme discovery controller device instance,
controller traddr and trsvcid. We'd expect userspace to handle
this event by issuing a discovery + connect.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 348af75583e5..d29ff9b910d9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1168,7 +1168,8 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 EXPORT_SYMBOL_GPL(nvme_set_queue_count);
 
 #define NVME_AEN_SUPPORTED \
-	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE)
+	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE | \
+	 NVME_AEN_CFG_DISC_CHANGE)
 
 static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 {
@@ -3527,6 +3528,28 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
 	kfree(envp[0]);
 }
 
+static void nvme_disc_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	struct nvmf_ctrl_options *opts = ctrl->opts;
+	char *envp[16];
+	int i, envloc = 0;
+
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_EVENT=discovery");
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_INSTANCE=%d", ctrl->instance);
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRTYPE=%s", opts->transport);
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRADDR=%s", opts->traddr);
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRSVCID=%s", opts->trsvcid);
+	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_HOST_TRADDR=%s",
+			opts->host_traddr ?: "none");
+	envp[envloc] = NULL;
+
+	for (i = 0; i < envloc; i++)
+		dev_dbg(ctrl->device, "%s\n", envp[i]);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+	for (i = 0; i < envloc; i++)
+		kfree(envp[i]);
+}
+
 static void nvme_async_event_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl =
@@ -3618,6 +3641,9 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 		queue_work(nvme_wq, &ctrl->ana_work);
 		break;
 #endif
+	case NVME_AER_NOTICE_DISC_CHANGED:
+		nvme_disc_aen_uevent(ctrl);
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
-- 
2.17.1

[PATCH rfc nvme-cli 4/5] fabrics: support persistent connections to a discovery controller

[Sagi Grimberg]

Simply don't destroy the discovery controller after getting the
log pages. Note that persistent connection to a discovery subsystem
require to pass in a non-zero kato value, so if not provided we
simply use a default of 30 seconds kato.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 10 +++++++++-
 fabrics.h |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fabrics.c b/fabrics.c
index 029105b227ff..44aee67759c8 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -62,6 +62,7 @@ static struct config {
 	char *raw;
 	char *device;
 	int  duplicate_connect;
+	bool persistent;
 } cfg = { NULL };
 
 #define BUF_SIZE		4096
@@ -743,7 +744,8 @@ static int do_discover(char *argstr, bool connect)
 		return errno;
 	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec);
 	free(dev_name);
-	remove_ctrl(instance);
+	if (!cfg.persistent)
+		remove_ctrl(instance);
 
 	switch (ret) {
 	case DISC_OK:
@@ -819,6 +821,9 @@ static int discover_from_conf_file(const char *desc, char *argstr,
 		if (err)
 			continue;
 
+		if (cfg.persistent && !cfg.keep_alive_tmo)
+			cfg.keep_alive_tmo = NVMF_DEF_DISC_TMO;
+
 		err = build_options(argstr, BUF_SIZE);
 		if (err) {
 			ret = err;
@@ -855,6 +860,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"persistent",  'p', "LIST", CFG_NONE, &cfg.persistent,  no_argument, "persistent discovery connection" },
 		{NULL},
 	};
 
@@ -869,6 +875,8 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		return discover_from_conf_file(desc, argstr,
 				command_line_options, connect);
 	} else {
+		if (cfg.persistent && !cfg.keep_alive_tmo)
+			cfg.keep_alive_tmo = NVMF_DEF_DISC_TMO;
 		ret = build_options(argstr, BUF_SIZE);
 		if (ret)
 			return ret;
diff --git a/fabrics.h b/fabrics.h
index 988f3ef2fbc4..7c1664b80d51 100644
--- a/fabrics.h
+++ b/fabrics.h
@@ -1,6 +1,8 @@
 #ifndef _DISCOVER_H
 #define _DISCOVER_H
 
+#define NVMF_DEF_DISC_TMO	30
+
 extern int discover(const char *desc, int argc, char **argv, bool connect);
 extern int connect(const char *desc, int argc, char **argv);
 extern int disconnect(const char *desc, int argc, char **argv);
-- 
2.17.1

[PATCH rfc nvme-cli 5/5] fabrics: allow user to retrieve discovery log from existing discovery controller

[Sagi Grimberg]

Simply allow discover to receive the discovery device node name.
Also centralize extraction of controller instance from the controller
name to a common helper.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 44aee67759c8..703020fa5b17 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -172,6 +172,19 @@ static const char *cms_str(__u8 cm)
 
 static int do_discover(char *argstr, bool connect);
 
+static int ctrl_instance(char *device)
+{
+	int ret, instance;
+
+	device = basename(device);
+	ret = sscanf(device, "nvme%d", &instance);
+	if (ret < 0)
+		return ret;
+	if (!ret)
+		return -1;
+	return instance;
+}
+
 static int add_ctrl(const char *argstr)
 {
 	substring_t args[MAX_OPT_ARGS];
@@ -736,7 +749,10 @@ static int do_discover(char *argstr, bool connect)
 	char *dev_name;
 	int instance, numrec = 0, ret;
 
-	instance = add_ctrl(argstr);
+	if (!cfg.device)
+		instance = add_ctrl(argstr);
+	else
+		instance = ctrl_instance(cfg.device);
 	if (instance < 0)
 		return instance;
 
@@ -744,7 +760,7 @@ static int do_discover(char *argstr, bool connect)
 		return errno;
 	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec);
 	free(dev_name);
-	if (!cfg.persistent)
+	if (!cfg.device && !cfg.persistent)
 		remove_ctrl(instance);
 
 	switch (ret) {
@@ -857,6 +873,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"hostnqn",     'q', "LIST", CFG_STRING, &cfg.hostnqn,     required_argument, "user-defined hostnqn (if default not used)" },
 		{"hostid",      'I', "LIST", CFG_STRING, &cfg.hostid,      required_argument, "user-defined hostid (if default not used)"},
 		{"raw",         'r', "LIST", CFG_STRING, &cfg.raw,         required_argument, "raw output file" },
+		{"device",      'd', "LIST", CFG_STRING, &cfg.device, required_argument, "use existing discovery contoller device" },
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
@@ -1001,15 +1018,10 @@ static int disconnect_by_nqn(char *nqn)
 static int disconnect_by_device(char *device)
 {
 	int instance;
-	int ret;
-
-	device = basename(device);
-	ret = sscanf(device, "nvme%d", &instance);
-	if (ret < 0)
-		return ret;
-	if (!ret)
-		return -1;
 
+	instance = ctrl_instance(device);
+	if (instance < 0)
+		return instance;
 	return remove_ctrl(instance);
 }
 
-- 
2.17.1

[PATCH rfc nvme-cli 4/5] fabrics: support persistent connections to a discovery controller

[Sagi Grimberg]

Please ignore this patch - it is a part of the nvme-cli patch set

[PATCH rfc nvme-cli 5/5] fabrics: allow user to retrieve discovery log from existing discovery controller

[Sagi Grimberg]

Please ignore this patch - it is a part of the nvme-cli patch set

[PATCH 1/3 rfc] nvme-fabrics: allow discovery subsystems accept a kato

[Hannes Reinecke]

On 2/23/19 3:31 AM, Sagi Grimberg wrote:
> This modifies the behavior of discovery subsystems to accept
> a kato as a preparation to support discovery log change
> events. This also means that now every discovery controller
> will have a default kato value, and for non-persistent connections
> the host needs to pass in a zero kato value (keep_alive_tmo=0).
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/fabrics.c | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 70c09abcfcbf..9d53ea4939cd 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -389,8 +389,8 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
>   	 * Set keep-alive timeout in seconds granularity (ms * 1000)
>   	 * and add a grace period for controller kato enforcement
>   	 */
> -	cmd.connect.kato = ctrl->opts->discovery_nqn ? 0 :
> -		cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000);
> +	cmd.connect.kato = ctrl->kato ?
> +		cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000) : 0;
>   
>   	if (ctrl->opts->disable_sqflow)
>   		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
> @@ -746,13 +746,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   				pr_warn("keep_alive_tmo 0 won't execute keep alives!!!\n");
>   			}
>   			opts->kato = token;
> -
> -			if (opts->discovery_nqn && opts->kato) {
> -				pr_err("Discovery controllers cannot accept KATO != 0\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -
>   			break;
>   		case NVMF_OPT_CTRL_LOSS_TMO:
>   			if (match_int(args, &token)) {
> @@ -873,7 +866,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   	}
>   
>   	if (opts->discovery_nqn) {
> -		opts->kato = 0;
>   		opts->nr_io_queues = 0;
>   		opts->nr_write_queues = 0;
>   		opts->nr_poll_queues = 0;
> 

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

[PATCH 2/3 rfc] nvme: enable aen also for discovery controllers

[Hannes Reinecke]

On 2/23/19 3:31 AM, Sagi Grimberg wrote:
> If the controller supports discovery log page change events,
> we want to enable it on the host side as well.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9ec88253ebcd..348af75583e5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1178,6 +1178,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>   	if (!supported_aens)
>   		return;
>   
> +	queue_work(nvme_wq, &ctrl->async_event_work);
> +
>   	status = nvme_set_features(ctrl, NVME_FEAT_ASYNC_EVENT, supported_aens,
>   			NULL, 0, &result);
>   	if (status)
> @@ -3662,10 +3664,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>   	if (ctrl->kato)
>   		nvme_start_keep_alive(ctrl);
>   
> +	if (ctrl->queue_count > 1 ||
> +	    (ctrl->ops->flags & NVME_F_FABRICS &&
> +	     ctrl->opts->discovery_nqn))
> +		nvme_enable_aen(ctrl);
> +
>   	if (ctrl->queue_count > 1) {
>   		nvme_queue_scan(ctrl);
> -		nvme_enable_aen(ctrl);
> -		queue_work(nvme_wq, &ctrl->async_event_work);
>   		nvme_start_queues(ctrl);
>   	}
>   }
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

[PATCH 3/3 rfc] nvme: fire discovery log page change events to userspace

[Hannes Reinecke]

On 2/23/19 3:31 AM, Sagi Grimberg wrote:
> Provide userspace with nvme discovery controller device instance,
> controller traddr and trsvcid. We'd expect userspace to handle
> this event by issuing a discovery + connect.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 348af75583e5..d29ff9b910d9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1168,7 +1168,8 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
>   EXPORT_SYMBOL_GPL(nvme_set_queue_count);
>   
>   #define NVME_AEN_SUPPORTED \
> -	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE)
> +	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE | \
> +	 NVME_AEN_CFG_DISC_CHANGE)
>   
>   static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>   {
> @@ -3527,6 +3528,28 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>   	kfree(envp[0]);
>   }
>   
> +static void nvme_disc_aen_uevent(struct nvme_ctrl *ctrl)
> +{
> +	struct nvmf_ctrl_options *opts = ctrl->opts;
> +	char *envp[16];
> +	int i, envloc = 0;
> +
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_EVENT=discovery");
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_INSTANCE=%d", ctrl->instance);
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRTYPE=%s", opts->transport);
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRADDR=%s", opts->traddr);
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRSVCID=%s", opts->trsvcid);
> +	envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_HOST_TRADDR=%s",
> +			opts->host_traddr ?: "none");
> +	envp[envloc] = NULL;
> +
trsvcid is also optional, so we should be using

opts->trsvcid ?: "none"

here, too.

Cheers,

Hannes

[PATCH 2/3 rfc] nvme: enable aen also for discovery controllers

[Max Gurtovoy]

On 2/23/2019 4:31 AM, Sagi Grimberg wrote:
> If the controller supports discovery log page change events,
> we want to enable it on the host side as well.
>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9ec88253ebcd..348af75583e5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1178,6 +1178,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>   	if (!supported_aens)
>   		return;
>   
> +	queue_work(nvme_wq, &ctrl->async_event_work);
> +
>   	status = nvme_set_features(ctrl, NVME_FEAT_ASYNC_EVENT, supported_aens,
>   			NULL, 0, &result);
>   	if (status)
> @@ -3662,10 +3664,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>   	if (ctrl->kato)
>   		nvme_start_keep_alive(ctrl);
>   
> +	if (ctrl->queue_count > 1 ||
> +	    (ctrl->ops->flags & NVME_F_FABRICS &&
> +	     ctrl->opts->discovery_nqn))
> +		nvme_enable_aen(ctrl);
> +

why queue_count > 1 is a condition to enable AEN ?

Sounds more like an admin related action...

>   	if (ctrl->queue_count > 1) {
>   		nvme_queue_scan(ctrl);
> -		nvme_enable_aen(ctrl);
> -		queue_work(nvme_wq, &ctrl->async_event_work);
>   		nvme_start_queues(ctrl);
>   	}
>   }

[PATCH 2/3 rfc] nvme: enable aen also for discovery controllers

[Sagi Grimberg]

>> @@ -3662,10 +3664,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>> ????? if (ctrl->kato)
>> ????????? nvme_start_keep_alive(ctrl);
>> +??? if (ctrl->queue_count > 1 ||
>> +??????? (ctrl->ops->flags & NVME_F_FABRICS &&
>> +???????? ctrl->opts->discovery_nqn))
>> +??????? nvme_enable_aen(ctrl);
>> +
> 
> why queue_count > 1 is a condition to enable AEN ?

I just pulled the below code up, but it seems you are right
regardless of this patch. The AER should be independent of
having I/O queues.

> Sounds more like an admin related action...
> 
>> ????? if (ctrl->queue_count > 1) {
>> ????????? nvme_queue_scan(ctrl);
>> -??????? nvme_enable_aen(ctrl);
>> -??????? queue_work(nvme_wq, &ctrl->async_event_work);
>> ????????? nvme_start_queues(ctrl);
>> ????? }
>> ? }

[PATCH 3/3 rfc] nvme: fire discovery log page change events to userspace

[Sagi Grimberg]

>> @@ -3527,6 +3528,28 @@ static void nvme_aen_uevent(struct nvme_ctrl 
>> *ctrl)
>> ????? kfree(envp[0]);
>> ? }
>> +static void nvme_disc_aen_uevent(struct nvme_ctrl *ctrl)
>> +{
>> +??? struct nvmf_ctrl_options *opts = ctrl->opts;
>> +??? char *envp[16];
>> +??? int i, envloc = 0;
>> +
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_EVENT=discovery");
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_INSTANCE=%d", 
>> ctrl->instance);
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRTYPE=%s", 
>> opts->transport);
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRADDR=%s", 
>> opts->traddr);
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRSVCID=%s", 
>> opts->trsvcid);
>> +??? envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_HOST_TRADDR=%s",
>> +??????????? opts->host_traddr ?: "none");
>> +??? envp[envloc] = NULL;
>> +
> trsvcid is also optional, so we should be using
> 
> opts->trsvcid ?: "none"
> 
> here, too.


Yea, you're right