nvmet: implement target passthru commands support

Hi,

This patchset implements NVMeOF target passthrough commands support.

In current implementation on the target side, NVMe command is mapped on the block layer request which allows the target to use generic block device.

With the help of passthrough interface, it can now identify the NVMe controller and allow the host to issue NVMe commands along with the VUCs on the target side. For example with this feature, depending on the target controller support host can issue namespace management commands,
Vendor unique commands etc.

The code is tested on the loopback target (nvme-loop) and cut against the Linux 4.16-rc7 (tag v4.16-rc7).

Regards,
Chaitanya

Chaitanya Kulkarni (8):
nvme-core: add new interfaces
nvme-core: export existing ctrl and ns interfaces
nvmet: add passthru members in target subsys
nvmet: use const values for id-ctrl
nvmet: export nvmet_add_async_event api
nvmet: add target passthru command support
nvmet: integrate passthru code with target core
nvmet: add configfs interface for target passthru

 drivers/nvme/host/core.c           |  74 ++++++-
 drivers/nvme/host/nvme.h           |   9 +
 drivers/nvme/target/Makefile       |   2 +-
 drivers/nvme/target/admin-cmd.c    |   6 +-
 drivers/nvme/target/configfs.c     | 145 +++++++++++++-
 drivers/nvme/target/core.c         |  53 ++++-
 drivers/nvme/target/nvmet.h        |  17 ++
 drivers/nvme/target/passthru-cmd.c | 399 +++++++++++++++++++++++++++++++++++++
 8 files changed, 693 insertions(+), 12 deletions(-)
 create mode 100644 drivers/nvme/target/passthru-cmd.c

2.14.1

The new interface nvme_get_ctrl_by_name() allows to search the nvme ctrl by its pathname. On success, it increments the refcount of the ctrl and associated subsystem. The caller needs to decrement the ctrl and
subsystem refcount by calling nvme_put_ctrl_by_name() once the work is done.

This is a preparation patch for implementing NVMe Over Fabric target passthru feature.

---
 drivers/nvme/host/core.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 2 files changed, 61 insertions(+)

index 7aeca5db7916..c605bad46ebf 100644
@@ -99,12 +99,64 @@ static struct class *nvme_subsys_class;
 
 static void nvme_ns_remove(struct nvme_ns *ns);
 static int nvme_revalidate_disk(struct gendisk *disk);
+static int nvme_get_subsystem(struct nvme_subsystem *subsys);
+static void nvme_put_subsystem(struct nvme_subsystem *subsys);
 
 static __le32 nvme_get_log_dw10(u8 lid, size_t size)
 {
 	return cpu_to_le32((((size / 4) - 1) << 16) | lid);
 }
 
+int nvme_get_ctrl_by_name(char *ctrl_name, struct nvme_ctrl **ctrl)
+{
+	int ret = -ENODEV;
+	char str[256] = "/dev/";
+	struct nvme_ctrl *ictrl = NULL;
+	struct nvme_subsystem *isubsys = NULL;
+
+	mutex_lock(&nvme_subsystems_lock);
+	list_for_each_entry(isubsys, &nvme_subsystems, entry) {
+		if (!nvme_get_subsystem(isubsys)) {
+			pr_info("failed to get the subsystem for ctrl %s\n",
+					ctrl_name);
+			goto out;
+		}
+		mutex_unlock(&nvme_subsystems_lock);
+
+		list_for_each_entry(ictrl, &isubsys->ctrls, subsys_entry) {
+			spin_lock(&ictrl->lock);
+			nvme_get_ctrl(ictrl);
+			strcat(str, kobject_name(&ictrl->device->kobj));
+			if (strncmp(str, ctrl_name, strlen(ctrl_name)) == 0) {
+				*ctrl = ictrl;
+				if (try_module_get(ictrl->ops->module)) {
+					spin_unlock(&ictrl->lock);
+					mutex_lock(&nvme_subsystems_lock);
+					ret = 0;
+					goto out;
+				}
+			}
+			nvme_put_ctrl(ictrl);
+			strcpy(str, "/dev/");
+			spin_unlock(&ictrl->lock);
+		}
+		mutex_lock(&nvme_subsystems_lock);
+		nvme_put_subsystem(isubsys);
+	}
+out:
+	mutex_unlock(&nvme_subsystems_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_get_ctrl_by_name);
+
+void nvme_put_ctrl_by_name(struct nvme_ctrl *ctrl)
+{
+	nvme_put_ctrl(ctrl);
+	module_put(ctrl->ops->module);
+	nvme_put_subsystem(ctrl->subsys);
+}
+EXPORT_SYMBOL_GPL(nvme_put_ctrl_by_name);
+
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
 {
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))

@@ -2024,6 +2076,13 @@ static void nvme_init_subnqn(struct nvme_subsystem *subsys, struct nvme_ctrl *ct
 	memset(subsys->subnqn + off, 0, sizeof(subsys->subnqn) - off);
 }
 
