[PATCH 0/4] Add controllers to nvmet configfs

[PATCH 0/4] Add controllers to nvmet configfs

[Israel Rukshin]

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

Israel Rukshin (4):
  nvmet: Add port pointer to nvmet_ctrl structure
  nvmet: Fix error flow in nvmet_alloc_ctrl()
  nvmet: Add controllers to configfs
  nvmet: Add feature close connection from target side

 drivers/nvme/target/configfs.c | 86 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     | 18 +++++++--
 drivers/nvme/target/nvmet.h    | 11 ++++++
 3 files changed, 111 insertions(+), 4 deletions(-)

-- 
1.8.4.3

[PATCH 1/4] nvmet: Add port pointer to nvmet_ctrl structure

[Israel Rukshin]

The ctrl is the "engine" of the subsystem per port, so link them
for future usage.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c  | 1 +
 drivers/nvme/target/nvmet.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 1b208be..08270c1 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -819,6 +819,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		/* keep-alive timeout in seconds */
 		ctrl->kato = DIV_ROUND_UP(kato, 1000);
 	}
+	ctrl->port = req->port;
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 7b8e20a..dc586e7 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -108,6 +108,7 @@ static inline struct nvmet_port *to_nvmet_port(struct config_item *item)
 
 struct nvmet_ctrl {
 	struct nvmet_subsys	*subsys;
+	struct nvmet_port	*port;
 	struct nvmet_cq		**cqs;
 	struct nvmet_sq		**sqs;
 
-- 
1.8.4.3

[PATCH 1/4] nvmet: Add port pointer to nvmet_ctrl structure

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 2/4] nvmet: Fix error flow in nvmet_alloc_ctrl()

[Israel Rukshin]

Also rearrange nvmet_ctrl_free().

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 08270c1..711df73 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -798,7 +798,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;
 		}
 
 		/*
@@ -829,6 +829,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:
@@ -846,21 +848,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)
-- 
1.8.4.3

[PATCH 2/4] nvmet: Fix error flow in nvmet_alloc_ctrl()

[Sagi Grimberg]

Israel,

> Also rearrange nvmet_ctrl_free().

It would be better to separate into two patches.
In the nvmet_ctrl_free() patch, please explain
why is the rearrangement needed.

[PATCH 3/4] nvmet: Add controllers to configfs

[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 | 73 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  6 ++++
 drivers/nvme/target/nvmet.h    | 10 ++++++
 3 files changed, 89 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b6aeb1d..a0942c3 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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->port->disc_addr.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->port->disc_addr.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,32 @@ 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_info("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;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 711df73..99bbf49 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -820,6 +820,10 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		ctrl->kato = DIV_ROUND_UP(kato, 1000);
 	}
 	ctrl->port = req->port;
+	ret = nvmet_ctrl_configfs_create(ctrl);
+	if (ret)
+		goto out_remove_ida;
+
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);
@@ -854,6 +858,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);
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index dc586e7..1d49873 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -135,8 +135,15 @@ struct nvmet_ctrl {
 
 	char			subsysnqn[NVMF_NQN_FIELD_LEN];
 	char			hostnqn[NVMF_NQN_FIELD_LEN];
+
+	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;
 
@@ -161,6 +168,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)
@@ -285,6 +293,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);
-- 
1.8.4.3

[PATCH 3/4] nvmet: Add controllers to configfs

[Sagi Grimberg]

> 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 | 73 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/target/core.c     |  6 ++++
>   drivers/nvme/target/nvmet.h    | 10 ++++++
>   3 files changed, 89 insertions(+)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index b6aeb1d..a0942c3 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -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->port->disc_addr.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->port->disc_addr.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);
> +

I think nvmet_ctrl is a much better fit as a config_item, perhaps we
need a kernel API to add a config_item? Or maybe it can bring all sorts
of races?

Christoph, what are your thoughts on this?

[PATCH 3/4] nvmet: Add controllers to configfs

[Max Gurtovoy]

On 10/12/2017 3:16 PM, roy wrote:
> hi,
> 
> Consider exposing all the info about the port (trtype, adrfam, etc') 
> maybe by symlink the controller to to port config_item.

We tought about it, but since we create the dirs dynamically (and not by 
the user performing mkdir), we prefered not to hack this symlink at 
first stage.

[PATCH 3/4] nvmet: Add controllers to configfs

[Max Gurtovoy]

On 10/15/2017 11:49 AM, roy wrote:
> Hi,
> 
> I think there is a potential problem here. for example in the next scenario:
> 
> core X is reading .../controllers/N/hostnqn and core Y is handling 
> nvmet_ctrl_free. basically
> 
> core Y can free the ctrl while core X will try to do sprintf on 
> ctrl->hostnqn which will cause NULL dereference.
> 
> Am I missing something?
> 

we delete the ctrl_configfs before freeing the ctrl. parallel processing 
should be protected by internal configfs mutex, just like in the case of 
reading namespace or port items.

for example:

static ssize_t nvmet_ns_enable_show(struct config_item *item, char *page)
{
         return sprintf(page, "%d\n", to_nvmet_ns(item)->enabled);
}

What I do see is an option for freeing the port (by rmdir ../ports/1/) 
and then ctrl->port will point to freed memory. I guess we can cache the 
needed values we want to show.

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

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

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index a0942c3..cb45f6c 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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,
 };
 
-- 
1.8.4.3

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

[Sagi Grimberg]

> 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

Why not rmdir?

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

[Max Gurtovoy]

On 10/11/2017 2:20 PM, Sagi Grimberg wrote:
> 
>> 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 
>>
> 
> Why not rmdir?

Theese dirs weren't created by mkdir..