nvme fabrics patches

PATCH New Sagi Grimberg 2 participants 8 messages Jan 12, 2017 – Jan 13, 2017

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #0 lore raw

cover nvme fabrics patches

This is a resend of some patches I sent a while ago and just now got some time to revisit.

I added a fix to a possible race condition where we free a controller without making sure that fatal_err and/or async works are cancelled/flushed.

Sagi Grimberg (5):
nvmet: delete controllers deletion upon subsystem release
nvmet: Make cntlid globally unique
nvme: Make controller state visible via sysfs
nvmet: cancel fatal error and flush async work before free controller
nvmet: Call fatal_error from keep-alive timout expiration

 drivers/nvme/host/core.c          | 24 ++++++++++++++++++++++++
 drivers/nvme/target/configfs.c    |  1 +
 drivers/nvme/target/core.c        | 25 ++++++++++++++++++-------
 drivers/nvme/target/fabrics-cmd.c |  4 ++--
 drivers/nvme/target/nvmet.h       |  2 +-
 5 files changed, 46 insertions(+), 10 deletions(-)

Signature · 1 line

2.7.4

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #1 lore raw

PATCH 1/5 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>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Reviewed-by Max Gurtovoy <maxg at mellanox.com>

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

---
 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 6f5074153dcd..be8c800078e2 100644
@@ -631,6 +631,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

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #2 lore raw

PATCH 2/5 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>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Reviewed-by Max Gurtovoy <maxg at mellanox.com>

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

---
 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..16a1c5e85826 100644
@@ -17,6 +17,7 @@
 #include "nvmet.h"
 
 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

@@ -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

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #3 lore raw

PATCH 3/5 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>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

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

---
 drivers/nvme/host/core.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/nvme/host/core.c +24 -0

index 8a3c3e32a704..439aebf4473c 100644
@@ -1521,6 +1521,29 @@ 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",
+	};
+
+	if ((unsigned)ctrl->state < ARRAY_SIZE(state_name) &&
+	    state_name[ctrl->state])
+		return sprintf(buf, "%s\n", state_name[ctrl->state]);
+
+	return sprintf(buf, "unknown state\n");
+}
+
+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)

@@ -1553,6 +1576,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

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #4 lore raw

PATCH 4/5 nvmet: cancel fatal error and flush async work before free controller

In reply to Sagi Grimberg

Make sure they are not running and we can free the controller safely.

Signed-off-by Roy Shterman <roys at lightbitslabs.com>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/target/core.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/nvme/target/core.c +3 -0

index 16a1c5e85826..dcdc2f6146f1 100644
@@ -817,6 +817,9 @@ static void nvmet_ctrl_free(struct kref *ref)
 	list_del(&ctrl->subsys_entry);
 	mutex_unlock(&subsys->lock);
 
+	flush_work(&ctrl->async_event_work);
+	cancel_work_sync(&ctrl->fatal_err_work);
+
 	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 	nvmet_subsys_put(subsys);

Signature · 1 line

2.7.4

Christoph Hellwig

maintainer

2017-01-13 07:21 UTC #5 lore raw

PATCH 4/5 nvmet: cancel fatal error and flush async work before free controller

Quoted 5 lines · Sagi Grimberg

Make sure they are not running and we can free the controller safely.
Signed-off-by Roy Shterman <roys at lightbitslabs.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks good,

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

Sagi Grimberg

author maintainer

2017-01-12 10:31 UTC #6 lore raw

PATCH 5/5 nvmet: Call fatal_error from keep-alive timout expiration

In reply to Sagi Grimberg

We only need to call delete_ctrl once, so given that both keep-alive timeout and any other fatal error can trigger it, just make sure we only call delete_ctrl once.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/target/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/nvme/target/core.c +1 -1

index dcdc2f6146f1..5267ce20c12d 100644
@@ -201,7 +201,7 @@ static void nvmet_keep_alive_timer(struct work_struct *work)
 	pr_err("ctrl %d keep-alive timer (%d seconds) expired!\n",
 		ctrl->cntlid, ctrl->kato);
 
-	ctrl->ops->delete_ctrl(ctrl);
+	nvmet_ctrl_fatal_error(ctrl);
 }
 
 static void nvmet_start_keep_alive_timer(struct nvmet_ctrl *ctrl)

Signature · 1 line

2.7.4

Christoph Hellwig

maintainer

2017-01-13 07:22 UTC #7 lore raw

PATCH 5/5 nvmet: Call fatal_error from keep-alive timout expiration

Quoted 14 lines · Sagi Grimberg

We only need to call delete_ctrl once, so given that both keep-alive timeout and any other fatal error can trigger it, just make sure we only call delete_ctrl once.
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/target/core.c +0 -0
index dcdc2f6146f1..5267ce20c12d 100644

drivers/nvme/target/core.c static void nvmet_keep_alive_timer(struct work_struct *work) · line 204

Quoted 5 lines

drivers/nvme/target/core.c +1 -1

@@ -201,7 +201,7 @@ static void nvmet_keep_alive_timer(struct work_struct *work)
 	pr_err("ctrl %d keep-alive timer (%d seconds) expired!\n",
 		ctrl->cntlid, ctrl->kato);
 
-	ctrl->ops->delete_ctrl(ctrl);
+	nvmet_ctrl_fatal_error(ctrl);

Looks good,

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