+static int nvme_get_subsystem(struct nvme_subsystem *subsys)
+{
+	lockdep_assert_held(&nvme_subsystems_lock);
+
+	return kref_get_unless_zero(&subsys->ref);
+}
+
 static void __nvme_release_subsystem(struct nvme_subsystem *subsys)
 {
 	ida_simple_remove(&nvme_subsystems_ida, subsys->instance);

index d733b14ede9d..d7c6ea850156 100644
@@ -401,6 +401,8 @@ int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
 int nvme_delete_ctrl(struct nvme_ctrl *ctrl);
 int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
 
+int nvme_get_ctrl_by_name(char *name, struct nvme_ctrl **ctrl);
+void nvme_put_ctrl_by_name(struct nvme_ctrl *ctrl);
 extern const struct attribute_group nvme_ns_id_attr_group;
 extern const struct block_device_operations nvme_ns_head_ops;
 

2.14.1

[CK] Okay, I'll fix it in next version.

On 3/30/18, 10:48 AM, "Logan Gunthorpe" <[email protected]> wrote:

On 30/03/18 12:57 AM, Chaitanya Kulkarni wrote:
> +int nvme_get_ctrl_by_name(char *ctrl_name, struct nvme_ctrl **ctrl) > +{
> + int ret = -ENODEV;
> + char str[256] = "/dev/";

Kernel code should not make assumptions about the layout of the filesystem. Prepending "/dev/" to a controller name is icky. I'd suggest looking at blkdev_get_by_path() and doing something similar. Also this function should probably be called nvme_get_ctrl_by_path() seeing it seems to be trying to take a path and not a name.

Logan

That is annoying but not really any different from someone holding open the block or char device, isn't it?

We export existing nvme ctrl and ns management APIs so that target passthru code can manage the nvme ctrl.

This is a preparation patch for implementing NVMe Over Fabric target passthru feature.

---
 drivers/nvme/host/core.c | 15 ++++++++++-----
 drivers/nvme/host/nvme.h |  7 +++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

index c605bad46ebf..dcdea2fe8cd5 100644
@@ -425,10 +425,11 @@ static void nvme_free_ns(struct kref *kref)
 	kfree(ns);
 }
 
-static void nvme_put_ns(struct nvme_ns *ns)
+void nvme_put_ns(struct nvme_ns *ns)
 {
 	kref_put(&ns->kref, nvme_free_ns);
 }
+EXPORT_SYMBOL_GPL(nvme_put_ns);
 
 struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd, blk_mq_req_flags_t flags, int qid)

@@ -1003,7 +1004,7 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
 	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000);
 }
 
-static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
+struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 		unsigned nsid)
 {
 	struct nvme_id_ns *id;

@@ -1028,6 +1029,7 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 
 	return id;
 }
+EXPORT_SYMBOL_GPL(nvme_identify_ns);
 
 static int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 		      void *buffer, size_t buflen, u32 *result)

@@ -1140,7 +1142,7 @@ static u32 nvme_known_admin_effects(u8 opcode)
 	return 0;
 }
 
-static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 								u8 opcode)
 {
 	u32 effects = 0;

@@ -1170,6 +1172,7 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	}
 	return effects;
 }
+EXPORT_SYMBOL_GPL(nvme_passthru_start);
 
 static void nvme_update_formats(struct nvme_ctrl *ctrl)
 {

@@ -1188,7 +1191,7 @@ static void nvme_update_formats(struct nvme_ctrl *ctrl)
 		nvme_ns_remove(ns);
 }
 
