[PATCH 0/3] some useful fabrics patches

[PATCH 0/3] some useful fabrics patches

[Sagi Grimberg]

No bug fixes, but useful patches I had in the pipe...

Sagi Grimberg (3):
  nvmet: delete controllers deletion upon subsystem release
  nvmet: Make cntlid globally unique
  nvme: Make controller state visible via sysfs

 drivers/nvme/host/core.c          | 21 +++++++++++++++++++++
 drivers/nvme/target/configfs.c    |  1 +
 drivers/nvme/target/core.c        | 20 ++++++++++++++------
 drivers/nvme/target/fabrics-cmd.c |  4 ++--
 drivers/nvme/target/nvmet.h       |  2 +-
 5 files changed, 39 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Sagi Grimberg]

No reason for them to be kept around if we are
deleting the subsystem, so instead of passively
wait for the host to disconnect, actively delete
the controllers.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/configfs.c |  1 +
 drivers/nvme/target/core.c     | 10 ++++++++++
 drivers/nvme/target/nvmet.h    |  1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index d0f60c36d576..634a2d780e16 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -633,6 +633,7 @@ static void nvmet_subsys_release(struct config_item *item)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
 
+	nvmet_subsys_del_ctrls(subsys);
 	nvmet_subsys_put(subsys);
 }
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b1d66ed655c9..4a367549eb93 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -935,6 +935,16 @@ static void nvmet_subsys_free(struct kref *ref)
 	kfree(subsys);
 }
 
+void nvmet_subsys_del_ctrls(struct nvmet_subsys *subsys)
+{
+	struct nvmet_ctrl *ctrl;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+		ctrl->ops->delete_ctrl(ctrl);
+	mutex_unlock(&subsys->lock);
+}
+
 void nvmet_subsys_put(struct nvmet_subsys *subsys)
 {
 	kref_put(&subsys->ref, nvmet_subsys_free);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 23d5eb1c944f..cc7ad06b43a7 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -282,6 +282,7 @@ void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		enum nvme_subsys_type type);
 void nvmet_subsys_put(struct nvmet_subsys *subsys);
+void nvmet_subsys_del_ctrls(struct nvmet_subsys *subsys);
 
 struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid);
 void nvmet_put_namespace(struct nvmet_ns *ns);
-- 
2.7.4

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Johannes Thumshirn]

On Mon, Dec 05, 2016@01:09:04PM +0200, Sagi Grimberg wrote:
> No reason for them to be kept around if we are
> deleting the subsystem, so instead of passively
> wait for the host to disconnect, actively delete
> the controllers.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Max Gurtovoy]

On 12/5/2016 1:39 PM, Johannes Thumshirn wrote:
> On Mon, Dec 05, 2016@01:09:04PM +0200, Sagi Grimberg wrote:
>> No reason for them to be kept around if we are
>> deleting the subsystem, so instead of passively
>> wait for the host to disconnect, actively delete
>> the controllers.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
>
> Looks good,
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
>

Looks fine,
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Christoph Hellwig]

On Mon, Dec 05, 2016@01:09:04PM +0200, Sagi Grimberg wrote:
> No reason for them to be kept around if we are
> deleting the subsystem, so instead of passively
> wait for the host to disconnect, actively delete
> the controllers.

Looks fine, but I'd really love to see a testcase for this in some form..

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Sagi Grimberg]

>> No reason for them to be kept around if we are
>> deleting the subsystem, so instead of passively
>> wait for the host to disconnect, actively delete
>> the controllers.
>
> Looks fine, but I'd really love to see a testcase for this in some form..

Should we revive the selftests branch?

>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
>

[PATCH 1/3] nvmet: delete controllers deletion upon subsystem release

[Christoph Hellwig]

On Mon, Dec 05, 2016@07:08:03PM +0200, Sagi Grimberg wrote:
>
>>> No reason for them to be kept around if we are
>>> deleting the subsystem, so instead of passively
>>> wait for the host to disconnect, actively delete
>>> the controllers.
>>
>> Looks fine, but I'd really love to see a testcase for this in some form..
>
> Should we revive the selftests branch?

As-is it's not very usable and would need some major rework.  At some
point Chaitanya mentioned he wanted to look into it, but I suspect
he is busy with other things at the moment.

Personally I think we should use nvme-cli and nvmetcli for our tests
instead of opencoding everything, and life would probably be easier
if we wrote it in a proper test framework similar to the nvme-cli
and nvmetcli tests.

[PATCH 2/3] nvmet: Make cntlid globally unique

[Sagi Grimberg]

We usually log the cntlid which is confusing in case
we have multiple subsystems each with it's own cntlid ida.
Instead make cntlid ida globally unique and log the initial
association.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/core.c        | 10 ++++------
 drivers/nvme/target/fabrics-cmd.c |  4 ++--
 drivers/nvme/target/nvmet.h       |  1 -
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4a367549eb93..cee26ddc47bb 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -18,6 +18,7 @@
 
 static struct nvmet_fabrics_ops *nvmet_transports[NVMF_TRTYPE_MAX];
 
+static DEFINE_IDA(cntlid_ida);
 /*
  * This read/write semaphore is used to synchronize access to configuration
  * information on a target system that will result in discovery log page
@@ -749,7 +750,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	if (!ctrl->sqs)
 		goto out_free_cqs;
 
-	ret = ida_simple_get(&subsys->cntlid_ida,
+	ret = ida_simple_get(&cntlid_ida,
 			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
 			     GFP_KERNEL);
 	if (ret < 0) {
@@ -816,7 +817,7 @@ static void nvmet_ctrl_free(struct kref *ref)
 	list_del(&ctrl->subsys_entry);
 	mutex_unlock(&subsys->lock);
 
-	ida_simple_remove(&subsys->cntlid_ida, ctrl->cntlid);
+	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 	nvmet_subsys_put(subsys);
 
 	kfree(ctrl->sqs);
@@ -915,9 +916,6 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	mutex_init(&subsys->lock);
 	INIT_LIST_HEAD(&subsys->namespaces);
 	INIT_LIST_HEAD(&subsys->ctrls);
-
-	ida_init(&subsys->cntlid_ida);
-
 	INIT_LIST_HEAD(&subsys->hosts);
 
 	return subsys;
@@ -930,7 +928,6 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
-	ida_destroy(&subsys->cntlid_ida);
 	kfree(subsys->subsysnqn);
 	kfree(subsys);
 }
@@ -973,6 +970,7 @@ static void __exit nvmet_exit(void)
 {
 	nvmet_exit_configfs();
 	nvmet_exit_discovery();
+	ida_destroy(&cntlid_ida);
 
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_entry) != 1024);
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_hdr) != 1024);
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index f4088198cd0d..18dc314601c3 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -153,8 +153,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 		goto out;
 	}
 
-	pr_info("creating controller %d for NQN %s.\n",
-			ctrl->cntlid, ctrl->hostnqn);
+	pr_info("creating controller %d for subsystem %s for NQN %s.\n",
+		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);
 	req->rsp->result.u16 = cpu_to_le16(ctrl->cntlid);
 
 out:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cc7ad06b43a7..1370eee0a3c0 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -142,7 +142,6 @@ struct nvmet_subsys {
 	unsigned int		max_nsid;
 
 	struct list_head	ctrls;
-	struct ida		cntlid_ida;
 
 	struct list_head	hosts;
 	bool			allow_any_host;
-- 
2.7.4

[PATCH 2/3] nvmet: Make cntlid globally unique

[Johannes Thumshirn]

On Mon, Dec 05, 2016@01:09:05PM +0200, Sagi Grimberg wrote:
> We usually log the cntlid which is confusing in case
> we have multiple subsystems each with it's own cntlid ida.
> Instead make cntlid ida globally unique and log the initial
> association.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/3] nvmet: Make cntlid globally unique

[Max Gurtovoy]

On 12/5/2016 1:38 PM, Johannes Thumshirn wrote:
> On Mon, Dec 05, 2016@01:09:05PM +0200, Sagi Grimberg wrote:
>> We usually log the cntlid which is confusing in case
>> we have multiple subsystems each with it's own cntlid ida.
>> Instead make cntlid ida globally unique and log the initial
>> association.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
>
> Looks good,
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
>

Looks fine,
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 2/3] nvmet: Make cntlid globally unique

[Christoph Hellwig]

On Mon, Dec 05, 2016@01:09:05PM +0200, Sagi Grimberg wrote:
> We usually log the cntlid which is confusing in case
> we have multiple subsystems each with it's own cntlid ida.
> Instead make cntlid ida globally unique and log the initial
> association.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/core.c        | 10 ++++------
>  drivers/nvme/target/fabrics-cmd.c |  4 ++--
>  drivers/nvme/target/nvmet.h       |  1 -
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 4a367549eb93..cee26ddc47bb 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -18,6 +18,7 @@
>  
>  static struct nvmet_fabrics_ops *nvmet_transports[NVMF_TRTYPE_MAX];
>  
> +static DEFINE_IDA(cntlid_ida);
>  /*

How about moving the empty line after this declaration instead of before?

> +	pr_info("creating controller %d for subsystem %s for NQN %s.\n",
> +		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);

That's going to be a crazy long line in the dmesg log..

Otherwise looks fine:


Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 2/3] nvmet: Make cntlid globally unique

[Sagi Grimberg]

>> +	pr_info("creating controller %d for subsystem %s for NQN %s.\n",
>> +		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);
>
> That's going to be a crazy long line in the dmesg log..

Have a better idea how to provide this information?

>
> Otherwise looks fine:
>
>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
>

[PATCH 2/3] nvmet: Make cntlid globally unique

[Christoph Hellwig]

On Mon, Dec 05, 2016@07:06:44PM +0200, Sagi Grimberg wrote:
>
>>> +	pr_info("creating controller %d for subsystem %s for NQN %s.\n",
>>> +		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);
>>
>> That's going to be a crazy long line in the dmesg log..
>
> Have a better idea how to provide this information?

Not sure.  Maybe multiple lines?

[PATCH 2/3] nvmet: Make cntlid globally unique

[J Freyensee]

On Tue, 2016-12-06@09:28 +0100, Christoph Hellwig wrote:
> On Mon, Dec 05, 2016@07:06:44PM +0200, Sagi Grimberg wrote:
> > 
> > 
> > > 
> > > > 
> > > > +	pr_info("creating controller %d for subsystem %s for
> > > > NQN %s.\n",
> > > > +		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl-
> > > > >hostnqn);
> > > 
> > > That's going to be a crazy long line in the dmesg log..
> > 
> > Have a better idea how to provide this information?
> 
> Not sure.??Maybe multiple lines?


pr_info("Creating controller: %d\n ? in subsystem: %s\n ? for host:
%s\n",
? ? ? ? ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);

????

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Sagi Grimberg]

Easier for debugging and testing state machine
transitions.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a37cb215652b..c5b8f7b56042 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1569,6 +1569,26 @@ static ssize_t nvme_sysfs_show_transport(struct device *dev,
 }
 static DEVICE_ATTR(transport, S_IRUGO, nvme_sysfs_show_transport, NULL);
 
+static ssize_t nvme_sysfs_show_state(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	static const char *const state_name[] = {
+		[NVME_CTRL_NEW]		= "new",
+		[NVME_CTRL_LIVE]	= "live",
+		[NVME_CTRL_RESETTING]	= "resetting",
+		[NVME_CTRL_RECONNECTING]= "reconnecting",
+		[NVME_CTRL_DELETING]	= "deleting",
+		[NVME_CTRL_DEAD]	= "dead",
+	};
+
+	return sprintf(buf, "%s\n",
+		       (unsigned)ctrl->state < ARRAY_SIZE(state_name) ?
+		       state_name[ctrl->state] : "???");
+}
+static DEVICE_ATTR(state, S_IRUGO, nvme_sysfs_show_state, NULL);
+
 static ssize_t nvme_sysfs_show_subsysnqn(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
@@ -1601,6 +1621,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_transport.attr,
 	&dev_attr_subsysnqn.attr,
 	&dev_attr_address.attr,
+	&dev_attr_state.attr,
 	NULL
 };
 
-- 
2.7.4

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Johannes Thumshirn]

On Mon, Dec 05, 2016@01:09:06PM +0200, Sagi Grimberg wrote:
> Easier for debugging and testing state machine
> transitions.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Max Gurtovoy]

Sagi,
what about locking the "ctrl->lock" and making it more up-to-date ?

On 12/5/2016 1:09 PM, Sagi Grimberg wrote:
> Easier for debugging and testing state machine
> transitions.
>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a37cb215652b..c5b8f7b56042 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1569,6 +1569,26 @@ static ssize_t nvme_sysfs_show_transport(struct device *dev,
>  }
>  static DEVICE_ATTR(transport, S_IRUGO, nvme_sysfs_show_transport, NULL);
>
> +static ssize_t nvme_sysfs_show_state(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	static const char *const state_name[] = {
> +		[NVME_CTRL_NEW]		= "new",
> +		[NVME_CTRL_LIVE]	= "live",
> +		[NVME_CTRL_RESETTING]	= "resetting",
> +		[NVME_CTRL_RECONNECTING]= "reconnecting",
> +		[NVME_CTRL_DELETING]	= "deleting",
> +		[NVME_CTRL_DEAD]	= "dead",
> +	};
> +
> +	return sprintf(buf, "%s\n",
> +		       (unsigned)ctrl->state < ARRAY_SIZE(state_name) ?
> +		       state_name[ctrl->state] : "???");
> +}
> +static DEVICE_ATTR(state, S_IRUGO, nvme_sysfs_show_state, NULL);
> +
>  static ssize_t nvme_sysfs_show_subsysnqn(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
> @@ -1601,6 +1621,7 @@ static struct attribute *nvme_dev_attrs[] = {
>  	&dev_attr_transport.attr,
>  	&dev_attr_subsysnqn.attr,
>  	&dev_attr_address.attr,
> +	&dev_attr_state.attr,
>  	NULL
>  };
>
>

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Sagi Grimberg]

> Sagi,
> what about locking the "ctrl->lock" and making it more up-to-date ?

Don't see why we should do that. What are you trying to protect against?

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Christoph Hellwig]

> +	return sprintf(buf, "%s\n",
> +		       (unsigned)ctrl->state < ARRAY_SIZE(state_name) ?
> +		       state_name[ctrl->state] : "???");

I'd use an if here instead of the ? : and print unknown instead of the ???

Otherwise this looks fine to me:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 3/3] nvme: Make controller state visible via sysfs

[Sagi Grimberg]

>> +	return sprintf(buf, "%s\n",
>> +		       (unsigned)ctrl->state < ARRAY_SIZE(state_name) ?
>> +		       state_name[ctrl->state] : "???");
>
> I'd use an if here instead of the ? : and print unknown instead of the ???

I can change that...