[RFC PATCH 0/8] nvmet: implement target passthru commands support

[RFC PATCH 0/8] nvmet: implement target passthru commands support

[Chaitanya Kulkarni]

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

[RFC PATCH 1/8] nvme-core: add new interfaces

[Chaitanya Kulkarni]

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.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 2 files changed, 61 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7aeca5db7916..c605bad46ebf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d733b14ede9d..d7c6ea850156 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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

[RFC PATCH 1/8] nvme-core: add new interfaces

[Logan Gunthorpe]

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

[RFC PATCH 1/8] nvme-core: add new interfaces

[Chaitanya Kulkarni]

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

On 3/30/18, 10:48 AM, "Logan Gunthorpe" <logang@deltatee.com> 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
    

[RFC PATCH 1/8] nvme-core: add new interfaces

[Sagi Grimberg]

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

What happens when the nvme device unplugs from the system? We have a
dangling reference to the controller.. Then when/if the device returns
it will occupy another controller name (instance id is occupied).

Do we know (or want to know) how to handle such a case?

[RFC PATCH 1/8] nvme-core: add new interfaces

[Christoph Hellwig]

On Wed, Apr 04, 2018@02:52:15PM +0300, Sagi Grimberg wrote:
> What happens when the nvme device unplugs from the system? We have a
> dangling reference to the controller.. Then when/if the device returns
> it will occupy another controller name (instance id is occupied).
>
> Do we know (or want to know) how to handle such a case?

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

[RFC PATCH 2/8] nvme-core: export existing ctrl and ns interfaces