-static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
+void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 {
 	/*
 	 * Revalidate LBA changes prior to unfreezing. This is necessary to

@@ -1204,6 +1207,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
 		nvme_queue_scan(ctrl);
 }
+EXPORT_SYMBOL_GPL(nvme_passthru_end);
 
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd)

@@ -2948,7 +2952,7 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 	return nsa->head->ns_id - nsb->head->ns_id;
 }
 
-static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns *ns, *ret = NULL;
 

@@ -2966,6 +2970,7 @@ static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	mutex_unlock(&ctrl->namespaces_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(nvme_find_get_ns);
 
 static int nvme_setup_streams_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
 {

index d7c6ea850156..990f0c07d19b 100644
@@ -363,6 +363,8 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl);
 void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
 void nvme_put_ctrl(struct nvme_ctrl *ctrl);
 int nvme_init_identify(struct nvme_ctrl *ctrl);
+struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
+void nvme_put_ns(struct nvme_ns *ns);
 
 void nvme_queue_scan(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);

@@ -393,9 +395,14 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		union nvme_result *result, void *buffer, unsigned bufflen,
 		unsigned timeout, int qid, int at_head,
 		blk_mq_req_flags_t flags);
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+		u8 opcode);
+void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects);
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
 void nvme_start_keep_alive(struct nvme_ctrl *ctrl);
 void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
+struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
+		unsigned nsid);
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
 int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
 int nvme_delete_ctrl(struct nvme_ctrl *ctrl);

2.14.1

I'd only export only the passthru portion of nvme_user_cmd and use that.

Something like the (untested):
--

index 383c4d0bc5ff..fce6b090a3a0 100644
@@ -1162,22 +1162,14 @@ static void nvme_passthru_end(struct nvme_ctrl 

*ctrl, u32 effects)

                 nvme_queue_scan(ctrl);

}

-static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
- struct nvme_passthru_cmd __user *ucmd)
+int nvme_execute_passtru_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+ struct nvme_passthru_cmd *cmd)
{

-       struct nvme_passthru_cmd cmd;
         struct nvme_command c;
         unsigned timeout = 0;
         u32 effects;
         int status;

- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
- if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
- return -EFAULT;
- if (cmd.flags)
- return -EINVAL;
-

         memset(&c, 0, sizeof(c));
         c.common.opcode = cmd.opcode;
         c.common.flags = cmd.flags;
@@ -1201,6 +1193,24 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, 
struct nvme_ns *ns,
                         0, &cmd.result, timeout);
         nvme_passthru_end(ctrl, effects);

+ return status;
+}
+EXPORT_SYMBOL_GPL(nvme_execute_passtru_cmd);
+
+static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+ struct nvme_passthru_cmd __user *ucmd)
+{
+ struct nvme_passthru_cmd cmd;
+ int status;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+ if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
+ return -EFAULT;
+ if (cmd.flags)
+ return -EINVAL;
+

+       status = nvme_execute_passtru_cmd(ctrl, ns, cmd);
         if (status >= 0) {
                 if (put_user(cmd.result, &ucmd->result))
                         return -EFAULT;

--

This patch adds new structure members to implement NVMeOF target passthru support.

On the target side, we add nvme_ctrl reference to the nvmet_subsys. We implement nvme ctrl passthru and attach passthru ctrl to the nvmet_subsys.

At the time when dynamically target ctrl is created, passthru ctrl is accessed via subsystem, in this way we can configure passthru ctrl similar to how namespaces are configured by default.

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

index 417f6c0331cc..584f1bcc5e79 100644
@@ -160,6 +160,9 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+
+	char			*pt_ctrl_path;
+	struct nvme_ctrl	*pt_ctrl;
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)

2.14.1

This patch adds constant values which are used in
target identify ctrl.

These values will be used in implementing target passthru identify ctrl to mask the passthru ctrl values with the default target ctrl values.

---
 drivers/nvme/target/admin-cmd.c | 6 +++---
 drivers/nvme/target/nvmet.h     | 4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

index 90dcdc40ac71..6b3d20b32187 100644
@@ -210,7 +210,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	 * comands.  But we don't do anything useful for abort either, so
 	 * no point in allowing more abort commands than the spec requires.
 	 */
-	id->acl = 3;
+	id->acl = NVMET_ID_CTRL_ACL;
 
 	id->aerl = NVMET_ASYNC_EVENTS - 1;
 

@@ -223,8 +223,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	/* We support keep-alive timeout in granularity of seconds */
 	id->kas = cpu_to_le16(NVMET_KAS);
 
-	id->sqes = (0x6 << 4) | 0x6;
-	id->cqes = (0x4 << 4) | 0x4;
+	id->sqes = NVMET_ID_CTRL_SQES;
+	id->cqes = NVMET_ID_CTRL_CQES;
 
 	/* no enforcement soft-limit for maxcmd - pick arbitrary high value */
 	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);

index 584f1bcc5e79..35c7bec641bf 100644
@@ -325,6 +325,10 @@ u32 nvmet_get_log_page_len(struct nvme_command *cmd);
 #define NVMET_MAX_CMD		NVMET_QUEUE_SIZE
 #define NVMET_KAS		10
 #define NVMET_DISC_KATO		120
+#define NVMET_ID_CTRL_SQES     ((0x6 << 4) | 0x6)
+#define NVMET_ID_CTRL_CQES     ((0x4 << 4) | 0x4)
+#define NVMET_ID_CTRL_ACL      3
+
 
 int __init nvmet_init_configfs(void);
 void __exit nvmet_exit_configfs(void);

2.14.1

[CK] In this way we have less number of code changes, may be when we add more target type we can think of adding helpers for id-ctrl and id-ns to isolate the common code ?

On 3/30/18, 11:51 AM, "Logan Gunthorpe" <[email protected]> wrote:

    On 30/03/18 12:57 AM, Chaitanya Kulkarni wrote:
    > This patch adds constant values which are used in
    > target identify ctrl.
    > 
    > These values will be used in implementing
    > target passthru identify ctrl to mask the passthru
    > ctrl values with the default target ctrl values.
    > 
    > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
    > ---
    >  drivers/nvme/target/admin-cmd.c | 6 +++---
    >  drivers/nvme/target/nvmet.h     | 4 ++++
    >  2 files changed, 7 insertions(+), 3 deletions(-)
    > 
    > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
    > index 90dcdc40ac71..6b3d20b32187 100644
    > --- a/drivers/nvme/target/admin-cmd.c
    > +++ b/drivers/nvme/target/admin-cmd.c
    > @@ -210,7 +210,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
    >  	 * comands.  But we don't do anything useful for abort either, so
    >  	 * no point in allowing more abort commands than the spec requires.
    >  	 */
    > -	id->acl = 3;
    > +	id->acl = NVMET_ID_CTRL_ACL;
    >  
    >  	id->aerl = NVMET_ASYNC_EVENTS - 1;
    >  
    > @@ -223,8 +223,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
    >  	/* We support keep-alive timeout in granularity of seconds */
    >  	id->kas = cpu_to_le16(NVMET_KAS);
    >  
    > -	id->sqes = (0x6 << 4) | 0x6;
    > -	id->cqes = (0x4 << 4) | 0x4;
    > +	id->sqes = NVMET_ID_CTRL_SQES;
    > +	id->cqes = NVMET_ID_CTRL_CQES;
    
    Hmm, I don't know... maybe see what others think but this doesn't feel
    right to me. Perhaps a helper function to set the common elements wtih
    nvmet_id_ctrl_init_fabircs_fields() of the id struct would be more
    appropriate.
    
    Logan

