Skip to contentSkip to conversation

Add controllers to nvmet configfs

PATCH New Israel Rukshin 3 participants 14 messages Nov 13, 2017 – Dec 4, 2017

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

Israel Rukshin

author
#0 lore raw

v2 cover Add controllers to nvmet configfs

This patch series expands nvmet configfs.
It adds to nvmet two main features
- Close an active connection from target side.
- Show all connections from the target point of view.

Those functionalities have some benefits:
- Give the administrator more control and information
- Enforce new ACL while there are active connections
- Disconnect from unreachable Hosts

Changes from v1:
- Split commit "Fix error flow in nvmet_alloc_ctrl()" to two
- Embed some port info (traddr and trsvcid) in nvmet_ctrl struct

Israel Rukshin (4):
nvmet: Fix error flow in nvmet_alloc_ctrl()
nvmet: Rearrange nvmet_ctrl_free()
nvmet: Add controllers to configfs
nvmet: Add feature close connection from target side

 drivers/nvme/target/configfs.c | 86 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     | 19 ++++++++--
 drivers/nvme/target/nvmet.h    | 12 ++++++
 3 files changed, 113 insertions(+), 4 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v2 1/4 nvmet: Fix error flow in nvmet_alloc_ctrl()

Remove the allocated id on error.

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvme/target/core.c +3 -1
index 22a2a2b..5699c9a 100644
@@ -821,7 +821,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		/* Don't accept keep-alive timeout for discovery controllers */
 		if (kato) {
 			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-			goto out_free_sqs;
+			goto out_remove_ida;
 		}
 
 		/*

@@ -851,6 +851,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	*ctrlp = ctrl;
 	return 0;
 
+out_remove_ida:
+	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 out_free_sqs:
 	kfree(ctrl->sqs);
 out_free_cqs:
Signature · 1 line
1.8.3.1
maintainer Applied
#2 lore raw

PATCH 1/4 nvmet: Fix error flow in nvmet_alloc_ctrl()

Thanks,

applied to nvme-4.16.

author
#3 lore raw

PATCH v2 2/4 nvmet: Rearrange nvmet_ctrl_free()

In reply to Israel Rukshin

Make it symmetric to nvmet_alloc_ctrl().

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/nvme/target/core.c +4 -3
index 5699c9a..995976e 100644
@@ -870,21 +870,22 @@ static void nvmet_ctrl_free(struct kref *ref)
 	struct nvmet_ctrl *ctrl = container_of(ref, struct nvmet_ctrl, ref);
 	struct nvmet_subsys *subsys = ctrl->subsys;
 
-	nvmet_stop_keep_alive_timer(ctrl);
-
 	mutex_lock(&subsys->lock);
 	list_del(&ctrl->subsys_entry);
 	mutex_unlock(&subsys->lock);
 
+	nvmet_stop_keep_alive_timer(ctrl);
+
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fatal_err_work);
 
 	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
-	nvmet_subsys_put(subsys);
 
 	kfree(ctrl->sqs);
 	kfree(ctrl->cqs);
 	kfree(ctrl);
+
+	nvmet_subsys_put(subsys);
 }
 
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl)
Signature · 1 line
1.8.3.1
maintainer Applied
#4 lore raw

PATCH 2/4 nvmet: Rearrange nvmet_ctrl_free()

Thanks,

applied to nvme-4.16.

author
#5 lore raw

PATCH v2 3/4 nvmet: Add controllers to configfs

In reply to Israel Rukshin

The commit show all the controllers and some info about them. This will allow the user to monitor the created controllers from target point of view.
The "controllers" folder was added per subsystem under:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/

<CTRL_ID> folder consists of:
- hostnqn: Host NQN
- port_traddr: Port Transport Address
- port_trsvcid: Port Transport Service ID

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/configfs.c | 72 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  8 +++++
 drivers/nvme/target/nvmet.h    | 12 +++++++
 3 files changed, 92 insertions(+)
drivers/nvme/target/configfs.c +72 -0
index b6aeb1d..2413df6 100644
@@ -469,6 +469,49 @@ static struct config_group *nvmet_ns_make(struct config_group *group,
 	.ct_owner		= THIS_MODULE,
 };
 
+static ssize_t nvmet_ctrl_hostnqn_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->hostnqn);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, hostnqn);
+
+static ssize_t nvmet_ctrl_traddr_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->traddr);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, traddr);
+
+static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->trsvcid);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
+
+static struct configfs_attribute *nvmet_ctrl_attrs[] = {
+	&nvmet_ctrl_attr_hostnqn,
+	&nvmet_ctrl_attr_traddr,
+	&nvmet_ctrl_attr_trsvcid,
+	NULL,
+};
+
+static struct config_item_type nvmet_ctrl_type = {
+	.ct_attrs		= nvmet_ctrl_attrs,
+	.ct_owner		= THIS_MODULE,
+};
+
+static struct config_item_type nvmet_controllers_type = {
+	.ct_owner		= THIS_MODULE,
+};
+
 static int nvmet_port_subsys_allow_link(struct config_item *parent,
 		struct config_item *target)
 {

@@ -760,6 +803,10 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 	configfs_add_default_group(&subsys->allowed_hosts_group,
 			&subsys->group);
 
+	config_group_init_type_name(&subsys->controllers_group,
+			"controllers", &nvmet_controllers_type);
+	configfs_add_default_group(&subsys->controllers_group, &subsys->group);
+
 	return &subsys->group;
 }
 

@@ -983,6 +1030,31 @@ static struct config_group *nvmet_hosts_make_group(struct config_group *group,
 	},
 };
 
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
+{
+	if (d_inode(ctrl->group.cg_item.ci_dentry))
+		configfs_unregister_group(&ctrl->group);
+}
+
+int nvmet_ctrl_configfs_create(struct nvmet_ctrl *ctrl)
+{
+	int res = 0;
+	char name[CONFIGFS_ITEM_NAME_LEN];
+
+	sprintf(name, "%d", ctrl->cntlid);
+	pr_debug("Adding controller %s to configfs\n", name);
+
+	config_group_init_type_name(&ctrl->group, name, &nvmet_ctrl_type);
+
+	res = configfs_register_group(&ctrl->subsys->controllers_group,
+				      &ctrl->group);
+	if (res)
+		pr_err("failed to register configfs group for controller %s\n",
+		       name);
+
+	return res;
+}
+
 int __init nvmet_init_configfs(void)
 {
 	int ret;
drivers/nvme/target/core.c +8 -0
index 995976e..631c0c5 100644
@@ -842,6 +842,12 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		/* keep-alive timeout in seconds */
 		ctrl->kato = DIV_ROUND_UP(kato, 1000);
 	}
