[PATCH v4 0/4] Support discovery log change events

[PATCH v4 0/4] 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.

This works with latest nvme-cli master with the nvme-cli patch added
to this series.

Changes from v3:
- Add nvme_class uevent callout for controller specific environment variables
- send discovery just like any AEN that we send to userspace
- merged discovery aen enable + send uevents to userspace into a single patch
  as they are now trivially adding support for the feature
- Added nvme-cli modifications to handle the new information from the event

Changes from v2:
- added patch to always enable aen, regardless of the number of I/O queues
- fixes line over 80 characters

Changes from v1:
- rebase to nvme-5.3
- pass none if trsvcid is uninitialized
- pass NVME_CTRL_NAME instead of NVME_CTRL_INSTANCE

Sagi Grimberg (4):
  nvme-fabrics: allow discovery subsystems accept a kato
  nvme: enable aen regardless of the presence of I/O queues
  nvme: add uevent variables for controller devices
  nvme: send discovery log page change events to userspace

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

-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 1/4] 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).

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
Reviewed-by: James Smart <james.smart@broadcom.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sagi Grimberg <sagi@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 145c210edb03..74b8818ac9a1 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -381,8 +381,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;
@@ -740,13 +740,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)) {
@@ -883,7 +876,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


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 1/4] nvme-fabrics: allow discovery subsystems accept a kato

[Chaitanya Kulkarni]

Looks good to me, btw do we have existing test-case which
covers this behavior ?

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 09/04/2019 03:00 PM, 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).
>
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: James Smart <james.smart@broadcom.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sagi Grimberg <sagi@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 145c210edb03..74b8818ac9a1 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -381,8 +381,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;
> @@ -740,13 +740,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)) {
> @@ -883,7 +876,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;
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 2/4] nvme: enable aen regardless of the presence of I/O queues

[Sagi Grimberg]

AENs in general are not related to the presence of I/O queues,
so enable them regardless. Note that the only exception is that
discovery controller will not support any of the requested AENs
and nvme_enable_aen will respect that and return, so it is still
safe to enable regardless.

Note it is safe to enable AENs even before the initial namespace
scanning as we have the scan operation in a workqueue context.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 14c0bfb55615..fccf28d77c03 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1201,6 +1201,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 	if (status)
 		dev_warn(ctrl->device, "Failed to configure AEN (cfg %x)\n",
 			 supported_aens);
+
+	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 
 static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
@@ -3780,10 +3782,10 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 	if (ctrl->kato)
 		nvme_start_keep_alive(ctrl);
 
+	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


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 2/4] nvme: enable aen regardless of the presence of I/O queues

[Hannes Reinecke]

On 9/4/19 11:59 PM, Sagi Grimberg wrote:
> AENs in general are not related to the presence of I/O queues,
> so enable them regardless. Note that the only exception is that
> discovery controller will not support any of the requested AENs
> and nvme_enable_aen will respect that and return, so it is still
> safe to enable regardless.
> 
> Note it is safe to enable AENs even before the initial namespace
> scanning as we have the scan operation in a workqueue context.
> 
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 14c0bfb55615..fccf28d77c03 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1201,6 +1201,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>  	if (status)
>  		dev_warn(ctrl->device, "Failed to configure AEN (cfg %x)\n",
>  			 supported_aens);
> +
> +	queue_work(nvme_wq, &ctrl->async_event_work);
>  }
>  
>  static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
> @@ -3780,10 +3782,10 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>  	if (ctrl->kato)
>  		nvme_start_keep_alive(ctrl);
>  
> +	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@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 2/4] nvme: enable aen regardless of the presence of I/O queues

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 09/04/2019 03:00 PM, Sagi Grimberg wrote:
> AENs in general are not related to the presence of I/O queues,
> so enable them regardless. Note that the only exception is that
> discovery controller will not support any of the requested AENs
> and nvme_enable_aen will respect that and return, so it is still
> safe to enable regardless.
>
> Note it is safe to enable AENs even before the initial namespace
> scanning as we have the scan operation in a workqueue context.
>
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 14c0bfb55615..fccf28d77c03 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1201,6 +1201,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>   	if (status)
>   		dev_warn(ctrl->device, "Failed to configure AEN (cfg %x)\n",
>   			 supported_aens);
> +
> +	queue_work(nvme_wq, &ctrl->async_event_work);
>   }
>
>   static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
> @@ -3780,10 +3782,10 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>   	if (ctrl->kato)
>   		nvme_start_keep_alive(ctrl);
>
> +	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);
>   	}
>   }
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 3/4] nvme: add uevent variables for controller devices