Or just keep it open coded, the numeric values should be pretty obvious from the (public) spec.

This patch changes the return value for nvmet_add_async_event() and exports the same API.

This change is needed for the target passthru code to generate async events.

---
 drivers/nvme/target/core.c  | 6 ++++--
 drivers/nvme/target/nvmet.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

index a78029e4e5f4..406fc4ca9768 100644
@@ -117,14 +117,14 @@ static void nvmet_async_event_work(struct work_struct *work)
 	}
 }
 
-static void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+bool nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 		u8 event_info, u8 log_page)
 {
 	struct nvmet_async_event *aen;
 
 	aen = kmalloc(sizeof(*aen), GFP_KERNEL);
 	if (!aen)
-		return;
+		return false;
 
 	aen->event_type = event_type;
 	aen->event_info = event_info;

@@ -135,6 +135,8 @@ static void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 	mutex_unlock(&ctrl->lock);
 
 	schedule_work(&ctrl->async_event_work);
+
+	return true;
 }
 
 int nvmet_register_transport(struct nvmet_fabrics_ops *ops)

index 35c7bec641bf..b51c922fa3f6 100644
@@ -304,6 +304,8 @@ void nvmet_ns_disable(struct nvmet_ns *ns);
 struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid);
 void nvmet_ns_free(struct nvmet_ns *ns);
 
+bool nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+		u8 event_info, u8 log_page);
 int nvmet_register_transport(struct nvmet_fabrics_ops *ops);
 void nvmet_unregister_transport(struct nvmet_fabrics_ops *ops);
 

2.14.1

This patch adds passthru command handling capability for the NVMeoF target and exports passthru APIs which are used to integrate with passthru code with nvmet-core.

We add passthru ns member to the target request to hold the ns reference for respective commands.

The new file passthru-cmd.c handles passthru cmd parsing and execution. In the passthru mode create a block layer request from the nvmet request and map the data on block layer request.

For handling the side effects we add two functions similar to the passthru functions present in the nvme-core.

We explicitly blacklist the commands at the time of parsing, which allows us to route the fabric commands through default code path.

---
 drivers/nvme/target/Makefile       |   2 +-
 drivers/nvme/target/nvmet.h        |   6 +
 drivers/nvme/target/passthru-cmd.c | 399 +++++++++++++++++++++++++++++++++++++
 3 files changed, 406 insertions(+), 1 deletion(-)
 create mode 100644 drivers/nvme/target/passthru-cmd.c

index 488250189c99..78238a7157e6 100644
@@ -7,7 +7,7 @@ obj-$(CONFIG_NVME_TARGET_FC)		+= nvmet-fc.o
 obj-$(CONFIG_NVME_TARGET_FCLOOP)	+= nvme-fcloop.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o io-cmd.o fabrics-cmd.o \
-			discovery.o
+			discovery.o passthru-cmd.o
 nvme-loop-y	+= loop.o
 nvmet-rdma-y	+= rdma.o
 nvmet-fc-y	+= fc.o

index b51c922fa3f6..1495e6df84a7 100644
@@ -27,6 +27,8 @@
 #include <linux/rcupdate.h>
 #include <linux/blkdev.h>
 
+#include "../host/nvme.h"
+
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
 

@@ -235,6 +237,8 @@ struct nvmet_req {
 
 	void (*execute)(struct nvmet_req *req);
 	struct nvmet_fabrics_ops *ops;
+
+	struct nvme_ns *pt_ns;
 };
 
 static inline void nvmet_set_status(struct nvmet_req *req, u16 status)

@@ -264,6 +268,7 @@ struct nvmet_async_event {
 };
 
 u16 nvmet_parse_connect_cmd(struct nvmet_req *req);
+u16 nvmet_parse_pt_cmd(struct nvmet_req *req);
 u16 nvmet_parse_io_cmd(struct nvmet_req *req);
 u16 nvmet_parse_admin_cmd(struct nvmet_req *req);
 u16 nvmet_parse_discovery_cmd(struct nvmet_req *req);

@@ -345,4 +350,5 @@ extern struct rw_semaphore nvmet_config_sem;
 bool nvmet_host_allowed(struct nvmet_req *req, struct nvmet_subsys *subsys,
 		const char *hostnqn);
 
+bool nvmet_is_pt_cmd_supported(struct nvmet_req *req);
 #endif /* _NVMET_H */

2.14.1

This patch integrates passthru code with target core, it exports APIs to enable/disable passthru ctrl via configfs.

---
 drivers/nvme/target/core.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h |  2 ++
 2 files changed, 49 insertions(+)

index 406fc4ca9768..2dad36666772 100644
@@ -400,6 +400,29 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	return ns;
 }
 