+	memcpy(ctrl->traddr, req->port->disc_addr.traddr, NVMF_TRADDR_SIZE);
+	memcpy(ctrl->trsvcid, req->port->disc_addr.trsvcid, NVMF_TRSVCID_SIZE);
+	ret = nvmet_ctrl_configfs_create(ctrl);
+	if (ret)
+		goto out_remove_ida;
+
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);

@@ -876,6 +882,8 @@ static void nvmet_ctrl_free(struct kref *ref)
 
 	nvmet_stop_keep_alive_timer(ctrl);
 
+	nvmet_ctrl_configfs_del(ctrl);
+
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fatal_err_work);
drivers/nvme/target/nvmet.h +12 -0
index 194ebff..c8fa32d 100644
@@ -134,8 +134,17 @@ struct nvmet_ctrl {
 
 	char			subsysnqn[NVMF_NQN_FIELD_LEN];
 	char			hostnqn[NVMF_NQN_FIELD_LEN];
+	char			traddr[NVMF_TRADDR_SIZE];
+	char			trsvcid[NVMF_TRSVCID_SIZE];
+
+	struct config_group	group;
 };
 
+static inline struct nvmet_ctrl *to_nvmet_ctrl(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct nvmet_ctrl, group);
+}
+
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 

@@ -160,6 +169,7 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+	struct config_group	controllers_group;
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)

@@ -288,6 +298,8 @@ u16 nvmet_ctrl_find_get(const char *subsysnqn, const char *hostnqn, u16 cntlid,
 		struct nvmet_req *req, struct nvmet_ctrl **ret);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd);
+int nvmet_ctrl_configfs_create(struct nvmet_ctrl *ctrl);
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl);
 
 struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		enum nvme_subsys_type type);
Signature · 1 line
1.8.3.1
maintainer
#6 lore raw