[Sagi Grimberg]

When we send uevents to userspace, add controller specific
environment variables to uniquly identify the controller beyond
its device name.

This will be useful to address discovery log change events by
actually verifying that the discovery controller is indeed the
same as the device that generated the event.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fccf28d77c03..7a1df95ec840 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3630,6 +3630,33 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
+static int nvme_class_uevent(struct device *d, struct kobj_uevent_env *env)
+{
+	struct nvme_ctrl *ctrl =
+		container_of(d, struct nvme_ctrl, ctrl_device);
+	struct nvmf_ctrl_options *opts = ctrl->opts;
+	int ret;
+
+	ret = add_uevent_var(env, "NVME_TRTYPE=%s", ctrl->ops->name);
+	if (ret)
+		return ret;
+
+	if (opts) {
+		ret = add_uevent_var(env, "NVME_TRADDR=%s", opts->traddr);
+		if (ret)
+			return ret;
+
+		ret = add_uevent_var(env, "NVME_TRSVCID=%s",
+			(opts && opts->trsvcid) ? opts->trsvcid : "none");
+		if (ret)
+			return ret;
+
+		ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
+			(opts && opts->host_traddr) ? opts->host_traddr : "none");
+	}
+	return ret;
+}
+
 static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
 {
 	char *envp[2] = { NULL, NULL };
@@ -4063,6 +4090,7 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_class);
 		goto unregister_chrdev;
 	}
+	nvme_class->dev_uevent = nvme_class_uevent,
 
 	nvme_subsys_class = class_create(THIS_MODULE, "nvme-subsystem");
 	if (IS_ERR(nvme_subsys_class)) {
-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[James Smart]

On 9/4/2019 2:59 PM, Sagi Grimberg wrote:
> When we send uevents to userspace, add controller specific
> environment variables to uniquly identify the controller beyond
> its device name.
>
> This will be useful to address discovery log change events by
> actually verifying that the discovery controller is indeed the
> same as the device that generated the event.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fccf28d77c03..7a1df95ec840 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3630,6 +3630,33 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
>   
> +static int nvme_class_uevent(struct device *d, struct kobj_uevent_env *env)
> +{
> +	struct nvme_ctrl *ctrl =
> +		container_of(d, struct nvme_ctrl, ctrl_device);
> +	struct nvmf_ctrl_options *opts = ctrl->opts;
> +	int ret;
> +
> +	ret = add_uevent_var(env, "NVME_TRTYPE=%s", ctrl->ops->name);
> +	if (ret)
> +		return ret;
> +
> +	if (opts) {
> +		ret = add_uevent_var(env, "NVME_TRADDR=%s", opts->traddr);
> +		if (ret)
> +			return ret;
> +
> +		ret = add_uevent_var(env, "NVME_TRSVCID=%s",
> +			(opts && opts->trsvcid) ? opts->trsvcid : "none");
> +		if (ret)
> +			return ret;
> +
> +		ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
> +			(opts && opts->host_traddr) ? opts->host_traddr : "none");
> +	}
> +	return ret;
> +}
> +
>   static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>   {
>   	char *envp[2] = { NULL, NULL };
> @@ -4063,6 +4090,7 @@ static int __init nvme_core_init(void)
>   		result = PTR_ERR(nvme_class);
>   		goto unregister_chrdev;
>   	}
> +	nvme_class->dev_uevent = nvme_class_uevent,

Comma at end of line ?