[Chaitanya Kulkarni]

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.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c | 15 ++++++++++-----
 drivers/nvme/host/nvme.h |  7 +++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c605bad46ebf..dcdea2fe8cd5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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)
 {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d7c6ea850156..990f0c07d19b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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

[RFC PATCH 2/8] nvme-core: export existing ctrl and ns interfaces

[Sagi Grimberg]

On 03/30/2018 09:57 AM, Chaitanya Kulkarni wrote:
> 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.

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

Something like the (untested):
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 383c4d0bc5ff..fce6b090a3a0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
--

[RFC PATCH 3/8] nvmet: add passthru members in target subsys

[Chaitanya Kulkarni]

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.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/nvmet.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 417f6c0331cc..584f1bcc5e79 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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

[RFC PATCH 4/8] nvmet: use const values for id-ctrl

[Chaitanya Kulkarni]

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;
 
 	/* no enforcement soft-limit for maxcmd - pick arbitrary high value */
 	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 584f1bcc5e79..35c7bec641bf 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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

[RFC PATCH 4/8] nvmet: use const values for id-ctrl

[Logan Gunthorpe]

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

[RFC PATCH 4/8] nvmet: use const values for id-ctrl

[Chaitanya Kulkarni]

[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" <logang@deltatee.com> 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
    

[RFC PATCH 4/8] nvmet: use const values for id-ctrl

[Christoph Hellwig]

On Fri, Mar 30, 2018@12:50:45PM -0600, Logan Gunthorpe 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.

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

[RFC PATCH 5/8] nvmet: export nvmet_add_async_event api

[Chaitanya Kulkarni]

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.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c  | 6 ++++--
 drivers/nvme/target/nvmet.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a78029e4e5f4..406fc4ca9768 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 35c7bec641bf..b51c922fa3f6 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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

[RFC PATCH 6/8] nvmet: add target passthru command support

[Chaitanya Kulkarni]

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.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 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

diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index 488250189c99..78238a7157e6 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -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
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index b51c922fa3f6..1495e6df84a7 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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 */
diff --git a/drivers/nvme/target/passthru-cmd.c b/drivers/nvme/target/passthru-cmd.c
new file mode 100644
index 000000000000..038174f4ca28
--- /dev/null
+++ b/drivers/nvme/target/passthru-cmd.c
@@ -0,0 +1,399 @@
+/*
+ * NVMe Over Fabrics Target Passthrough command implementation.
+ * Copyright (c) 2018 Chaitanya Kulkarni Western Digital Corporation
+ * or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/delay.h>
+
+#include "nvmet.h"
+
+#define NVMET_PT_NS_CMD_DELAY	1000
+
+static inline struct nvme_ctrl *nvmet_pt_ctrl(struct nvmet_req *req)
+{
+	return req->sq->ctrl->subsys->pt_ctrl;
+}
+
+static void nvmet_passthru_req_done(struct request *rq,
+		blk_status_t blk_status)
+{
+	struct nvmet_req *req = rq->end_io_data;
+	u16 status = nvme_req(rq)->status;
+
+	nvmet_set_result(req, nvme_req(rq)->result.u32);
+
+	/* prioritize nvme request status over blk_status_t */
+	if (!status && blk_status)
+		status = NVME_SC_INTERNAL;
+
+	nvmet_req_complete(req, status);
+	__blk_put_request(rq->q, rq);
+
+	if (req->pt_ns) {
+		nvme_put_ns(req->pt_ns);
+		req->pt_ns = NULL;
+	}
+}
+
+static struct request *nvmet_blk_make_request(struct nvmet_req *req,
+		struct bio *bio, gfp_t gfp_mask)
+{
+	struct request *rq;
+	struct request_queue *queue;
+	struct nvme_ctrl *pt_ctrl = nvmet_pt_ctrl(req);
+
+	queue = pt_ctrl->admin_q;
+	if (likely(req->sq->qid != 0))
+		queue = req->pt_ns->queue;
+
+	rq = nvme_alloc_request(queue, req->cmd, BLK_MQ_REQ_NOWAIT,
+			NVME_QID_ANY);
+	if (IS_ERR(rq))
+		return rq;
+
+	for_each_bio(bio) {
+		int ret = blk_rq_append_bio(rq, &bio);
+
+		if (unlikely(ret)) {
+			blk_put_request(rq);
+			return ERR_PTR(ret);
+		}
+	}
+	/* for now just use PRPs */
+	req->cmd->common.flags &= (!NVME_CMD_FUSE_FIRST | !NVME_CMD_FUSE_SECOND)
+		| !NVME_CMD_SGL_ALL;
+	return rq;
+}
+
+static inline u16 nvmet_admin_format_nvm_start(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+	int nsid = le32_to_cpu(req->cmd->format.nsid);
+	int lbaf = le32_to_cpu(req->cmd->format.cdw10) & 0x0000000F;
+	struct nvme_id_ns *id;
+
+	id = nvme_identify_ns(nvmet_pt_ctrl(req), nsid);
+	if (!id)
+		return NVME_SC_INTERNAL;
+	/*
+	 * XXX: Please update this code once NVMeOF target starts supoorting
+	 * metadata. We don't support ns lba format with metadata over fabrics
+	 * right now, so report error if format nvm cmd tries to format
+	 * a namespace with the LBA format which has metadata.
+	 */
+	if (id->lbaf[lbaf].ms)
+		status = NVME_SC_INVALID_NS;
+
+	kfree(id);
+	return status;
+}
+
+static inline u16 nvmet_admin_passthru_start(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	/*
+	 * Handle command specific preprocessing .
+	 */
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_format_nvm:
+		status = nvmet_admin_format_nvm_start(req);
+		break;
+	}
+	return status;
+}
+
+static inline u16 nvmet_id_ctrl_init_fabircs_fields(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvme_id_ctrl *id;
+	u16 status = NVME_SC_SUCCESS;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(struct nvme_id_ctrl));
+	if (status)
+		goto out_free;
+
+	id->cntlid = cpu_to_le16(ctrl->cntlid); /* TODO :  ptctrl cntlid*/
+
+	id->acl = NVMET_ID_CTRL_ACL;
+	/* XXX: update these values when AER is implemented for the passthru */
+	id->aerl = 0;
+
+	/* emulate kas as most of the PCIe ctrl don't have a support for kas */
+	id->kas = cpu_to_le16(NVMET_KAS);
+
+	/* don't support host memory buffer */
+	id->hmpre = 0;
+	id->hmmin = 0;
+
+	id->sqes = min_t(__u8, NVMET_ID_CTRL_SQES, id->sqes);
+	id->cqes = min_t(__u8, NVMET_ID_CTRL_CQES, id->cqes);
+	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
+
+	/* don't support fuse commands */
+	id->fuses = 0;
+
+	id->sgls = cpu_to_le32(1 << 0); /* we always support SGLs */
+	if (ctrl->ops->has_keyed_sgls)
+		id->sgls |= cpu_to_le32(1 << 2);
+	if (ctrl->ops->sqe_inline_size)
+		id->sgls |= cpu_to_le32(1 << 20);
+
+	/* to allow loop mode don't use passthru ctrl subnqn */
+	memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));
+
+	/* use fabric id-ctrl values */
+	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
+				ctrl->ops->sqe_inline_size) / 16);
+	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
+
+	id->msdbd = ctrl->ops->msdbd;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(struct nvme_id_ctrl));
+
+out_free:
+	kfree(id);
+out:
+	return status;
+}
+
+static inline u16 nvmet_id_ns_init_fabircs_fields(struct nvmet_req *req)
+{
+	int i;
+	struct nvme_id_ns *id;
+	u16 status = NVME_SC_SUCCESS;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(struct nvme_id_ns));
+	if (status)
+		goto out_free;
+
+	/* don't report the metadata support, clear respective fields */
+	for (i = 0; i < id->nlbaf + 1; i++)
+		if (id->lbaf[i].ms)
+			memset(&id->lbaf[i], 0, sizeof(id->lbaf[i]));
+
+	id->flbas = id->flbas & ~(1 << 4);
+	id->mc = 0;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+
+out_free:
+	kfree(id);
+out:
+	return status;
+}
+
+static inline u16 nvmet_admin_cmd_identify_end(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->identify.cns) {
+	case NVME_ID_CNS_CTRL:
+		status = nvmet_id_ctrl_init_fabircs_fields(req);
+		break;
+	case NVME_ID_CNS_NS:
+		status = nvmet_id_ns_init_fabircs_fields(req);
+		break;
+	}
+
+	return status;
+}
+
+static u16 nvmet_admin_passthru_end(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_identify:
+		status = nvmet_admin_cmd_identify_end(req);
+		break;
+	case nvme_admin_ns_mgmt:
+	case nvme_admin_ns_attach:
+	case nvme_admin_format_nvm:
+		/* allow passthru ctrl to finish the operation */
+		mdelay(NVMET_PT_NS_CMD_DELAY);
+		if (nvmet_add_async_event(req->sq->ctrl,
+					NVME_AER_TYPE_NOTICE, 0, 0) == false)
+			status = NVME_SC_INTERNAL;
+		mdelay(NVMET_PT_NS_CMD_DELAY);
+		break;
+	}
+	return status;
+}
+
+static void nvmet_execute_admin_cmd(struct nvmet_req *req,
+		struct request *ptrq)
+{
+	u16 status;
+	u32 effects;
+
+	status = nvmet_admin_passthru_start(req);
+	if (status)
+		goto out;
+
+	effects = nvme_passthru_start(nvmet_pt_ctrl(req), NULL,
+			req->cmd->common.opcode);
+
+	blk_execute_rq(ptrq->q, NULL, ptrq, 0);
+
+	nvme_passthru_end(nvmet_pt_ctrl(req), effects);
+	status = nvmet_admin_passthru_end(req);
+out:
+	if (status)
+		nvmet_req_complete(req, status);
+	else {
+		nvmet_set_result(req, nvme_req(ptrq)->result.u32);
+		nvmet_req_complete(req, nvme_req(ptrq)->status);
+	}
+	__blk_put_request(ptrq->q, ptrq);
+}
+
+static void nvmet_execute_passthru(struct nvmet_req *req)
+{
+	int i;
+	int op = REQ_OP_READ;
+	int op_flags = 0;
+	int sg_cnt = req->sg_cnt;
+	struct scatterlist *sg;
+	struct bio *bio = NULL;
+	struct bio *prev = NULL;
+	struct bio *first_bio = NULL;
+	struct request *ptrq;
+
+	if (nvme_is_write(req->cmd)) {
+		op = REQ_OP_WRITE;
+		op_flags = REQ_SYNC;
+	}
+
+	if (req->sg_cnt) {
+		bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
+		first_bio = bio;
+		bio->bi_end_io = bio_put;
+
+		for_each_sg(req->sg, sg, req->sg_cnt, i) {
+			if (bio_add_page(bio, sg_page(sg), sg->length,
+						sg->offset) != sg->length) {
+				prev = bio;
+				bio_set_op_attrs(bio, op, op_flags);
+				bio = bio_alloc(GFP_KERNEL,
+						min(sg_cnt, BIO_MAX_PAGES));
+				bio_chain(bio, prev);
+			}
+			sg_cnt--;
+		}
+	}
+
+	ptrq = nvmet_blk_make_request(req, first_bio, GFP_KERNEL);
+	if (!ptrq || IS_ERR(ptrq))
+		goto fail_free_bio;
+
+	if (likely(req->sq->qid != 0)) {
+		ptrq->end_io_data = req;
+		blk_execute_rq_nowait(ptrq->q, NULL, ptrq, 0,
+				nvmet_passthru_req_done);
+	} else
+		nvmet_execute_admin_cmd(req, ptrq);
+	return;
+
+fail_free_bio:
+	while (first_bio) {
+		bio = first_bio;
+		first_bio = first_bio->bi_next;
+		bio_endio(bio);
+	}
+}
+
+static inline bool nvmet_is_pt_admin_cmd_supported(struct nvmet_req *req)
+{
+	bool ret = true;
+	unsigned int fid;
+	struct nvme_command *cmd = req->cmd;
+
+	switch (cmd->common.opcode) {
+	/* black listed commands */
+	case nvme_admin_create_sq:
+	case nvme_admin_create_cq:
+	case nvme_admin_delete_sq:
+	case nvme_admin_delete_cq:
+	case nvme_admin_async_event:	/* not implemented */
+	case nvme_admin_activate_fw:
+	case nvme_admin_download_fw:
+	case nvme_admin_directive_send:
+	case nvme_admin_directive_recv:
+	case nvme_admin_dbbuf:
+	case nvme_admin_security_send:
+	case nvme_admin_security_recv:
+	case nvme_fabrics_command:
+		/* fall thru */
+	/*
+	 * Most PCIe ctrls don't support keep alive cmd, we route
+	 * keep alive to the non-passthru mode. In future please change
+	 * this code when PCIe ctrls with keep alive support available.
+	 */
+	case nvme_admin_keep_alive:
+		ret = false;
+		break;
+	case nvme_admin_set_features:
+		fid = le32_to_cpu(req->cmd->features.fid);
+		switch (fid) {
+		case NVME_FEAT_NUM_QUEUES:	/* disabled */
+		case NVME_FEAT_ASYNC_EVENT:	/* not implemented */
+		case NVME_FEAT_KATO:		/* route through target code */
+			ret = false;
+			break;
+		}
+		break;
+	}
+	return ret;
+}
+
+bool nvmet_is_pt_cmd_supported(struct nvmet_req *req)
+{
+	if (unlikely(req->sq->qid == 0))
+		return nvmet_is_pt_admin_cmd_supported(req);
+
+	return true;
+}
+
+u16 nvmet_parse_pt_cmd(struct nvmet_req *req)
+{
+	if (nvmet_check_ctrl_status(req, req->cmd))
+		return NVME_SC_INVALID_NS | NVME_SC_DNR;
+
+	req->execute = nvmet_execute_passthru;
+
+	/* parse io command */
+	if (likely(req->sq->qid != 0))  {
+		req->pt_ns = nvme_find_get_ns(nvmet_pt_ctrl(req),
+				le32_to_cpu(req->cmd->common.nsid));
+		if (!req->pt_ns) {
+			pr_err("failed to get passthru ns.\n");
+			return NVME_SC_INVALID_NS | NVME_SC_DNR;
+		}
+
+	}
+	return NVME_SC_SUCCESS;
+}
-- 
2.14.1