PATCH 3/4 nvmet: Add controllers to configfs

drivers/nvme/target/configfs.c static struct config_group *nvmet_hosts_make_group(struct config_group *group, · line 1036

Quoted 5 lines
drivers/nvme/target/configfs.c +4 -0
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
+{
+	if (d_inode(ctrl->group.cg_item.ci_dentry))
+		configfs_unregister_group(&ctrl->group);

I'm not sure I understand the conditional here, care to explain?

author
#7 lore raw

PATCH v2 3/4 nvmet: Add controllers to configfs

Quoted 7 lines · Sagi Grimberg

drivers/nvme/target/configfs.c static struct config_group *nvmet_hosts_make_group(struct config_group *group, · line 1036

Quoted 5 lines
drivers/nvme/target/configfs.c +4 -0
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
+{
+    if (d_inode(ctrl->group.cg_item.ci_dentry))
+        configfs_unregister_group(&ctrl->group);

I'm not sure I understand the conditional here, care to explain?

Since I created the controller folder with configfs_register_group(), it creates this folder as a default folder.
Default folders are removed when their parent folder is removed.

The new filesystem hierarchy:
<SUBSYSTEM>/controllers/<CTRL_ID>/

Both controllers and controller (CTRL_ID) are default folders. This condition protect us from a case that the user remove the subsystem folder with active connections.
When subsystem folder is removed by the user it first remove the controllers folder
and then it removes all the controller (CTRL_ID) folders. In this case there is no need to remove again the controller (CTRL_ID) folders when
the actual controller object is going to be freed.

This check simply asks if controller (CTRL_ID) folders was already removed.

maintainer
#8 lore raw

PATCH 3/4 nvmet: Add controllers to configfs

In reply to Israel Rukshin
Quoted 8 lines · Israel Rukshin

The commit show all the controllers and some info about them. This will allow the user to monitor the created controllers from target point of view.
The "controllers" folder was added per subsystem under:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/

<CTRL_ID> folder consists of:
- hostnqn: Host NQN

This makes sense.

Quoted 2 lines

- port_traddr: Port Transport Address
- port_trsvcid: Port Transport Service ID

These are port properties. You probably want to create a symlink to the port from the controller directory instead.

drivers/nvme/target/configfs.c static struct config_group *nvmet_hosts_make_group(struct config_group *group, · line 1037

Quoted 5 lines
drivers/nvme/target/configfs.c +5 -0
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
+{
+	if (d_inode(ctrl->group.cg_item.ci_dentry))
+		configfs_unregister_group(&ctrl->group);
+}

Are we even guaranteed that the denty is still around? What happens if you do a drop of all caches after the manual removal and then ?xecute this path later?

drivers/nvme/target/configfs.c static struct config_group *nvmet_hosts_make_group(struct config_group *group, · line 1048

Quoted 8 lines
drivers/nvme/target/configfs.c +8 -0
+	int res = 0;
+	char name[CONFIGFS_ITEM_NAME_LEN];
+
+	sprintf(name, "%d", ctrl->cntlid);
+	pr_debug("Adding controller %s to configfs\n", name);
+
+	config_group_init_type_name(&ctrl->group, name, &nvmet_ctrl_type);
+

Hmm - we probably should either add varargs to config_group_init_type_name or just open code it. config_item_set_name takes varargs, so having this static buffer is a little silly. I'll take a look at the configfs side.

author
#9 lore raw

PATCH v2 4/4 nvmet: Add feature close connection from target side

In reply to Israel Rukshin

This allows the user to close any connection from target side by writing to the file "force_close" at the controller folder. Full path:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/configfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/nvme/target/configfs.c +13 -0
index 2413df6..90e5925 100644
@@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
 
 CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
 
+static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	ctrl->ops->delete_ctrl(ctrl);
+
+	return count;
+}
+
+CONFIGFS_ATTR_WO(nvmet_ctrl_, force_close);
+
 static struct configfs_attribute *nvmet_ctrl_attrs[] = {
 	&nvmet_ctrl_attr_hostnqn,
 	&nvmet_ctrl_attr_traddr,
 	&nvmet_ctrl_attr_trsvcid,
+	&nvmet_ctrl_attr_force_close,
 	NULL,
 };
Signature · 1 line
1.8.3.1
maintainer
#10 lore raw

PATCH 4/4 nvmet: Add feature close connection from target side

Hi Israel,

Quoted 16 lines

This allows the user to close any connection from target side by writing to the file "force_close" at the controller folder. Full path:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
  drivers/nvme/target/configfs.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
drivers/nvme/target/configfs.c +0 -0
index 2413df6..90e5925 100644

drivers/nvme/target/configfs.c static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page) · line 500

Quoted 5 lines
drivers/nvme/target/configfs.c +2 -0
@@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
 
  CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
 
+static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
+		const char *page, size_t count)

Maybe simply 'delete' is a better attribute name?

drivers/nvme/target/configfs.c static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page) · line 505

Quoted 5 lines
drivers/nvme/target/configfs.c +5 -0
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	ctrl->ops->delete_ctrl(ctrl);
+

What guarantees that calling delete_ctrl is safe here? Controllers can be freed under various conditions (client disconnect, fatal_error, device removal). What protects from user initiated force_close hitting a use-after-free condition?

author
#11 lore raw

PATCH v2 4/4 nvmet: Add feature close connection from target side

Hi Sagi,

Quoted 14 lines · Sagi Grimberg

Hi Israel,

Quoted 12 lines

This allows the user to close any connection from target side by writing to the file "force_close" at the controller folder. Full path:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close

Signed-off-by Israel Rukshin <israelr at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
  drivers/nvme/target/configfs.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
Quoted 12 lines

drivers/nvme/target/configfs.c static ssize_t nvmet_ctrl_trsvcid_show(struct · line 499

Quoted 10 lines
drivers/nvme/target/configfs.c +1 -0
index 2413df6..90e5925 100644
@@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct 
 config_item *item, char *page)
    CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
  +static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
+        const char *page, size_t count)

Maybe simply 'delete' is a better attribute name?

You probably right.

Quoted 11 lines

drivers/nvme/target/configfs.c static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page) · line 505