>   
>   	nvme_subsys_class = class_create(THIS_MODULE, "nvme-subsystem");
>   	if (IS_ERR(nvme_subsys_class)) {


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[Sagi Grimberg]

>>   static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>>   {
>>       char *envp[2] = { NULL, NULL };
>> @@ -4063,6 +4090,7 @@ static int __init nvme_core_init(void)
>>           result = PTR_ERR(nvme_class);
>>           goto unregister_chrdev;
>>       }
>> +    nvme_class->dev_uevent = nvme_class_uevent,
> 
> Comma at end of line ?

Hmm, still valid I guess.

I can fix it up, I'll wait for some more feedback first...

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[James Smart]

On 9/4/2019 5:18 PM, Sagi Grimberg wrote:
>
>>>   static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>>>   {
>>>       char *envp[2] = { NULL, NULL };
>>> @@ -4063,6 +4090,7 @@ static int __init nvme_core_init(void)
>>>           result = PTR_ERR(nvme_class);
>>>           goto unregister_chrdev;
>>>       }
>>> +    nvme_class->dev_uevent = nvme_class_uevent,
>>
>> Comma at end of line ?
>
> Hmm, still valid I guess.
>
> I can fix it up, I'll wait for some more feedback first...

Overall, it looked good to me.

Reviewed-by: James Smart <james.smart@broadcom.com>

-- james

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[Sagi Grimberg]

> Overall, it looked good to me.
> 
> Reviewed-by: James Smart <james.smart@broadcom.com>

Thanks,

Christoph, any feedback?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[Christoph Hellwig]

On Wed, Sep 04, 2019 at 02:59:52PM -0700, Sagi Grimberg wrote:
> When we send uevents to userspace, add controller specific
> environment variables to uniquly identify the controller beyond
> its device name.
> 
> This will be useful to address discovery log change events by
> actually verifying that the discovery controller is indeed the
> same as the device that generated the event.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fccf28d77c03..7a1df95ec840 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3630,6 +3630,33 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
>  
> +static int nvme_class_uevent(struct device *d, struct kobj_uevent_env *env)
> +{
> +	struct nvme_ctrl *ctrl =
> +		container_of(d, struct nvme_ctrl, ctrl_device);

Why the weird variable name of d instead of dev? as we usually use?


> +	if (opts) {

	if (!opts)
		return;

?  But then again how we can end up here without ctrl->opts?

> +		ret = add_uevent_var(env, "NVME_TRSVCID=%s",
> +			(opts && opts->trsvcid) ? opts->trsvcid : "none");

No need to check opts here again.

> +		ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
> +			(opts && opts->host_traddr) ? opts->host_traddr : "none");

Same here.

Otherwise this looks fine modulo the nitpick from James.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[Sagi Grimberg]

>> +static int nvme_class_uevent(struct device *d, struct kobj_uevent_env *env)
>> +{
>> +	struct nvme_ctrl *ctrl =
>> +		container_of(d, struct nvme_ctrl, ctrl_device);
> 
> Why the weird variable name of d instead of dev? as we usually use?

copy and paste from netdev_uevent :)

>> +	if (opts) {
> 
> 	if (!opts)
> 		return;
> 
> ?  But then again how we can end up here without ctrl->opts?

pci controllers.

>> +		ret = add_uevent_var(env, "NVME_TRSVCID=%s",
>> +			(opts && opts->trsvcid) ? opts->trsvcid : "none");
> 
> No need to check opts here again.

Will fix

> 
>> +		ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
>> +			(opts && opts->host_traddr) ? opts->host_traddr : "none");
> 
> Same here.

Will fix

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 3/4] nvme: add uevent variables for controller devices

[Hannes Reinecke]

On 9/4/19 11:59 PM, Sagi Grimberg wrote:
> When we send uevents to userspace, add controller specific
> environment variables to uniquly identify the controller beyond
> its device name.
> 
> This will be useful to address discovery log change events by
> actually verifying that the discovery controller is indeed the
> same as the device that generated the event.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fccf28d77c03..7a1df95ec840 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3630,6 +3630,33 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
>  
> +static int nvme_class_uevent(struct device *d, struct kobj_uevent_env *env)
> +{
> +	struct nvme_ctrl *ctrl =
> +		container_of(d, struct nvme_ctrl, ctrl_device);
> +	struct nvmf_ctrl_options *opts = ctrl->opts;
> +	int ret;
> +
> +	ret = add_uevent_var(env, "NVME_TRTYPE=%s", ctrl->ops->name);
> +	if (ret)
> +		return ret;
> +
> +	if (opts) {
> +		ret = add_uevent_var(env, "NVME_TRADDR=%s", opts->traddr);
> +		if (ret)
> +			return ret;
> +
> +		ret = add_uevent_var(env, "NVME_TRSVCID=%s",
> +			(opts && opts->trsvcid) ? opts->trsvcid : "none");
> +		if (ret)
> +			return ret;
> +
> +		ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
> +			(opts && opts->host_traddr) ? opts->host_traddr : "none");
> +	}
> +	return ret;
> +}
> +
>  static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>  {
>  	char *envp[2] = { NULL, NULL };
> @@ -4063,6 +4090,7 @@ static int __init nvme_core_init(void)
>  		result = PTR_ERR(nvme_class);
>  		goto unregister_chrdev;
>  	}
> +	nvme_class->dev_uevent = nvme_class_uevent,
>  
>  	nvme_subsys_class = class_create(THIS_MODULE, "nvme-subsystem");
>  	if (IS_ERR(nvme_subsys_class)) {
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 4/4] nvme: send discovery log page change events to userspace