[RFC PATCH 7/8] nvmet: integrate passthru code with target core

[Chaitanya Kulkarni]

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

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h |  2 ++
 2 files changed, 49 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 406fc4ca9768..2dad36666772 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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);
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 1495e6df84a7..f2d694fffa1c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Chaitanya Kulkarni]

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".

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/configfs.c | 145 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e6b2d2af81b6..fba968086442 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Logan Gunthorpe]

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

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Chaitanya Kulkarni]

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" <logang@deltatee.com> 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
    

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Logan Gunthorpe]

On 02/04/18 06:13 PM, Chaitanya Kulkarni wrote:
> [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. 

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.

> Also having different directories actually makes user aware about what is being
> configured.

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?

> [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.

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.

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

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

> /sys/kernel/config/
> ??? nvmet
>     ??? hosts
>     ??? ports
>     ?   ??? 1
>     ?       ??? referrals
>     ?       ??? subsystems
>     ?           ??? nvme0 -> ../../../../nvmet/pt/nvme0
>     ??? pt
>     ?   ??? nvme0

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.
> [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.

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

> [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.

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

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Christoph Hellwig]

On Fri, Mar 30, 2018@12:13:28PM -0600, Logan Gunthorpe wrote:
> 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

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?

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

Ugg.  Hell no!  Way too much magic.

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Logan Gunthorpe]

On 13/04/18 11:30 AM, Christoph Hellwig wrote:
> 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?

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

> Ugg.  Hell no!  Way too much magic.

Fair point.

Logan

[RFC PATCH 8/8] nvmet: add configfs interface for target passthru

[Stephen Bates]

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

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.

> (compared to a whole new top level directory named 'pt'). 

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.

Tested-By: Stephen Bates <sbates at raithlin.com>

Stephen

[RFC PATCH 0/8] nvmet: implement target passthru commands support

[Logan Gunthorpe]

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

[RFC PATCH 0/8] nvmet: implement target passthru commands support

[Chaitanya Kulkarni]

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

On 3/30/18, 10:47 AM, "Logan Gunthorpe" <logang@deltatee.com> 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
    

[RFC PATCH 0/8] nvmet: implement target passthru commands support

[Sagi Grimberg]

> Hi,

Hi Chaitanya,

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

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?

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

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

> For example with this feature, depending on the target controller
> support host can issue namespace management commands,
> Vendor unique commands etc.

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.

[RFC PATCH 0/8] nvmet: implement target passthru commands support

[Christoph Hellwig]

On Wed, Apr 04, 2018@02:44:11PM +0300, Sagi Grimberg wrote:
> 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...

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

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

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.

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

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

> 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

Sounds reasonable. 

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

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.