+int nvmet_pt_ctrl_enable(struct nvmet_subsys *subsys)
+{
+	if (!subsys)
+		return -ENODEV;
+
+	if (nvme_get_ctrl_by_name(subsys->pt_ctrl_path, &subsys->pt_ctrl)) {
+		pr_err("unable to find passthru ctrl' %s'\n",
+				subsys->pt_ctrl_path);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+void nvmet_pt_ctrl_disable(struct nvmet_subsys *subsys)
+{
+	if (!subsys || !subsys->pt_ctrl)
+		return;
+
+	nvme_put_ctrl_by_name(subsys->pt_ctrl);
+	subsys->pt_ctrl = NULL;
+}
+
 static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
 {
 	u32 old_sqhd, new_sqhd;

@@ -501,6 +524,14 @@ int nvmet_sq_init(struct nvmet_sq *sq)
 }
 EXPORT_SYMBOL_GPL(nvmet_sq_init);
 
+static bool nvmet_ctrl_pt_allow(struct nvmet_req *req)
+{
+	if (req->sq->ctrl && !req->sq->ctrl->subsys->pt_ctrl)
+		return false;
+
+	return nvmet_is_pt_cmd_supported(req);
+}
+
 bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 		struct nvmet_sq *sq, struct nvmet_fabrics_ops *ops)
 {

@@ -535,6 +566,8 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 	if (unlikely(!req->sq->ctrl))
 		/* will return an error for any Non-connect command: */
 		status = nvmet_parse_connect_cmd(req);
+	else if (nvmet_ctrl_pt_allow(req))
+		status = nvmet_parse_pt_cmd(req);
 	else if (likely(req->sq->qid != 0))
 		status = nvmet_parse_io_cmd(req);
 	else if (req->cmd->common.opcode == nvme_fabrics_command)

@@ -570,6 +603,19 @@ EXPORT_SYMBOL_GPL(nvmet_req_uninit);
 
 void nvmet_req_execute(struct nvmet_req *req)
 {
+	/*
+	 * Right now data_len is calculated before the transfer len
+	 * after we parse the command, With passthru interface
+	 * we allow VUC's. In order to make the code simple and compact,
+	 * instead of assinging the the dala len for each VUC in the command
+	 * parse function just use the transfer len as it is. This may
+	 * result is error if expected data_len != transfer_len.
+	 */
+	if (req->sq->ctrl && req->sq->ctrl->subsys->pt_ctrl) {
+		req->execute(req);
+		return;
+	}
+
 	if (unlikely(req->data_len != req->transfer_len))
 		nvmet_req_complete(req, NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR);
 	else

@@ -1010,6 +1056,7 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
+	kfree(subsys->pt_ctrl_path);
 	kfree(subsys->subsysnqn);
 	kfree(subsys);
 }

index 1495e6df84a7..f2d694fffa1c 100644
@@ -350,5 +350,7 @@ extern struct rw_semaphore nvmet_config_sem;
 bool nvmet_host_allowed(struct nvmet_req *req, struct nvmet_subsys *subsys,
 		const char *hostnqn);
 
+int nvmet_pt_ctrl_enable(struct nvmet_subsys *subsys);
+void nvmet_pt_ctrl_disable(struct nvmet_subsys *subsys);
 bool nvmet_is_pt_cmd_supported(struct nvmet_req *req);
 #endif /* _NVMET_H */

2.14.1

This patch adds configfs interface for target passthru ctrl management.

The new directory "pt" under nvmet configfs is added to configure passthru ctrl (pt-ctrl). Once all the fields for the pt-ctrl are set user can enable the pt-ctrl through configfs.

The new directory pt is parallel to the subsystem and has similar attributes as default subsystem, each passthru ctrl is represented as one subsystem. Since we allow ctrl passthru we don't export or allow user to access the passthru namespace(s) in the configfs.

The new attribute "attr_ctrl_path" expects nvme ctrl path, e.g. "/dev/nvmeX".

---
 drivers/nvme/target/configfs.c | 145 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

index e6b2d2af81b6..fba968086442 100644
@@ -21,6 +21,7 @@
 #include "nvmet.h"
 
 static const struct config_item_type nvmet_host_type;
+static const struct config_item_type nvmet_pt_type;
 static const struct config_item_type nvmet_subsys_type;
 
 /*

@@ -477,7 +478,8 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 	struct nvmet_subsys_link *link, *p;
 	int ret;
 
-	if (target->ci_type != &nvmet_subsys_type) {
+	if (!(target->ci_type == &nvmet_subsys_type ||
+				target->ci_type == &nvmet_pt_type)) {
 		pr_err("can only link subsystems into the subsystems dir.!\n");
 		return -EINVAL;
 	}

@@ -772,6 +774,141 @@ static const struct config_item_type nvmet_subsystems_type = {
 	.ct_owner		= THIS_MODULE,
 };
 
+
+/*
+ * Passthru attributes and operations.
+ */
+static ssize_t nvmet_pt_attr_ctrl_path_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", subsys->pt_ctrl_path);
+}
+
+static ssize_t nvmet_pt_attr_ctrl_path_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int ret = -ENOMEM;
+
+	mutex_lock(&subsys->lock);
+	kfree(subsys->pt_ctrl_path);
+
+	subsys->pt_ctrl_path = kstrdup(page, GFP_KERNEL);
+	if (!subsys->pt_ctrl_path)
+		goto out_unlock;
+
+	mutex_unlock(&subsys->lock);
+
+	return count;
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
+
+}
+CONFIGFS_ATTR(nvmet_pt_, attr_ctrl_path);
+
+static ssize_t nvmet_pt_attr_enable_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", to_subsys(item)->pt_ctrl ? 1 : 0);
+}
+
+static ssize_t nvmet_pt_attr_enable_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	bool enable;
+	int ret = 0;
+
+	if (subsys->pt_ctrl_path == NULL) {
+		pr_err("passthru ctrl path is not initialized.\n");
+		return -EINVAL;
+	}
+
+	if (strtobool(page, &enable))
+		return -EINVAL;
+
+	if (enable)
+		ret = nvmet_pt_ctrl_enable(subsys);
+	else
+		nvmet_pt_ctrl_disable(subsys);
+
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_pt_, attr_enable);
+
+static struct configfs_attribute *nvmet_pt_attrs[] = {
+	&nvmet_pt_attr_attr_ctrl_path,
+	&nvmet_pt_attr_attr_enable,
+	&nvmet_subsys_attr_attr_allow_any_host,
+	&nvmet_subsys_attr_attr_version,
+	&nvmet_subsys_attr_attr_serial,
+	NULL,
+};
+
+/*
+ * Passthru structures & folder operation functions below
+ */
+static void nvmet_pt_release(struct config_item *item)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	nvmet_subsys_del_ctrls(subsys);
+	/*
+	 * Since we are the only one to manage the pt ctrl,
+	 * disable the pt ctrl in case user did not disable
+	 * the ctrl attribute prior to removing the passthru subsystem.
+	 */
+
+	nvmet_pt_ctrl_disable(subsys);
+
+	nvmet_subsys_put(subsys);
+}
+
+static struct configfs_item_operations nvmet_pt_item_ops = {
+	.release		= nvmet_pt_release,
+};
+
+static const struct config_item_type nvmet_pt_type = {
+	.ct_item_ops		= &nvmet_pt_item_ops,
+	.ct_attrs		= nvmet_pt_attrs,
+	.ct_owner		= THIS_MODULE,
+};
+
+static struct config_group *nvmet_pt_make(struct config_group *group,
+		const char *name)
+{
+	struct nvmet_subsys *subsys;
+
+	if (sysfs_streq(name, NVME_DISC_SUBSYS_NAME)) {
+		pr_err("can't create discovery subsystem through configfs\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
+	if (!subsys)
+		return ERR_PTR(-ENOMEM);
+
+	config_group_init_type_name(&subsys->group, name, &nvmet_pt_type);
+
+	config_group_init_type_name(&subsys->allowed_hosts_group,
+			"allowed_hosts", &nvmet_allowed_hosts_type);
+	configfs_add_default_group(&subsys->allowed_hosts_group,
+			&subsys->group);
+
+	return &subsys->group;
+}
+
+static struct configfs_group_operations nvmet_pts_group_ops = {
+	.make_group		= nvmet_pt_make,
+};
+
+static struct config_item_type nvmet_pts_type = {
+	.ct_group_ops		= &nvmet_pts_group_ops,
+	.ct_owner		= THIS_MODULE,
+};
+
 static ssize_t nvmet_referral_enable_show(struct config_item *item,
 		char *page)
 {

@@ -927,6 +1064,7 @@ static const struct config_item_type nvmet_ports_type = {
 };
 
 static struct config_group nvmet_subsystems_group;
+static struct config_group nvmet_pts_group;
 static struct config_group nvmet_ports_group;
 
 static void nvmet_host_release(struct config_item *item)

@@ -990,6 +1128,11 @@ int __init nvmet_init_configfs(void)
 	config_group_init(&nvmet_configfs_subsystem.su_group);
 	mutex_init(&nvmet_configfs_subsystem.su_mutex);
 
+	config_group_init_type_name(&nvmet_pts_group,
+			"pt", &nvmet_pts_type);
+	configfs_add_default_group(&nvmet_pts_group,
+			&nvmet_configfs_subsystem.su_group);
+
 	config_group_init_type_name(&nvmet_subsystems_group,
 			"subsystems", &nvmet_subsystems_type);
 	configfs_add_default_group(&nvmet_subsystems_group,

2.14.1

I *really* dislike this interface. Not only is the "pt" directory confusing to the user, it's nearly identical to what a subsystem is. So as developers, if we want to add a config option to a subsystem we also have to add it to the equivalent pt. I've already seen one patch set that would be mutually exclusive to this only because of the configuration mess.

I'd much rather see either a couple new attributes in the existing subsystem or some mechanism such that if you create a namespace that points to a controller (eg /dev/nvme1) it takes it as though you're trying to pass through the whole NVMe device. (The code would need to ensure that a passthrough subsystem configured this way could only have exactly one namespace.) I think that would be a lot more intuitive than a directory label "pt" and prevents all the duplication of what a subsystem is.

Logan

In general, I?ve implemented the configfs structure in such a way it will keep pt configuration isolated from the subsystem management.

Please see my inline comment for more details.

On 3/30/18, 11:14 AM, "Logan Gunthorpe" <[email protected]> wrote:

On 30/03/18 12:57 AM, Chaitanya Kulkarni wrote:
> This patch adds configfs interface for target passthru > ctrl management.
>
> The new directory "pt" under nvmet configfs is added to > configure passthru ctrl (pt-ctrl). Once all the fields > for the pt-ctrl are set user can enable the pt-ctrl through > configfs.
>
> The new directory pt is parallel to the subsystem and has similar > attributes as default subsystem, each passthru ctrl is represented > as one subsystem. Since we allow ctrl passthru we don't export or > allow user to access the passthru namespace(s) in the configfs.

I *really* dislike this interface. Not only is the "pt" directory confusing to the user, it's nearly identical to what a subsystem is.

[CK] Actually pt directory is not identical to what subsystem is in following ways: -

1. Configuration: -
subsystem: - we use configfs namespaces directory structure to configure the ns.
pt: - we use NVMe ns-mgmt commands which allows host to configure the ns.
2. Internal representation: -
subsystem: - we create target ns object for each ns.
pt: - we don't create target ns object for each ns.
3. NS-Management: -
subsystem: - Namespaces are created and configured on the target side with only configfs interface.
pt: - Namespaces are created from the host side (and optionally on the target side with tools before pt configuration) without going through configfs interface.
Also having different directories actually makes user aware about what is being configured.

So as developers, if we want to add a config option to a subsystem we also have to add it to the equivalent pt. I've already seen one patch set that would be mutually exclusive to this only because of the configuration mess.

[CK] We should only add configfs options to the pt interface if
it is supported by the pt interface, if it is not supported by the
pt interface then it we should avoid it in the configfs, current proposal provides developers flexibility to have only supported options for pt. In this way we keep the distinction clear about subsystem and pt.
Also, we reuse most of the subsystem code for the attributes
so even in future subsys gets a new attribute there shouldn't
be much duplication of the code.

If we really don?t want have ?pt? parallel to ?subsystems? then please have a look at 3 rd approach of pt under subsystem, this way we will share the subsystems attributes, here as you mentioned we can prevent creation of the namespaces through configfs when ?attr_ctrl_path? is configured.
For reference providing existing Subsystem (1) and pt (2) configuration.

1. Existing Subsystem: -

/sys/kernel/config/
??? nvmet

    ??? hosts
    ??? ports
    ?   ??? 1
    ?       ??? addr_adrfam
    ?       ??? addr_traddr
    ?       ??? addr_treq
    ?       ??? addr_trsvcid
    ?       ??? addr_trtype
    ?       ??? referrals
    ?       ??? subsystems
    ?           ??? testnqn1 -> ../../../../nvmet/subsystems/testnqn1
    ??? pt
    ??? subsystems
        ??? testnqn1
            ??? allowed_hosts
            ??? attr_allow_any_host
            ??? attr_serial
            ??? attr_version
            ??? namespaces
                ??? 1
                    ??? device_nguid
                    ??? device_path
                    ??? device_uuid
                    ??? enable

2. Current PT implementation: -

/sys/kernel/config/
??? nvmet
??? hosts
??? ports
? ??? 1
? ??? addr_adrfam
? ??? addr_traddr
? ??? addr_treq
? ??? addr_trsvcid
? ??? addr_trtype
? ??? referrals
? ??? subsystems
? ??? nvme0 -> ../../../../nvmet/pt/nvme0
??? pt
? ??? nvme0
? ??? allowed_hosts
? ??? attr_allow_any_host
? ??? attr_ctrl_path
? ??? attr_enable
? ??? attr_serial
? ??? attr_version
??? subsystems

11 directories, 10 files

3. PT under Subsystem proposed implementation: -

/sys/kernel/config/
??? nvmet
??? hosts
??? ports
? ??? 1
? ??? addr_adrfam
? ??? addr_traddr
? ??? addr_treq
? ??? addr_trsvcid
? ??? addr_trtype
? ??? referrals
? ??? subsystems
? ??? testnqn1 -> ../../../../nvmet/subsystems/testnqn1
??? subsystems
??? nvme0
??? allowed_hosts
??? attr_allow_any_host
??? attr_serial
??? attr_version
??? namespaces
??? pt
??? attr_ctrl_path
???attr_enable

I'd much rather see either a couple new attributes in the existing subsystem or some mechanism such that if you create a namespace that points to a controller (eg /dev/nvme1) it takes it as though you're trying to pass through the whole NVMe device.

[CK] That is exactly what I'd try to avoid since it will entangle the code of subsystem and pt. In case if everyone is okay with this approach, let?s do this.

(The code would need to
ensure that a passthrough subsystem configured this way could only have exactly one namespace.)

[CK] This imposes burden on the sysadmin to have a mandatory namespace. Since it is a ctrl pass-through we should avoid use of both pass-through-ns, configfs-namespaces directory and internal target ns structure as much as possible.

I think that would be a lot more intuitive than
a directory label "pt" and prevents all the duplication of what a subsystem is.

Logan

So this is just a long winded way to say that pt subsystems don't need to configure the namespaces. It's not really a justification to make them their own first class entity.

Not really. You're making too large a distinction between pt and subsystem. They are really the same thing and used in the same way. The only difference is one has explicit namespaces and the other takes them from the backing device. So why not just make them the same thing and configure them slightly differently?

I suspect most new config attributes will be supported by both and, as I said, it will be a burden to developers to have to put them in both places. 'pt' already replicates many of the same options as subsystems.

I don't know what you are getting at here.

This is also confusing to the user: linking a pt under the subsystems directory. This is more evidence that a pt is just a subsystem.

Good, we want them entangled because they are the same thing.

No, it imposes no additional burden on the sysadmin. With the pt structure they require a single mandatory ns as well. The only difference is where things are configured and your current scheme is confusing, creates extra work for developers and unnecessarily duplicates an existing concept. 'pt' is also a very poor name that's completely meaningless to anyone that doesn't already understand what it already means.

Logan

Maybe. But how would you design it? Have a passthrough filed under the subsystem that you can echo a controller name to? And then make the exclusive vs creating namespaces?

Ugg. Hell no! Way too much magic.

Yes. It only really requires one field (the path to the nvme char device) and making it exclusive to namespaces doesn't seem that unintuitive. An error and appropriate dmesg if one is set before the other would be pretty clear and quickly understood (compared to a whole new top level directory named 'pt'). When I'm playing around with sysfs/configfs/debugfs interfaces I rarely read the documentation and just watch dmesg if I see an error returned. When I first played around with this patchset the current interface required me to look at the code to understand what was going on (there was no documentation).

Fair point.

Logan

I have to agree. Creating a whole new tree for passthru seems a lot less intuitive than adding a single passthru field to the subsystem tree.

Pretty please can we call it passthru? pt is totally incompressible to the casual reader...

BTW we did test this series on one of our x86_64 target setups and it did a great job passing our vendor specific admin commands through to the NVMe namespaces.

Stephen

Hi Chaitanya,

I'm very much in favor of the concept of this patchset but I think it still needs some work. I'll post some notes on the following patches shortly.

I'm already aware of what this patchset is trying to accomplish but I find this description confusing and hard to understand. I think you need to say something along the lines of:

"This patchset aims to enable passing through entire physical NVMe drive through an NVMe over fabrics interface. This implies that all namespaces on the target drive are passed through to the initiator one-to-one and all admin commands from the initiator are forwarded back to the target drive. This allows for exposing Vender Unique Commands (VUCs) over the NVMe-OF interface."

Logan

[CK] Thanks for the comments, I'm fine with your description, if everyone agrees.

On 3/30/18, 10:47 AM, "Logan Gunthorpe" <[email protected]> wrote:

Hi Chaitanya,

I'm very much in favor of the concept of this patchset but I think it still needs some work. I'll post some notes on the following patches shortly.

On 30/03/18 12:57 AM, Chaitanya Kulkarni wrote:
> This patchset implements NVMeOF target passthrough commands support. >
> In current implementation on the target side, NVMe command is mapped on the > block layer request which allows the target to use generic block device. >
> With the help of passthrough interface, it can now identify the NVMe controller > and allow the host to issue NVMe commands along with the VUCs on the > target side. For example with this feature, depending on the target controller > support host can issue namespace management commands,
> Vendor unique commands etc.

I'm already aware of what this patchset is trying to accomplish but I find this description confusing and hard to understand. I think you need to say something along the lines of:

"This patchset aims to enable passing through entire physical NVMe drive through an NVMe over fabrics interface. This implies that all namespaces on the target drive are passed through to the initiator one-to-one and all admin commands from the initiator are forwarded back to the target drive. This allows for exposing Vender Unique Commands (VUCs) over the NVMe-OF interface."

Logan

Hi Chaitanya,

So for normal IO commands, I would simply move the bio construction loop to shared code and call it from either the normal nvmet_execute_* or nvmet_execute_passthru_io_cmd. I still don't understand what that buys us... I guess one benefit would be to utilize ranged (merged) discards instead of executing them range by range...

For admin commands I would do something different though, why not simply exporting __nvme_passthru_cmd (portion of nvme_user_cmd) and simply call that with a bit of tweaks to the host memory layout. That would eliminate the re-code in nvmet_execute_admin_cmd() Thoughts?

We need code to exclude more than a single access to the same passthru controller (or subsystem in fact). Otherwise we might be exposing the user to lots of troubles...

Its also odd that a namespace is given a passthru access to a controller. Maybe we need to add a configfs interface for passthru subsystems? (under subsystems/ we have passthru file that takes the controller name or subsystem id or something)

Then automatically all the namespaces of that controller will be exposed via the subsystem (or fail if namespaces are already attached to other subsystems).

So namespace management needs a mechanism to add the new namespace via the fabrics target. This would require adding an event client mechanism to nvme that will create that namespace in nvmet before issuing the AEN.

What are "normal" I/O commands? There can be vendor defined I/O commands that can do all kinds of weird things the block layer can't do. Hell, there are plenty of NVMe defined I/O commands that map to nothing the block layer could deal with.

We'll need an implementation that can deal with anything thrown at it, and Chaitanya has written that (turns out that part looks fairly trivial, it's all the setup around it that is a bit of a mess).

Why? Not really much simpler, probably much slower due to the synchronous nature, and it also requires reshuffling the on the wire format of the SQe into another form just to unpack it again.

Only to the same controller I think, and then only allow one fabrics controller to be created for it.

Sounds reasonable.

Yeah, for now we should simply disallow it. Unfortunatelt it turns out the whole NVMe architecture isn't really well suitable for any kind of tunneling.