[Sagi Grimberg]

If the controller supports discovery log page change events,
we want to enable it. When we see a discovery log change event
we will send it up to userspace and expect it to handle it.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
Reviewed-by: James Smart <james.smart@broadcom.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7a1df95ec840..b1ff49ffde25 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1186,7 +1186,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)
 {
@@ -3763,6 +3764,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:
+		ctrl->aen_result = result;
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/4] nvme: send discovery log page change events to userspace

[Christoph Hellwig]

> -	(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)

Please fix the > 80 char line here.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/4] nvme: send discovery log page change events to userspace

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 09/04/2019 03:00 PM, Sagi Grimberg wrote:
> If the controller supports discovery log page change events,
> we want to enable it. When we see a discovery log change event
> we will send it up to userspace and expect it to handle it.
>
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: James Smart <james.smart@broadcom.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7a1df95ec840..b1ff49ffde25 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1186,7 +1186,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)
>   {
> @@ -3763,6 +3764,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:
> +		ctrl->aen_result = result;
> +		break;
>   	default:
>   		dev_warn(ctrl->device, "async event result %08x\n", result);
>   	}
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 5/4 nvme-cli] udev: convert the discovery event handler to the kernel support

[Sagi Grimberg]

The kernel will not send us a specific event for discovery but
rather the AEN result code. So expect NVME_AEN=0x70f002 for
discovery log change events.

Also, we don't get the NVME_CTRL_NAME env var anymore as this is
available from the device $kernel.

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

diff --git a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules
index c909fb036d54..cbbebc56ea80 100644
--- a/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules
+++ b/nvmf-autoconnect/udev-rules/70-nvmf-autoconnect.rules
@@ -6,10 +6,12 @@
 #
 
 # Events from persistent discovery controllers or nvme-fc transport events
-ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_EVENT}=="discovery",\
-  ENV{NVME_CTRL_NAME}=="*", ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
+# NVME_AEN:
+#   type 0x2 (NOTICE) info 0xf0 (DISCOVERY_LOG_CHANGE) log-page-id 0x70 (DISCOVERY_LOG_PAGE)
+ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="0x70f002",\
+  ENV{NVME_TRTYPE}=="*", ENV{NVME_TRADDR}=="*", \
   ENV{NVME_TRSVCID}=="*", ENV{NVME_HOST_TRADDR}=="*", \
-  RUN+="/bin/systemctl --no-block start nvmf-connect@--device=$env{NVME_CTRL_NAME}\t--transport=$env{NVME_TRTYPE}\t--traddr=$env{NVME_TRADDR}\t--trsvcid=$env{NVME_TRSVCID}\t--host-traddr=$env{NVME_HOST_TRADDR}.service"
+  RUN+="/bin/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"
 
 # nvme-fc transport generated events (old-style for compatibility)
 ACTION=="change", SUBSYSTEM=="fc", ENV{FC_EVENT}=="nvmediscovery", \
-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme