Skip to contentSkip to conversation

some useful fabrics patches

PATCH New Sagi Grimberg 5 participants 20 messages Dec 5, 2016 – Dec 6, 2016

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

cover some useful fabrics patches

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(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

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

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(+)
drivers/nvme/target/configfs.c +1 -0
index d0f60c36d576..634a2d780e16 100644
@@ -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);
 }
drivers/nvme/target/core.c +10 -0
index b1d66ed655c9..4a367549eb93 100644
@@ -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);
drivers/nvme/target/nvmet.h +1 -0
index 23d5eb1c944f..cc7ad06b43a7 100644
@@ -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);
Signature · 1 line
2.7.4
#2 lore raw

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

Quoted 7 lines · 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>

---

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

Signature · 6 lines
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
#3 lore raw

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

Quoted 6 lines · Johannes Thumshirn
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 2 lines
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>

maintainer
#4 lore raw

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

In reply to Sagi Grimberg
Quoted 4 lines · 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..

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#5 lore raw

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

Quoted 7 lines
Quoted 5 lines

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?

Quoted 3 lines
Reviewed-by Christoph Hellwig <hch at lst.de>
maintainer
#6 lore raw

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

Quoted 9 lines · Sagi Grimberg
Quoted 7 lines
Quoted 5 lines

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.

author maintainer
#7 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

In reply to 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(-)
drivers/nvme/target/core.c +4 -6
index 4a367549eb93..cee26ddc47bb 100644
@@ -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);
drivers/nvme/target/fabrics-cmd.c +2 -2
index f4088198cd0d..18dc314601c3 100644
@@ -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:
drivers/nvme/target/nvmet.h +0 -1
index cc7ad06b43a7..1370eee0a3c0 100644
@@ -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;
Signature · 1 line
2.7.4
#8 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

Quoted 7 lines · 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>

---

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

Signature · 6 lines
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
#9 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

Quoted 6 lines · Johannes Thumshirn
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 2 lines
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>

maintainer
#10 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

In reply to Sagi Grimberg
Quoted 17 lines · 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(-)
drivers/nvme/target/core.c +0 -0
index 4a367549eb93..cee26ddc47bb 100644

drivers/nvme/target/core.c line 22

Quoted 5 lines
drivers/nvme/target/core.c +1 -0
@@ -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?

drivers/nvme/target/core.c line 24

Quoted 2 lines
drivers/nvme/target/core.c +2 -0
+	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>
author maintainer
#11 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

Quoted 5 lines

drivers/nvme/target/fabrics-cmd.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 157

Quoted 3 lines
drivers/nvme/target/fabrics-cmd.c +2 -0
+	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?

Quoted 6 lines

Otherwise looks fine:

Reviewed-by Christoph Hellwig <hch at lst.de>
maintainer
#12 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

Quoted 7 lines · Sagi Grimberg
Quoted 5 lines

drivers/nvme/target/fabrics-cmd.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 157

Quoted 3 lines
drivers/nvme/target/fabrics-cmd.c +2 -0
+	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?

#13 lore raw

PATCH 2/3 nvmet: Make cntlid globally unique

Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Sagi Grimberg
Quoted 7 lines
Quoted 7 lines
(quoted diff) +1 -0
+	pr_info("creating controller %d for subsystem %s for

NQN %s.\n",

(quoted diff) +1 -0
+		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl-
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

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

????

author maintainer
#14 lore raw

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

In reply to 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(+)
drivers/nvme/host/core.c +21 -0
index a37cb215652b..c5b8f7b56042 100644
@@ -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
 };
Signature · 1 line
2.7.4
#15 lore raw

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

Quoted 5 lines · Sagi Grimberg

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>

Signature · 6 lines
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
#16 lore raw

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

In reply to Sagi Grimberg

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

Quoted 48 lines · 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(+)
drivers/nvme/host/core.c +21 -0
index a37cb215652b..c5b8f7b56042 100644
@@ -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
 };
author maintainer
#17 lore raw

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

Quoted 3 lines

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?

maintainer
#18 lore raw

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

In reply to Sagi Grimberg

drivers/nvme/host/core.c static ssize_t nvme_sysfs_show_transport(struct device *dev, · line 1588

Quoted 3 lines
drivers/nvme/host/core.c +3 -0
+	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>
author maintainer
#19 lore raw

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

Quoted 6 lines

drivers/nvme/host/core.c static ssize_t nvme_sysfs_show_transport(struct device *dev, · line 1588

Quoted 4 lines
drivers/nvme/host/core.c +3 -0
+	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...