Quoted 6 lines
drivers/nvme/target/configfs.c +5 -0
+{
+    struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+    ctrl->ops->delete_ctrl(ctrl);
+

What guarantees that calling delete_ctrl is safe here? Controllers can be freed under various conditions (client disconnect, fatal_error, device removal). What protects from user initiated force_close hitting a use-after-free condition?

The fact that I call nvmet_ctrl_configfs_del() at nvmet_ctrl_free() before freeing the controller guarantees
that the user will not see the controller at the filesystem after this point.
The second thing is that configfs_unregister_group() should wait for all the user calls on the controller to finish.
I don't see a difference between nvmet_ctrl_trsvcid_show() and nvmet_ctrl_force_close_store() (both are synchronous functions).

Regarding the races that you mentioned with client disconnect, fatal_error ...
__nvmet_rdma_queue_disconnect() is protected with a state lock.

maintainer
#12 lore raw

PATCH 4/4 nvmet: Add feature close connection from target side

Quoted 15 lines
Quoted 5 lines

What guarantees that calling delete_ctrl is safe here? Controllers can be freed under various conditions (client disconnect, fatal_error, device removal). What protects from user initiated force_close hitting a use-after-free condition?

The fact that I call nvmet_ctrl_configfs_del() at nvmet_ctrl_free() before freeing the controller guarantees
that the user will not see the controller at the filesystem after this point.
The second thing is that configfs_unregister_group() should wait for all the user calls on the controller to finish.
I don't see a difference between nvmet_ctrl_trsvcid_show() and nvmet_ctrl_force_close_store() (both are synchronous functions).

Quoted 3 lines

Regarding the races that you mentioned with client disconnect, fatal_error ...
__nvmet_rdma_queue_disconnect() is protected with a state lock.

The point is that ->delete_ctrl() is not designed to be safe for calling it multiple times (although it might be the case for rdma) and the code assumes that (which is wrong).

You need to call it conditionally. See what is done in nvmet_ctrl_fatal_error()

maintainer
#13 lore raw

PATCH 4/4 nvmet: Add feature close connection from target side

Quoted 6 lines

The point is that ->delete_ctrl() is not designed to be safe for calling it multiple times (although it might be the case for rdma) and the code assumes that (which is wrong).

You need to call it conditionally. See what is done in nvmet_ctrl_fatal_error()

Yes. In fact he should probably just call nvmet_ctrl_fatal_error.