Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v9 00/12] nvmet: add target passthru commands support
@ 2019-10-09 19:25 Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
                   ` (12 more replies)
  0 siblings, 13 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

Hi,

This is v9 of the passthru patchset. This addresses some of Sagi's
feedback from v8 and rebases onto v5.4-rc2.

--

Chaitainya has asked us to take on these patches as we have an
interest in getting them into upstream. To that end, we've done
a large amount of testing, bug fixes and cleanup.

Passthru support for nvmet allows users to export an entire
NVMe controller through NVMe-oF. When exported in this way (as opposed
to exporting each namespace as a block device), all the NVMe commands
are passed to the given controller unmodified, including most admin
commands and Vendor Unique Commands (VUCs). A passthru target will
expose all namespaces for a given device to the remote host.

There are three major non-bugfix changes that we've done to the series:

1) Instead of using a seperate special passthru subsystem in
   configfs simply add a passthru directory that's analogous to
   the existing namespace directories. The directories have
   very similar attributes to namespaces but are mutually exclusive.
   If a user enables a namespaces, they can't then enable
   passthru controller and vice versa. This simplifies the code
   required to implement passthru configfs and IMO creates a much
   clearer and uniform interface.

2) Instead of taking a bare controller name (ie. "nvme1"), take a
   full device path to the controller's char device. This is more
   consistent with the regular namespaces which take a path and
   also allows users to make use of udev rules and symlinks to
   manage their controllers instead of the potentially unstable
   device names.

3) Implement block accounting for the passthru devices. This is so
   the target OS can still track device usage using /proc/diskstats.

Besides these three changes, we've also found a large number of bugs
and crashes and did a bunch of testing with KASAN, lockdep and kmemleak.
A more complete list of changes is given below.

Additionally, we've written some new blktests to test the passthru
code. A branch is available here[1] and can be submitted once these
patches are upstream.

These patches are based off of v5.4-rc2 and a git branch is available
at [2].

Thanks,

Logan

[1] https://github.com/Eideticom/blktests nvmet_passthru
[2] https://github.com/sbates130272/linux-p2pmem/ nvmet_passthru_v9

--

v9 Changes:
  1. Rebased onto v5.4-rc2 (required adjusting nvme_identify_ns() usage)
  2. Collected Sagi's Reviewed-By Tags
  3. Squashed seperate Kconfig patch into passthru patch (Per Sagi)
  4. Set REQ_FUA for flush requests and remove special casing
     on RQF_IO_STAT (Per Sagi)

v8 Changes:
  1. Rebased onto v5.3-rc6
  2. Collected Max's Reviewed-By tags
  3. Converted admin command black-list to a white-list, but
     allow all vendor specific commands. With this, we feel
     it's safe to allow multiple connections from hosts.
     (As per Sagi's feedback)

v7 Changes:
  1. Rebased onto v5.3-rc2
  2. Rework nvme_ctrl_get_by_path() to use filp_open() instead of
     the cdev changes that were in v6. (Per Al)
  3. Override the cmic bit to allow multipath and allow
     multiple connections from the same hostnqn. (At the same
     time I cleaned up the method of rejecting multiple connections.)
     See Patch 8)
  4. Found a bug when used with the tcp transport (See Patch 10)

v6 Changes:
  1. Rebased onto v5.3-rc1
  2. Rework configfs interface to simply be a passthru directory
     within the existing subsystem. The directory is similar to
     and consistent with a namespace directory.
  3. Have the configfs take a path instead of a bare controller name
  4. Renamed the main passthru file to io-cmd-passthru.c for consistency
     with the file and block-dev methods.
  5. Cleaned up all the CONFIG_NVME_TARGET_PASSTHRU usage to remove
     all the inline #ifdefs
  6. Restructured nvmet_passthru_make_request() a bit for clearer code
  7. Moved nvme_find_get_ns() call into nvmet_passthru_execute_cmd()
     seeing calling it in nvmet_req_init() causes a lockdep warning
     due to nvme_find_get_ns() being able to sleep.
  8. Added a check in nvmet_passthru_execute_cmd() to ensure we don't
     violate queue_max_segments or queue_max_hw_sectors and overrode
     mdts to ensure hosts don't intentionally submit commands
     that will exceed these limits.
  9. Reworked the code which ensures there's only one subsystem per
     passthru controller to use an xarray instead of a list as this is
     simpler and more easily fixed some bugs triggered by disabling
     subsystems that weren't enabled.
 10. Removed the overide of the target cntlid with the passthru cntlid;
     this seemed like a really bad idea especially in the presence of
     mixed systems as you could end up with two ctrlrs with the same
     cntlid. For now, commands that depend on cntlid are black listed.
 11. Implement block accounting for passthru so the target can track
     usage using /proc/diskstats
 12. A number of other minor bug fixes and cleanups

v5 Changes (not sent to list, from Chaitanya):
  1. Added workqueue for admin commands.
  2. Added kconfig option for the pass-thru feature.
  3. Restructure the parsing code according to your suggestion,
     call nvmet_xxx_parse_cmd() from nvmet_passthru_parse_cmd().
  4. Use pass-thru instead of pt.
  5. Several cleanups and add comments at the appropriate locations.
  6. Minimize the code for checking pass-thru ns across all the subsystems.
  7. Removed the delays in the ns related admin commands since I was
     not able to reproduce the previous bug.

v4 Changes:
  1. Add request polling interface to the block layer.
  2. Use request polling interface in the NVMEoF target passthru code
     path.
  3. Add checks suggested by Sagi for creating one target ctrl per
     passthru ctrl.
  4. Don't enable the namespace if it belongs to the configured passthru
     ctrl.
  5. Adjust the code latest kernel.

v3 Changes:
  1. Split the addition of passthru command handlers and integration
     into two different patches since we add guards to create one target
     controller per passthru controller. This way it will be easier to
     review the code.
  2. Adjust the code for 4.18.

v2 Changes:
  1. Update the new nvme core controller find API naming and
     changed the string comparison of the ctrl.
  2. Get rid of the newly added #defines for target ctrl values.
  3. Use the newly added structure members in the same patch where
     they are used. Aggregate the passthru command handling support
     and integration with nvmet-core into one patch.
  4. Introduce global NVMe Target subsystem list for connected and
     not connected subsystems on the target side.
  5. Add check when configuring the target ns and target
     passthru ctrl to allow only one target controller to be created
     for one passthru subsystem.
  6. Use the passthru ctrl cntlid when creating the
     target controller.

Chaitanya Kulkarni (4):
  nvme-core: export existing ctrl and ns interfaces
  nvmet: add return value to  nvmet_add_async_event()
  nvmet-passthru: add passthru code to process commands
  nvmet-core: don't check the data len for pt-ctrl

Logan Gunthorpe (8):
  nvme-core: introduce nvme_ctrl_get_by_path()
  nvmet: make nvmet_copy_ns_identifier() non-static
  nvmet-passthru: add enable/disable helpers
  nvmet-tcp: don't check data_len in nvmet_tcp_map_data()
  nvmet-configfs: introduce passthru configfs interface
  block: don't check blk_rq_is_passthrough() in blk_do_io_stat()
  block: call blk_account_io_start() in blk_execute_rq_nowait()
  nvmet-passthru: support block accounting

 block/blk-exec.c                      |   2 +
 block/blk-mq.c                        |   2 +-
 block/blk.h                           |   5 +-
 drivers/nvme/host/core.c              |  43 +-
 drivers/nvme/host/nvme.h              |   9 +
 drivers/nvme/target/Kconfig           |  10 +
 drivers/nvme/target/Makefile          |   1 +
 drivers/nvme/target/admin-cmd.c       |   4 +-
 drivers/nvme/target/configfs.c        |  99 ++++
 drivers/nvme/target/core.c            |  36 +-
 drivers/nvme/target/io-cmd-passthru.c | 647 ++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h           |  61 ++-
 drivers/nvme/target/tcp.c             |   2 +-
 include/linux/nvme.h                  |   1 +
 14 files changed, 903 insertions(+), 19 deletions(-)
 create mode 100644 drivers/nvme/target/io-cmd-passthru.c

--
2.20.1

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path()
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 22:13   ` Keith Busch
  2019-10-10 11:37   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces Logan Gunthorpe
                   ` (11 subsequent siblings)
  12 siblings, 2 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

nvme_ctrl_get_by_path() is analagous to blkdev_get_by_path() except it
gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0).
It makes use of filp_open() to open the file and uses the private
data to obtain a pointer to the struct nvme_ctrl. If the fops of the
file do not match, -EINVAL is returned.

The purpose of this function is to support NVMe-OF target passthru.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 24 ++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fd7dea36c3b6..30f9d91e25e3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2950,6 +2950,30 @@ static const struct file_operations nvme_dev_fops = {
 	.compat_ioctl	= nvme_dev_ioctl,
 };
 
+struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path)
+{
+	struct nvme_ctrl *ctrl;
+	struct file *f;
+
+	f = filp_open(path, O_RDWR, 0);
+	if (IS_ERR(f))
+		return ERR_CAST(f);
+
+	if (f->f_op != &nvme_dev_fops) {
+		ctrl = ERR_PTR(-EINVAL);
+		goto out_close;
+	}
+
+	ctrl = f->private_data;
+	nvme_get_ctrl(ctrl);
+
+out_close:
+	filp_close(f, NULL);
+
+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(nvme_ctrl_get_by_path);
+
 static ssize_t nvme_sysfs_reset(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 38a83ef5bcd3..f83f990e409d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -507,6 +507,8 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
 extern const struct attribute_group *nvme_ns_id_attr_groups[];
 extern const struct block_device_operations nvme_ns_head_ops;
 
+struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path);
+
 #ifdef CONFIG_NVME_MULTIPATH
 static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
 {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 22:14   ` Keith Busch
  2019-10-09 19:25 ` [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event() Logan Gunthorpe
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates,
	Chaitanya Kulkarni, Logan Gunthorpe

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

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@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 19 ++++++++++++-------
 drivers/nvme/host/nvme.h |  7 +++++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 30f9d91e25e3..c6303493a5b7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -422,10 +422,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);
 
 static inline void nvme_clear_nvme_request(struct request *req)
 {
@@ -1089,7 +1090,7 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
 				    NVME_IDENTIFY_DATA_SIZE);
 }
 
-static int nvme_identify_ns(struct nvme_ctrl *ctrl,
+int nvme_identify_ns(struct nvme_ctrl *ctrl,
 		unsigned nsid, struct nvme_id_ns **id)
 {
 	struct nvme_command c = { };
@@ -1112,6 +1113,7 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl,
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(nvme_identify_ns);
 
 static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
 		unsigned int dword11, void *buffer, size_t buflen, u32 *result)
@@ -1263,8 +1265,8 @@ static u32 nvme_known_admin_effects(u8 opcode)
 	return 0;
 }
 
-static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-								u8 opcode)
+u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+		u8 opcode)
 {
 	u32 effects = 0;
 
@@ -1296,6 +1298,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)
 {
@@ -1310,7 +1313,7 @@ static void nvme_update_formats(struct nvme_ctrl *ctrl)
 	nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
 }
 
-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
@@ -1330,6 +1333,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)
@@ -2844,7 +2848,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ret = nvme_configure_apst(ctrl);
 	if (ret < 0)
 		return ret;
-	
+
 	ret = nvme_configure_timestamp(ctrl);
 	if (ret < 0)
 		return ret;
@@ -3411,7 +3415,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;
 
@@ -3429,6 +3433,7 @@ static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	up_read(&ctrl->namespaces_rwsem);
 	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 f83f990e409d..31f42610e9bb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -459,6 +459,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 int nsid);
+void nvme_put_ns(struct nvme_ns *ns);
 
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
@@ -495,8 +497,13 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned int fid,
 int nvme_get_features(struct nvme_ctrl *dev, unsigned int fid,
 		      unsigned int dword11, void *buffer, size_t buflen,
 		      u32 *result);
+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_stop_keep_alive(struct nvme_ctrl *ctrl);
+int nvme_identify_ns(struct nvme_ctrl *ctrl,
+		unsigned nsid, struct nvme_id_ns **id);
 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.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 03/12] nvmet: add return value to  nvmet_add_async_event()
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 22:17   ` Keith Busch
  2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates,
	Chaitanya Kulkarni, Logan Gunthorpe

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Change the return value for nvmet_add_async_event().

This change is needed for the target passthru code which will
submit async events on namespaces changes and can fail the command
should adding the event fail (on -ENOMEM).

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
[logang@deltatee.com:
 * fleshed out commit message
 * change to using int as a return type instead of bool
]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/core.c  | 6 ++++--
 drivers/nvme/target/nvmet.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3a67e244e568..d6dcb86d8be7 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -173,14 +173,14 @@ static void nvmet_async_event_work(struct work_struct *work)
 	}
 }
 
-void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+int 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 -ENOMEM;
 
 	aen->event_type = event_type;
 	aen->event_info = event_info;
@@ -191,6 +191,8 @@ void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 	mutex_unlock(&ctrl->lock);
 
 	schedule_work(&ctrl->async_event_work);
+
+	return 0;
 }
 
 static void nvmet_add_to_changed_ns_log(struct nvmet_ctrl *ctrl, __le32 nsid)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd01dc4..3d313a6452cc 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -441,7 +441,7 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
 		struct nvmet_subsys *subsys);
 void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
 		struct nvmet_host *host);
-void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+int nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 		u8 event_info, u8 log_page);
 
 #define NVMET_QUEUE_SIZE	1024
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (2 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event() Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 22:18   ` Keith Busch
  2019-10-10 11:50   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com> Logan Gunthorpe
                   ` (8 subsequent siblings)
  12 siblings, 2 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe,
	Chaitanya Kulkarni

This function will be needed by the upcoming passthru code.

Passthru will need an emulated version of identify_desclist which
copies the eui64, uuid and nguid from the passed-thru controller into
the request SGL.

[chaitanya.kulkarni@wdc.com: this was factored out of a patch
 originally authored by Chaitanya]
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/admin-cmd.c | 4 ++--
 drivers/nvme/target/nvmet.h     | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 831a062d27cb..67b6642bb628 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -506,8 +506,8 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
-				    void *id, off_t *off)
+u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
+			     void *id, off_t *off)
 {
 	struct nvme_ns_id_desc desc = {
 		.nidt = type,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 3d313a6452cc..5dfd4e0ae096 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -489,6 +489,8 @@ u16 nvmet_bdev_flush(struct nvmet_req *req);
 u16 nvmet_file_flush(struct nvmet_req *req);
 void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
 
+u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
+			     void *id, off_t *off);
 static inline u32 nvmet_rw_len(struct nvmet_req *req)
 {
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) <<
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (3 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-10 12:34   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 05/12] nvmet-passthru: add passthru code to process commands Logan Gunthorpe
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates,
	Chaitanya Kulkarni, Logan Gunthorpe

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

nvmet-passthru: add passthru code to process commands

Add passthru command handling capability for the NVMeOF target and
export passthru APIs which are used to integrate passthru
code with nvmet-core. A passthru ns member is added to the target request
to hold the ns reference for respective commands.

The new file io-cmd-passthru.c handles passthru cmd parsing and execution.
In the passthru mode, we create a block layer request from the nvmet
request and map the data on to the block layer request. For handling the
side effects of the passthru admin commands we add two functions similar
to the nvme_passthru[start|end]() functions present in the nvme-core.
Only admin commands on a white list are let through which includes
vendor unique commands.

We introduce new passthru workqueue similar to the one we have for the
file backend for NVMeOF target to execute the NVMe Admin passthru
commands.

All the new passthrtu code is enabled or disabled by a new  KConfig option
for the NVMeOF target.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
[logang@deltatee.com:
   * renamed passthru-cmd.c to io-cmd-passthru.c for consistency
   * squashed "update target makefile for passthru"
   * squashed "integrate passthru request processing"
   * squashed "update KConfig with config passthru option"
   * added appropriate CONFIG_NVME_TARGET_PASSTHRU #ifdefs
   * pushed passthru_wq into passthrtu.c and introduced
     nvmet_passthru_init() and nvmet_passthru_destroy() to avoid
     inline #ifdef mess
   * renamed nvmet_passthru_ctrl() to nvmet_req_passthru_ctrl()
     and provided nvmet_passthr_ctrl() to get the ctrl from a subsys
   * fixed failure path in nvmet_passthru_execute_cmd() to ensure
     we always complete the request (with an error when appropriate)
   * restructered out nvmet_passthru_make_request() and
     nvmet_passthru_execute_cmd() to create nvmet_passthru_map_sg()
     which makes the code simpler and more readable.
   * move call to nvme_find_get_ns() into nvmet_passthru_execute_cmd()
     to prevent a lockdep error. nvme_find_get_ns() takes a
     lock and can sleep but nvme_init_req() is called while
     hctx_lock() is held (in the loop transport) and therefore
     should not sleep.
   * added check in nvmet_passthru_execute_cmd() to ensure we don't
     violate queue_max_segments or queue_max_hw_sectors.
   * added nvmet_passthru_set_mdts() to prevent requests that
     exceed max_segments
   * convert admin command blacklist to a white list with vendor
     unique commands specifically allowed
   * force setting cmic bit to support multipath for connections
     to the target
   * dropped le16_to_cpu() conversion in nvmet_passthru_req_done() as
     it's currently already done in nvme_end_request()
   * unabbreviated 'VUC' in a comment as it's not a commonly known
     acronym
   * removed unnecessary inline tags on static functions
   * minor edits to commit message
]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/Kconfig           |  10 +
 drivers/nvme/target/Makefile          |   1 +
 drivers/nvme/target/core.c            |  11 +-
 drivers/nvme/target/io-cmd-passthru.c | 567 ++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h           |  46 +++
 include/linux/nvme.h                  |   1 +
 6 files changed, 635 insertions(+), 1 deletion(-)
 create mode 100644 drivers/nvme/target/io-cmd-passthru.c

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index d7f48c0fb311..2478cb5a932d 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -15,6 +15,16 @@ config NVME_TARGET
 	  To configure the NVMe target you probably want to use the nvmetcli
 	  tool from http://git.infradead.org/users/hch/nvmetcli.git.
 
+config NVME_TARGET_PASSTHRU
+	bool "NVMe Target Passthrough support"
+	depends on NVME_CORE
+	depends on NVME_TARGET
+	help
+	  This enables target side NVMe passthru controller support for the
+	  NVMe Over Fabrics protocol. It allows for hosts to manage and
+	  directly access an actual NVMe controller residing on the target
+	  side, incuding executing Vendor Unique Commands.
+
 config NVME_TARGET_LOOP
 	tristate "NVMe loopback device support"
 	depends on NVME_TARGET
diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index 2b33836f3d3e..bf57799fde63 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NVME_TARGET_TCP)		+= nvmet-tcp.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o fabrics-cmd.o \
 			discovery.o io-cmd-file.o io-cmd-bdev.o
+nvmet-$(CONFIG_NVME_TARGET_PASSTHRU)	+= io-cmd-passthru.o
 nvme-loop-y	+= loop.o
 nvmet-rdma-y	+= rdma.o
 nvmet-fc-y	+= fc.o
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index d6dcb86d8be7..256f765e772b 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -896,6 +896,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_req_passthru_ctrl(req))
+		status = nvmet_parse_passthru_cmd(req);
 	else if (likely(req->sq->qid != 0))
 		status = nvmet_parse_io_cmd(req);
 	else if (nvme_is_fabrics(req->cmd))
@@ -1463,11 +1465,15 @@ static int __init nvmet_init(void)
 
 	nvmet_ana_group_enabled[NVMET_DEFAULT_ANA_GRPID] = 1;
 
+	error = nvmet_passthru_init();
+	if (error)
+		goto out;
+
 	buffered_io_wq = alloc_workqueue("nvmet-buffered-io-wq",
 			WQ_MEM_RECLAIM, 0);
 	if (!buffered_io_wq) {
 		error = -ENOMEM;
-		goto out;
+		goto out_passthru_destroy;
 	}
 
 	error = nvmet_init_discovery();
@@ -1483,6 +1489,8 @@ static int __init nvmet_init(void)
 	nvmet_exit_discovery();
 out_free_work_queue:
 	destroy_workqueue(buffered_io_wq);
+out_passthru_destroy:
+	nvmet_passthru_destroy();
 out:
 	return error;
 }
@@ -1493,6 +1501,7 @@ static void __exit nvmet_exit(void)
 	nvmet_exit_discovery();
 	ida_destroy(&cntlid_ida);
 	destroy_workqueue(buffered_io_wq);
+	nvmet_passthru_destroy();
 
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_entry) != 1024);
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_hdr) != 1024);
diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c
new file mode 100644
index 000000000000..1eb855b4071c
--- /dev/null
+++ b/drivers/nvme/target/io-cmd-passthru.c
@@ -0,0 +1,567 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVMe Over Fabrics Target Passthrough command implementation.
+ *
+ * Copyright (c) 2017-2018 Western Digital Corporation or its
+ * affiliates.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/module.h>
+
+#include "../host/nvme.h"
+#include "nvmet.h"
+
+static struct workqueue_struct *passthru_wq;
+
+int nvmet_passthru_init(void)
+{
+	passthru_wq = alloc_workqueue("nvmet-passthru-wq", WQ_MEM_RECLAIM, 0);
+	if (!passthru_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void nvmet_passthru_destroy(void)
+{
+	destroy_workqueue(passthru_wq);
+}
+
+static void nvmet_passthru_req_complete(struct nvmet_req *req,
+		struct request *rq, u16 status)
+{
+	nvmet_req_complete(req, status);
+
+	if (rq)
+		blk_put_request(rq);
+}
+
+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;
+
+	req->cqe->result.u32 = nvme_req(rq)->result.u32;
+
+	nvmet_passthru_req_complete(req, rq, status);
+}
+
+static u16 nvmet_passthru_override_format_nvm(struct nvmet_req *req)
+{
+	int lbaf = le32_to_cpu(req->cmd->format.cdw10) & 0x0000000F;
+	int nsid = le32_to_cpu(req->cmd->format.nsid);
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ns *id;
+	int ret;
+
+	ret = nvme_identify_ns(nvmet_req_passthru_ctrl(req), nsid, &id);
+	if (ret)
+		return NVME_SC_INTERNAL;
+	/*
+	 * XXX: Please update this code once NVMeOF target starts supporting
+	 * metadata. We don't support ns lba format with metadata over fabrics
+	 * right now, so report an 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 void nvmet_passthru_set_mdts(struct nvmet_ctrl *ctrl,
+				    struct nvme_id_ctrl *id)
+{
+	struct nvme_ctrl *pctrl = ctrl->subsys->passthru_ctrl;
+	u32 max_hw_sectors;
+	int page_shift;
+
+	/*
+	 * The passthru NVMe driver may have a limit on the number
+	 * of segments which depends on the host's memory fragementation.
+	 * To solve this, ensure mdts is limitted to the pages equal to
+	 * the number of segments.
+	 */
+
+	max_hw_sectors = min_not_zero(pctrl->max_segments << (PAGE_SHIFT - 9),
+				      pctrl->max_hw_sectors);
+
+	page_shift = NVME_CAP_MPSMIN(ctrl->cap) + 12;
+
+	id->mdts = ilog2(max_hw_sectors) + 9 - page_shift;
+}
+
+static u16 nvmet_passthru_override_id_ctrl(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ctrl *id;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(*id));
+	if (status)
+		goto out_free;
+
+	id->cntlid = cpu_to_le16(ctrl->cntlid);
+	id->ver = cpu_to_le32(ctrl->subsys->ver);
+
+	nvmet_passthru_set_mdts(ctrl, id);
+
+	id->acl = 3;
+	/*
+	 * We export aerl limit for the fabrics controller, update this when
+	 * passthru based aerl support is added.
+	 */
+	id->aerl = NVMET_ASYNC_EVENTS - 1;
+
+	/* 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, ((0x6 << 4) | 0x6), id->sqes);
+	id->cqes = min_t(__u8, ((0x4 << 4) | 0x4), 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 (req->port->inline_data_size)
+		id->sgls |= cpu_to_le32(1 << 20);
+
+	/*
+	 * When passsthru controller is setup using nvme-loop transport it will
+	 * export the passthru ctrl subsysnqn (PCIe NVMe ctrl) and will fail in
+	 * the nvme/host/core.c in the nvme_init_subsystem()->nvme_active_ctrl()
+	 * code path with duplicate ctr subsynqn. In order to prevent that we
+	 * mask the passthru-ctrl subsysnqn with the target ctrl subsysnqn.
+	 */
+	memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));
+
+	/* use fabric id-ctrl values */
+	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
+				req->port->inline_data_size) / 16);
+	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
+
+	id->msdbd = ctrl->ops->msdbd;
+
+	/* Support multipath connections with fabrics */
+	id->cmic |= 1 << 1;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(struct nvme_id_ctrl));
+
+out_free:
+	kfree(id);
+out:
+	return status;
+}
+
+static u16 nvmet_passthru_override_id_ns(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ns *id;
+	int i;
+
+	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;
+
+	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 u16 nvmet_passthru_fixup_identify(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->identify.cns) {
+	case NVME_ID_CNS_CTRL:
+		status = nvmet_passthru_override_id_ctrl(req);
+		break;
+	case NVME_ID_CNS_NS:
+		status = nvmet_passthru_override_id_ns(req);
+		break;
+	}
+	return status;
+}
+
+static u16 nvmet_passthru_admin_passthru_start(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_format_nvm:
+		status = nvmet_passthru_override_format_nvm(req);
+		break;
+	}
+	return status;
+}
+
+static u16 nvmet_passthru_admin_passthru_end(struct nvmet_req *req)
+{
+	u8 aer_type = NVME_AER_TYPE_NOTICE;
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_identify:
+		status = nvmet_passthru_fixup_identify(req);
+		break;
+	case nvme_admin_ns_mgmt:
+	case nvme_admin_ns_attach:
+	case nvme_admin_format_nvm:
+		if (nvmet_add_async_event(req->sq->ctrl, aer_type, 0, 0))
+			status = NVME_SC_INTERNAL;
+		break;
+	}
+	return status;
+}
+
+static void nvmet_passthru_execute_admin_cmd(struct nvmet_req *req)
+{
+	u8 opcode = req->cmd->common.opcode;
+	u32 effects;
+	u16 status;
+
+	status = nvmet_passthru_admin_passthru_start(req);
+	if (status)
+		goto out;
+
+	effects = nvme_passthru_start(nvmet_req_passthru_ctrl(req), NULL,
+				      opcode);
+
+	/*
+	 * Admin Commands have side effects and it is better to handle those
+	 * side effects in the submission thread context than in the request
+	 * completion path, which is in the interrupt context. Also in this
+	 * way, we keep the passhru admin command code path consistent with the
+	 * nvme/host/core.c sync command submission APIs/IOCTLs and use
+	 * nvme_passthru_start/end() to handle side effects consistently.
+	 */
+	blk_execute_rq(req->p.rq->q, NULL, req->p.rq, 0);
+
+	nvme_passthru_end(nvmet_req_passthru_ctrl(req), effects);
+	status = nvmet_passthru_admin_passthru_end(req);
+out:
+	if (status == NVME_SC_SUCCESS) {
+		nvmet_set_result(req, nvme_req(req->p.rq)->result.u32);
+		status = nvme_req(req->p.rq)->status;
+	}
+
+	nvmet_passthru_req_complete(req, req->p.rq, status);
+}
+
+static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
+{
+	int sg_cnt = req->sg_cnt;
+	struct scatterlist *sg;
+	int op = REQ_OP_READ;
+	int op_flags = 0;
+	struct bio *bio;
+	int i, ret;
+
+	if (req->cmd->common.opcode == nvme_cmd_flush) {
+		op_flags = REQ_FUA;
+	} else if (nvme_is_write(req->cmd)) {
+		op = REQ_OP_WRITE;
+		op_flags = REQ_SYNC | REQ_IDLE;
+	}
+
+	bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
+	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) {
+			ret = blk_rq_append_bio(rq, &bio);
+			if (unlikely(ret))
+				return ret;
+
+			bio_set_op_attrs(bio, op, op_flags);
+			bio = bio_alloc(GFP_KERNEL,
+					min(sg_cnt, BIO_MAX_PAGES));
+		}
+		sg_cnt--;
+	}
+
+	ret = blk_rq_append_bio(rq, &bio);
+	if (unlikely(ret))
+		return ret;
+
+	return 0;
+}
+
+static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req,
+		struct nvme_ns *ns, gfp_t gfp_mask)
+{
+	struct nvme_ctrl *passthru_ctrl = nvmet_req_passthru_ctrl(req);
+	struct nvme_command *cmd = req->cmd;
+	struct request_queue *q;
+	struct request *rq;
+	int ret;
+
+	if (ns)
+		q = ns->queue;
+	else
+		q = passthru_ctrl->admin_q;
+
+	rq = nvme_alloc_request(q, cmd, BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
+	if (unlikely(IS_ERR(rq)))
+		return rq;
+
+	if (req->sg_cnt) {
+		ret = nvmet_passthru_map_sg(req, rq);
+		if (unlikely(ret)) {
+			blk_put_request(rq);
+			return ERR_PTR(ret);
+		}
+	}
+
+	/*
+	 * We don't support fused cmds, also nvme-pci driver uses its own
+	 * sgl_threshold parameter to decide whether to use SGLs or PRPs hence
+	 * turn off those bits in the flags.
+	 */
+	req->cmd->common.flags &= ~(NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND |
+			NVME_CMD_SGL_ALL);
+	return rq;
+}
+
+
+static void nvmet_passthru_execute_admin_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
+
+	nvmet_passthru_execute_admin_cmd(req);
+}
+
+static void nvmet_passthru_submit_admin_cmd(struct nvmet_req *req)
+{
+	INIT_WORK(&req->p.work, nvmet_passthru_execute_admin_work);
+	queue_work(passthru_wq, &req->p.work);
+}
+
+static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
+{
+	struct request *rq = NULL;
+	struct nvme_ns *ns = NULL;
+	u16 status;
+
+	if (likely(req->sq->qid != 0)) {
+		u32 nsid = le32_to_cpu(req->cmd->common.nsid);
+
+		ns = nvme_find_get_ns(nvmet_req_passthru_ctrl(req), nsid);
+		if (unlikely(!ns)) {
+			pr_err("failed to get passthru ns nsid:%u\n", nsid);
+			status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+			goto fail_out;
+		}
+	}
+
+	rq = nvmet_passthru_blk_make_request(req, ns, GFP_KERNEL);
+	if (unlikely(IS_ERR(rq))) {
+		rq = NULL;
+		status = NVME_SC_INTERNAL;
+		goto fail_out;
+	}
+
+	if (unlikely(blk_rq_nr_phys_segments(rq) > queue_max_segments(rq->q) ||
+	    (blk_rq_payload_bytes(rq) >> 9) > queue_max_hw_sectors(rq->q))) {
+		status = NVME_SC_INVALID_FIELD;
+		goto fail_out;
+	}
+
+	rq->end_io_data = req;
+	if (req->sq->qid != 0) {
+		blk_execute_rq_nowait(rq->q, NULL, rq, 0,
+				      nvmet_passthru_req_done);
+	} else {
+		req->p.rq = rq;
+		nvmet_passthru_submit_admin_cmd(req);
+	}
+
+	if (ns)
+		nvme_put_ns(ns);
+
+	return;
+
+fail_out:
+	if (ns)
+		nvme_put_ns(ns);
+	nvmet_passthru_req_complete(req, rq, status);
+}
+
+/*
+ * We emulate commands which are not routed through the existing target
+ * code and not supported by the passthru ctrl. E.g consider a scenario where
+ * passthru ctrl version is < 1.3.0. Target Fabrics ctrl version is >= 1.3.0
+ * in that case in order to be fabrics compliant we need to emulate ns-desc-list
+ * command which is 1.3.0 compliant but not present for the passthru ctrl due
+ * to lower version.
+ */
+static void nvmet_passthru_emulate_id_desclist(struct nvmet_req *req)
+{
+	int nsid = le32_to_cpu(req->cmd->common.nsid);
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_ns_ids *ids;
+	struct nvme_ns *ns;
+	off_t off = 0;
+
+	ns = nvme_find_get_ns(nvmet_req_passthru_ctrl(req), nsid);
+	if (unlikely(!ns)) {
+		pr_err("failed to get passthru ns nsid:%u\n", nsid);
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+		goto out;
+	}
+	/*
+	 * Instead of refactoring and creating helpers, keep it simple and
+	 * just re-use the code from admin-cmd.c ->
+	 * nvmet_execute_identify_ns_desclist().
+	 */
+	ids = &ns->head->ids;
+	if (memchr_inv(ids->eui64, 0, sizeof(ids->eui64))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_EUI64,
+						  NVME_NIDT_EUI64_LEN,
+						  &ids->eui64, &off);
+		if (status)
+			goto out_put_ns;
+	}
+	if (memchr_inv(&ids->uuid, 0, sizeof(ids->uuid))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
+						  NVME_NIDT_UUID_LEN,
+						  &ids->uuid, &off);
+		if (status)
+			goto out_put_ns;
+	}
+	if (memchr_inv(ids->nguid, 0, sizeof(ids->nguid))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID,
+						  NVME_NIDT_NGUID_LEN,
+						  &ids->nguid, &off);
+		if (status)
+			goto out_put_ns;
+	}
+
+	if (sg_zero_buffer(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE - off,
+			off) != NVME_IDENTIFY_DATA_SIZE - off)
+		status = NVME_SC_INTERNAL | NVME_SC_DNR;
+out_put_ns:
+	nvme_put_ns(ns);
+out:
+	nvmet_req_complete(req, status);
+}
+
+/*
+ * In the passthru mode we support three types for commands:-
+ * 1. Commands which are black-listed.
+ * 2. Commands which are routed through target code.
+ * 3. Commands which are emulated in the target code, since we can't rely
+ *    on passthru-ctrl and cannot route through the target code.
+ */
+static u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
+{
+	struct nvme_command *cmd = req->cmd;
+	u16 status = 0;
+
+	if (cmd->common.opcode >= nvme_admin_vendor_unique_start) {
+		/*
+		 * Passthru all vendor unique commands
+		 */
+		req->execute = nvmet_passthru_execute_cmd;
+		return status;
+	}
+
+	switch (cmd->common.opcode) {
+	/* 2. commands which are routed through target code */
+	case nvme_admin_async_event:
+	/*
+	 * Right now we don't monitor any events for the passthru controller.
+	 * Instead generate asyn event notice for the ns-mgmt/format/attach
+	 * commands so that host can update it's ns-inventory.
+	 */
+		/* fallthru */
+	case nvme_admin_keep_alive:
+	/*
+	 * 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.
+	 */
+		status = nvmet_parse_admin_cmd(req);
+		break;
+	case nvme_admin_set_features:
+		switch (le32_to_cpu(req->cmd->features.fid)) {
+		case NVME_FEAT_ASYNC_EVENT:
+		case NVME_FEAT_KATO:
+		case NVME_FEAT_NUM_QUEUES:
+			status = nvmet_parse_admin_cmd(req);
+			break;
+		default:
+			req->execute = nvmet_passthru_execute_cmd;
+		}
+		break;
+	/* 3. commands which are emulated in the passthru code */
+	case nvme_admin_identify:
+		switch (req->cmd->identify.cns) {
+		case NVME_ID_CNS_NS_DESC_LIST:
+			req->execute = nvmet_passthru_emulate_id_desclist;
+			break;
+		default:
+			req->execute = nvmet_passthru_execute_cmd;
+		}
+		break;
+	/* 4. By default, blacklist all admin commands */
+	default:
+
+		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		req->execute = NULL;
+		break;
+	}
+
+	return status;
+}
+
+u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
+{
+	int ret;
+
+	if (unlikely(req->cmd->common.opcode == nvme_fabrics_command))
+		return nvmet_parse_fabrics_cmd(req);
+	else if (unlikely(req->sq->ctrl->subsys->type == NVME_NQN_DISC))
+		return nvmet_parse_discovery_cmd(req);
+
+	ret = nvmet_check_ctrl_status(req, req->cmd);
+	if (unlikely(ret))
+		return ret;
+
+	if (unlikely(req->sq->qid == 0))
+		return nvmet_parse_passthru_admin_cmd(req);
+
+	req->execute = nvmet_passthru_execute_cmd;
+	return NVME_SC_SUCCESS;
+}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5dfd4e0ae096..daec1240307c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -227,6 +227,10 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+	struct nvme_ctrl	*passthru_ctrl;
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
@@ -302,6 +306,10 @@ struct nvmet_req {
 			struct bio_vec          *bvec;
 			struct work_struct      work;
 		} f;
+		struct {
+			struct request		*rq;
+			struct work_struct      work;
+		} p;
 	};
 	int			sg_cnt;
 	/* data length as parsed from the command: */
@@ -497,6 +505,44 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 			req->ns->blksize_shift;
 }
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+
+int nvmet_passthru_init(void);
+void nvmet_passthru_destroy(void);
+u16 nvmet_parse_passthru_cmd(struct nvmet_req *req);
+
+static inline
+struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return subsys->passthru_ctrl;
+}
+
+#else /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static inline int nvmet_passthru_init(void)
+{
+	return 0;
+}
+static inline void nvmet_passthru_destroy(void)
+{
+}
+static inline u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
+{
+	return 0;
+}
+static inline
+struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static inline struct nvme_ctrl *nvmet_req_passthru_ctrl(struct nvmet_req *req)
+{
+	return nvmet_passthru_ctrl(req->sq->ctrl->subsys);
+}
+
 u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
 
 /* Convert a 32-bit number to a 16-bit 0's based number */
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index f61d6906e59d..94e730b5d0a3 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -816,6 +816,7 @@ enum nvme_admin_opcode {
 	nvme_admin_security_recv	= 0x82,
 	nvme_admin_sanitize_nvm		= 0x84,
 	nvme_admin_get_lba_status	= 0x86,
+	nvme_admin_vendor_unique_start	= 0xC0,
 };
 
 #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 05/12] nvmet-passthru: add passthru code to process commands
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (4 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com> Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 06/12] nvmet-passthru: add enable/disable helpers Logan Gunthorpe
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates,
	Chaitanya Kulkarni, Logan Gunthorpe

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Add passthru command handling capability for the NVMeOF target and
export passthru APIs which are used to integrate passthru
code with nvmet-core. A passthru ns member is added to the target request
to hold the ns reference for respective commands.

The new file io-cmd-passthru.c handles passthru cmd parsing and execution.
In the passthru mode, we create a block layer request from the nvmet
request and map the data on to the block layer request. For handling the
side effects of the passthru admin commands we add two functions similar
to the nvme_passthru[start|end]() functions present in the nvme-core.
Only admin commands on a white list are let through which includes
vendor unique commands.

We introduce new passthru workqueue similar to the one we have for the
file backend for NVMeOF target to execute the NVMe Admin passthru
commands.

All the new passthrtu code is enabled or disabled by a new  KConfig option
for the NVMeOF target.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
[logang@deltatee.com:
   * renamed passthru-cmd.c to io-cmd-passthru.c for consistency
   * squashed "update target makefile for passthru"
   * squashed "integrate passthru request processing"
   * squashed "update KConfig with config passthru option"
   * added appropriate CONFIG_NVME_TARGET_PASSTHRU #ifdefs
   * pushed passthru_wq into passthrtu.c and introduced
     nvmet_passthru_init() and nvmet_passthru_destroy() to avoid
     inline #ifdef mess
   * renamed nvmet_passthru_ctrl() to nvmet_req_passthru_ctrl()
     and provided nvmet_passthr_ctrl() to get the ctrl from a subsys
   * fixed failure path in nvmet_passthru_execute_cmd() to ensure
     we always complete the request (with an error when appropriate)
   * restructered out nvmet_passthru_make_request() and
     nvmet_passthru_execute_cmd() to create nvmet_passthru_map_sg()
     which makes the code simpler and more readable.
   * move call to nvme_find_get_ns() into nvmet_passthru_execute_cmd()
     to prevent a lockdep error. nvme_find_get_ns() takes a
     lock and can sleep but nvme_init_req() is called while
     hctx_lock() is held (in the loop transport) and therefore
     should not sleep.
   * added check in nvmet_passthru_execute_cmd() to ensure we don't
     violate queue_max_segments or queue_max_hw_sectors.
   * added nvmet_passthru_set_mdts() to prevent requests that
     exceed max_segments
   * convert admin command blacklist to a white list with vendor
     unique commands specifically allowed
   * force setting cmic bit to support multipath for connections
     to the target
   * dropped le16_to_cpu() conversion in nvmet_passthru_req_done() as
     it's currently already done in nvme_end_request()
   * unabbreviated 'VUC' in a comment as it's not a commonly known
     acronym
   * removed unnecessary inline tags on static functions
   * minor edits to commit message
]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/Kconfig           |  10 +
 drivers/nvme/target/Makefile          |   1 +
 drivers/nvme/target/core.c            |  11 +-
 drivers/nvme/target/io-cmd-passthru.c | 567 ++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h           |  46 +++
 include/linux/nvme.h                  |   1 +
 6 files changed, 635 insertions(+), 1 deletion(-)
 create mode 100644 drivers/nvme/target/io-cmd-passthru.c

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index d7f48c0fb311..2478cb5a932d 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -15,6 +15,16 @@ config NVME_TARGET
 	  To configure the NVMe target you probably want to use the nvmetcli
 	  tool from http://git.infradead.org/users/hch/nvmetcli.git.
 
+config NVME_TARGET_PASSTHRU
+	bool "NVMe Target Passthrough support"
+	depends on NVME_CORE
+	depends on NVME_TARGET
+	help
+	  This enables target side NVMe passthru controller support for the
+	  NVMe Over Fabrics protocol. It allows for hosts to manage and
+	  directly access an actual NVMe controller residing on the target
+	  side, incuding executing Vendor Unique Commands.
+
 config NVME_TARGET_LOOP
 	tristate "NVMe loopback device support"
 	depends on NVME_TARGET
diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index 2b33836f3d3e..bf57799fde63 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NVME_TARGET_TCP)		+= nvmet-tcp.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o fabrics-cmd.o \
 			discovery.o io-cmd-file.o io-cmd-bdev.o
+nvmet-$(CONFIG_NVME_TARGET_PASSTHRU)	+= io-cmd-passthru.o
 nvme-loop-y	+= loop.o
 nvmet-rdma-y	+= rdma.o
 nvmet-fc-y	+= fc.o
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index d6dcb86d8be7..256f765e772b 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -896,6 +896,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_req_passthru_ctrl(req))
+		status = nvmet_parse_passthru_cmd(req);
 	else if (likely(req->sq->qid != 0))
 		status = nvmet_parse_io_cmd(req);
 	else if (nvme_is_fabrics(req->cmd))
@@ -1463,11 +1465,15 @@ static int __init nvmet_init(void)
 
 	nvmet_ana_group_enabled[NVMET_DEFAULT_ANA_GRPID] = 1;
 
+	error = nvmet_passthru_init();
+	if (error)
+		goto out;
+
 	buffered_io_wq = alloc_workqueue("nvmet-buffered-io-wq",
 			WQ_MEM_RECLAIM, 0);
 	if (!buffered_io_wq) {
 		error = -ENOMEM;
-		goto out;
+		goto out_passthru_destroy;
 	}
 
 	error = nvmet_init_discovery();
@@ -1483,6 +1489,8 @@ static int __init nvmet_init(void)
 	nvmet_exit_discovery();
 out_free_work_queue:
 	destroy_workqueue(buffered_io_wq);
+out_passthru_destroy:
+	nvmet_passthru_destroy();
 out:
 	return error;
 }
@@ -1493,6 +1501,7 @@ static void __exit nvmet_exit(void)
 	nvmet_exit_discovery();
 	ida_destroy(&cntlid_ida);
 	destroy_workqueue(buffered_io_wq);
+	nvmet_passthru_destroy();
 
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_entry) != 1024);
 	BUILD_BUG_ON(sizeof(struct nvmf_disc_rsp_page_hdr) != 1024);
diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c
new file mode 100644
index 000000000000..1eb855b4071c
--- /dev/null
+++ b/drivers/nvme/target/io-cmd-passthru.c
@@ -0,0 +1,567 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVMe Over Fabrics Target Passthrough command implementation.
+ *
+ * Copyright (c) 2017-2018 Western Digital Corporation or its
+ * affiliates.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/module.h>
+
+#include "../host/nvme.h"
+#include "nvmet.h"
+
+static struct workqueue_struct *passthru_wq;
+
+int nvmet_passthru_init(void)
+{
+	passthru_wq = alloc_workqueue("nvmet-passthru-wq", WQ_MEM_RECLAIM, 0);
+	if (!passthru_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void nvmet_passthru_destroy(void)
+{
+	destroy_workqueue(passthru_wq);
+}
+
+static void nvmet_passthru_req_complete(struct nvmet_req *req,
+		struct request *rq, u16 status)
+{
+	nvmet_req_complete(req, status);
+
+	if (rq)
+		blk_put_request(rq);
+}
+
+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;
+
+	req->cqe->result.u32 = nvme_req(rq)->result.u32;
+
+	nvmet_passthru_req_complete(req, rq, status);
+}
+
+static u16 nvmet_passthru_override_format_nvm(struct nvmet_req *req)
+{
+	int lbaf = le32_to_cpu(req->cmd->format.cdw10) & 0x0000000F;
+	int nsid = le32_to_cpu(req->cmd->format.nsid);
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ns *id;
+	int ret;
+
+	ret = nvme_identify_ns(nvmet_req_passthru_ctrl(req), nsid, &id);
+	if (ret)
+		return NVME_SC_INTERNAL;
+	/*
+	 * XXX: Please update this code once NVMeOF target starts supporting
+	 * metadata. We don't support ns lba format with metadata over fabrics
+	 * right now, so report an 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 void nvmet_passthru_set_mdts(struct nvmet_ctrl *ctrl,
+				    struct nvme_id_ctrl *id)
+{
+	struct nvme_ctrl *pctrl = ctrl->subsys->passthru_ctrl;
+	u32 max_hw_sectors;
+	int page_shift;
+
+	/*
+	 * The passthru NVMe driver may have a limit on the number
+	 * of segments which depends on the host's memory fragementation.
+	 * To solve this, ensure mdts is limitted to the pages equal to
+	 * the number of segments.
+	 */
+
+	max_hw_sectors = min_not_zero(pctrl->max_segments << (PAGE_SHIFT - 9),
+				      pctrl->max_hw_sectors);
+
+	page_shift = NVME_CAP_MPSMIN(ctrl->cap) + 12;
+
+	id->mdts = ilog2(max_hw_sectors) + 9 - page_shift;
+}
+
+static u16 nvmet_passthru_override_id_ctrl(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ctrl *id;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(*id));
+	if (status)
+		goto out_free;
+
+	id->cntlid = cpu_to_le16(ctrl->cntlid);
+	id->ver = cpu_to_le32(ctrl->subsys->ver);
+
+	nvmet_passthru_set_mdts(ctrl, id);
+
+	id->acl = 3;
+	/*
+	 * We export aerl limit for the fabrics controller, update this when
+	 * passthru based aerl support is added.
+	 */
+	id->aerl = NVMET_ASYNC_EVENTS - 1;
+
+	/* 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, ((0x6 << 4) | 0x6), id->sqes);
+	id->cqes = min_t(__u8, ((0x4 << 4) | 0x4), 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 (req->port->inline_data_size)
+		id->sgls |= cpu_to_le32(1 << 20);
+
+	/*
+	 * When passsthru controller is setup using nvme-loop transport it will
+	 * export the passthru ctrl subsysnqn (PCIe NVMe ctrl) and will fail in
+	 * the nvme/host/core.c in the nvme_init_subsystem()->nvme_active_ctrl()
+	 * code path with duplicate ctr subsynqn. In order to prevent that we
+	 * mask the passthru-ctrl subsysnqn with the target ctrl subsysnqn.
+	 */
+	memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));
+
+	/* use fabric id-ctrl values */
+	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
+				req->port->inline_data_size) / 16);
+	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
+
+	id->msdbd = ctrl->ops->msdbd;
+
+	/* Support multipath connections with fabrics */
+	id->cmic |= 1 << 1;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(struct nvme_id_ctrl));
+
+out_free:
+	kfree(id);
+out:
+	return status;
+}
+
+static u16 nvmet_passthru_override_id_ns(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ns *id;
+	int i;
+
+	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;
+
+	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 u16 nvmet_passthru_fixup_identify(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->identify.cns) {
+	case NVME_ID_CNS_CTRL:
+		status = nvmet_passthru_override_id_ctrl(req);
+		break;
+	case NVME_ID_CNS_NS:
+		status = nvmet_passthru_override_id_ns(req);
+		break;
+	}
+	return status;
+}
+
+static u16 nvmet_passthru_admin_passthru_start(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_format_nvm:
+		status = nvmet_passthru_override_format_nvm(req);
+		break;
+	}
+	return status;
+}
+
+static u16 nvmet_passthru_admin_passthru_end(struct nvmet_req *req)
+{
+	u8 aer_type = NVME_AER_TYPE_NOTICE;
+	u16 status = NVME_SC_SUCCESS;
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_identify:
+		status = nvmet_passthru_fixup_identify(req);
+		break;
+	case nvme_admin_ns_mgmt:
+	case nvme_admin_ns_attach:
+	case nvme_admin_format_nvm:
+		if (nvmet_add_async_event(req->sq->ctrl, aer_type, 0, 0))
+			status = NVME_SC_INTERNAL;
+		break;
+	}
+	return status;
+}
+
+static void nvmet_passthru_execute_admin_cmd(struct nvmet_req *req)
+{
+	u8 opcode = req->cmd->common.opcode;
+	u32 effects;
+	u16 status;
+
+	status = nvmet_passthru_admin_passthru_start(req);
+	if (status)
+		goto out;
+
+	effects = nvme_passthru_start(nvmet_req_passthru_ctrl(req), NULL,
+				      opcode);
+
+	/*
+	 * Admin Commands have side effects and it is better to handle those
+	 * side effects in the submission thread context than in the request
+	 * completion path, which is in the interrupt context. Also in this
+	 * way, we keep the passhru admin command code path consistent with the
+	 * nvme/host/core.c sync command submission APIs/IOCTLs and use
+	 * nvme_passthru_start/end() to handle side effects consistently.
+	 */
+	blk_execute_rq(req->p.rq->q, NULL, req->p.rq, 0);
+
+	nvme_passthru_end(nvmet_req_passthru_ctrl(req), effects);
+	status = nvmet_passthru_admin_passthru_end(req);
+out:
+	if (status == NVME_SC_SUCCESS) {
+		nvmet_set_result(req, nvme_req(req->p.rq)->result.u32);
+		status = nvme_req(req->p.rq)->status;
+	}
+
+	nvmet_passthru_req_complete(req, req->p.rq, status);
+}
+
+static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
+{
+	int sg_cnt = req->sg_cnt;
+	struct scatterlist *sg;
+	int op = REQ_OP_READ;
+	int op_flags = 0;
+	struct bio *bio;
+	int i, ret;
+
+	if (req->cmd->common.opcode == nvme_cmd_flush) {
+		op_flags = REQ_FUA;
+	} else if (nvme_is_write(req->cmd)) {
+		op = REQ_OP_WRITE;
+		op_flags = REQ_SYNC | REQ_IDLE;
+	}
+
+	bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
+	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) {
+			ret = blk_rq_append_bio(rq, &bio);
+			if (unlikely(ret))
+				return ret;
+
+			bio_set_op_attrs(bio, op, op_flags);
+			bio = bio_alloc(GFP_KERNEL,
+					min(sg_cnt, BIO_MAX_PAGES));
+		}
+		sg_cnt--;
+	}
+
+	ret = blk_rq_append_bio(rq, &bio);
+	if (unlikely(ret))
+		return ret;
+
+	return 0;
+}
+
+static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req,
+		struct nvme_ns *ns, gfp_t gfp_mask)
+{
+	struct nvme_ctrl *passthru_ctrl = nvmet_req_passthru_ctrl(req);
+	struct nvme_command *cmd = req->cmd;
+	struct request_queue *q;
+	struct request *rq;
+	int ret;
+
+	if (ns)
+		q = ns->queue;
+	else
+		q = passthru_ctrl->admin_q;
+
+	rq = nvme_alloc_request(q, cmd, BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
+	if (unlikely(IS_ERR(rq)))
+		return rq;
+
+	if (req->sg_cnt) {
+		ret = nvmet_passthru_map_sg(req, rq);
+		if (unlikely(ret)) {
+			blk_put_request(rq);
+			return ERR_PTR(ret);
+		}
+	}
+
+	/*
+	 * We don't support fused cmds, also nvme-pci driver uses its own
+	 * sgl_threshold parameter to decide whether to use SGLs or PRPs hence
+	 * turn off those bits in the flags.
+	 */
+	req->cmd->common.flags &= ~(NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND |
+			NVME_CMD_SGL_ALL);
+	return rq;
+}
+
+
+static void nvmet_passthru_execute_admin_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
+
+	nvmet_passthru_execute_admin_cmd(req);
+}
+
+static void nvmet_passthru_submit_admin_cmd(struct nvmet_req *req)
+{
+	INIT_WORK(&req->p.work, nvmet_passthru_execute_admin_work);
+	queue_work(passthru_wq, &req->p.work);
+}
+
+static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
+{
+	struct request *rq = NULL;
+	struct nvme_ns *ns = NULL;
+	u16 status;
+
+	if (likely(req->sq->qid != 0)) {
+		u32 nsid = le32_to_cpu(req->cmd->common.nsid);
+
+		ns = nvme_find_get_ns(nvmet_req_passthru_ctrl(req), nsid);
+		if (unlikely(!ns)) {
+			pr_err("failed to get passthru ns nsid:%u\n", nsid);
+			status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+			goto fail_out;
+		}
+	}
+
+	rq = nvmet_passthru_blk_make_request(req, ns, GFP_KERNEL);
+	if (unlikely(IS_ERR(rq))) {
+		rq = NULL;
+		status = NVME_SC_INTERNAL;
+		goto fail_out;
+	}
+
+	if (unlikely(blk_rq_nr_phys_segments(rq) > queue_max_segments(rq->q) ||
+	    (blk_rq_payload_bytes(rq) >> 9) > queue_max_hw_sectors(rq->q))) {
+		status = NVME_SC_INVALID_FIELD;
+		goto fail_out;
+	}
+
+	rq->end_io_data = req;
+	if (req->sq->qid != 0) {
+		blk_execute_rq_nowait(rq->q, NULL, rq, 0,
+				      nvmet_passthru_req_done);
+	} else {
+		req->p.rq = rq;
+		nvmet_passthru_submit_admin_cmd(req);
+	}
+
+	if (ns)
+		nvme_put_ns(ns);
+
+	return;
+
+fail_out:
+	if (ns)
+		nvme_put_ns(ns);
+	nvmet_passthru_req_complete(req, rq, status);
+}
+
+/*
+ * We emulate commands which are not routed through the existing target
+ * code and not supported by the passthru ctrl. E.g consider a scenario where
+ * passthru ctrl version is < 1.3.0. Target Fabrics ctrl version is >= 1.3.0
+ * in that case in order to be fabrics compliant we need to emulate ns-desc-list
+ * command which is 1.3.0 compliant but not present for the passthru ctrl due
+ * to lower version.
+ */
+static void nvmet_passthru_emulate_id_desclist(struct nvmet_req *req)
+{
+	int nsid = le32_to_cpu(req->cmd->common.nsid);
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_ns_ids *ids;
+	struct nvme_ns *ns;
+	off_t off = 0;
+
+	ns = nvme_find_get_ns(nvmet_req_passthru_ctrl(req), nsid);
+	if (unlikely(!ns)) {
+		pr_err("failed to get passthru ns nsid:%u\n", nsid);
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+		goto out;
+	}
+	/*
+	 * Instead of refactoring and creating helpers, keep it simple and
+	 * just re-use the code from admin-cmd.c ->
+	 * nvmet_execute_identify_ns_desclist().
+	 */
+	ids = &ns->head->ids;
+	if (memchr_inv(ids->eui64, 0, sizeof(ids->eui64))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_EUI64,
+						  NVME_NIDT_EUI64_LEN,
+						  &ids->eui64, &off);
+		if (status)
+			goto out_put_ns;
+	}
+	if (memchr_inv(&ids->uuid, 0, sizeof(ids->uuid))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
+						  NVME_NIDT_UUID_LEN,
+						  &ids->uuid, &off);
+		if (status)
+			goto out_put_ns;
+	}
+	if (memchr_inv(ids->nguid, 0, sizeof(ids->nguid))) {
+		status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID,
+						  NVME_NIDT_NGUID_LEN,
+						  &ids->nguid, &off);
+		if (status)
+			goto out_put_ns;
+	}
+
+	if (sg_zero_buffer(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE - off,
+			off) != NVME_IDENTIFY_DATA_SIZE - off)
+		status = NVME_SC_INTERNAL | NVME_SC_DNR;
+out_put_ns:
+	nvme_put_ns(ns);
+out:
+	nvmet_req_complete(req, status);
+}
+
+/*
+ * In the passthru mode we support three types for commands:-
+ * 1. Commands which are black-listed.
+ * 2. Commands which are routed through target code.
+ * 3. Commands which are emulated in the target code, since we can't rely
+ *    on passthru-ctrl and cannot route through the target code.
+ */
+static u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
+{
+	struct nvme_command *cmd = req->cmd;
+	u16 status = 0;
+
+	if (cmd->common.opcode >= nvme_admin_vendor_unique_start) {
+		/*
+		 * Passthru all vendor unique commands
+		 */
+		req->execute = nvmet_passthru_execute_cmd;
+		return status;
+	}
+
+	switch (cmd->common.opcode) {
+	/* 2. commands which are routed through target code */
+	case nvme_admin_async_event:
+	/*
+	 * Right now we don't monitor any events for the passthru controller.
+	 * Instead generate asyn event notice for the ns-mgmt/format/attach
+	 * commands so that host can update it's ns-inventory.
+	 */
+		/* fallthru */
+	case nvme_admin_keep_alive:
+	/*
+	 * 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.
+	 */
+		status = nvmet_parse_admin_cmd(req);
+		break;
+	case nvme_admin_set_features:
+		switch (le32_to_cpu(req->cmd->features.fid)) {
+		case NVME_FEAT_ASYNC_EVENT:
+		case NVME_FEAT_KATO:
+		case NVME_FEAT_NUM_QUEUES:
+			status = nvmet_parse_admin_cmd(req);
+			break;
+		default:
+			req->execute = nvmet_passthru_execute_cmd;
+		}
+		break;
+	/* 3. commands which are emulated in the passthru code */
+	case nvme_admin_identify:
+		switch (req->cmd->identify.cns) {
+		case NVME_ID_CNS_NS_DESC_LIST:
+			req->execute = nvmet_passthru_emulate_id_desclist;
+			break;
+		default:
+			req->execute = nvmet_passthru_execute_cmd;
+		}
+		break;
+	/* 4. By default, blacklist all admin commands */
+	default:
+
+		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		req->execute = NULL;
+		break;
+	}
+
+	return status;
+}
+
+u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
+{
+	int ret;
+
+	if (unlikely(req->cmd->common.opcode == nvme_fabrics_command))
+		return nvmet_parse_fabrics_cmd(req);
+	else if (unlikely(req->sq->ctrl->subsys->type == NVME_NQN_DISC))
+		return nvmet_parse_discovery_cmd(req);
+
+	ret = nvmet_check_ctrl_status(req, req->cmd);
+	if (unlikely(ret))
+		return ret;
+
+	if (unlikely(req->sq->qid == 0))
+		return nvmet_parse_passthru_admin_cmd(req);
+
+	req->execute = nvmet_passthru_execute_cmd;
+	return NVME_SC_SUCCESS;
+}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5dfd4e0ae096..daec1240307c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -227,6 +227,10 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+	struct nvme_ctrl	*passthru_ctrl;
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
@@ -302,6 +306,10 @@ struct nvmet_req {
 			struct bio_vec          *bvec;
 			struct work_struct      work;
 		} f;
+		struct {
+			struct request		*rq;
+			struct work_struct      work;
+		} p;
 	};
 	int			sg_cnt;
 	/* data length as parsed from the command: */
@@ -497,6 +505,44 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 			req->ns->blksize_shift;
 }
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+
+int nvmet_passthru_init(void);
+void nvmet_passthru_destroy(void);
+u16 nvmet_parse_passthru_cmd(struct nvmet_req *req);
+
+static inline
+struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return subsys->passthru_ctrl;
+}
+
+#else /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static inline int nvmet_passthru_init(void)
+{
+	return 0;
+}
+static inline void nvmet_passthru_destroy(void)
+{
+}
+static inline u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
+{
+	return 0;
+}
+static inline
+struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static inline struct nvme_ctrl *nvmet_req_passthru_ctrl(struct nvmet_req *req)
+{
+	return nvmet_passthru_ctrl(req->sq->ctrl->subsys);
+}
+
 u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
 
 /* Convert a 32-bit number to a 16-bit 0's based number */
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index f61d6906e59d..94e730b5d0a3 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -816,6 +816,7 @@ enum nvme_admin_opcode {
 	nvme_admin_security_recv	= 0x82,
 	nvme_admin_sanitize_nvm		= 0x84,
 	nvme_admin_get_lba_status	= 0x86,
+	nvme_admin_vendor_unique_start	= 0xC0,
 };
 
 #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 06/12] nvmet-passthru: add enable/disable helpers
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (5 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 05/12] nvmet-passthru: add passthru code to process commands Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl Logan Gunthorpe
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe,
	Chaitanya Kulkarni

This patch adds helper functions which are used in the NVMeOF configfs
when the user is configuring the passthru subsystem. Here we ensure
that only one subsys is assigned to each nvme_ctrl by using an xarray
on the cntlid.

[chaitanya.kulkarni@wdc.com: this patch is very roughly based
 on a similar one by Chaitanya]
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/core.c            |  8 +++
 drivers/nvme/target/io-cmd-passthru.c | 77 +++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h           | 10 ++++
 3 files changed, 95 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 256f765e772b..986b2511d284 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -520,6 +520,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 
 	mutex_lock(&subsys->lock);
 	ret = 0;
+
+	if (nvmet_passthru_ctrl(subsys)) {
+		pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+		goto out_unlock;
+	}
+
 	if (ns->enabled)
 		goto out_unlock;
 
@@ -1440,6 +1446,8 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
+	nvmet_passthru_subsys_free(subsys);
+
 	kfree(subsys->subsysnqn);
 	kfree(subsys);
 }
diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c
index 1eb855b4071c..c482e55f0fb8 100644
--- a/drivers/nvme/target/io-cmd-passthru.c
+++ b/drivers/nvme/target/io-cmd-passthru.c
@@ -11,6 +11,11 @@
 #include "../host/nvme.h"
 #include "nvmet.h"
 
+/*
+ * xarray to maintain one passthru subsystem per nvme controller.
+ */
+static DEFINE_XARRAY(passthru_subsystems);
+
 static struct workqueue_struct *passthru_wq;
 
 int nvmet_passthru_init(void)
@@ -27,6 +32,78 @@ void nvmet_passthru_destroy(void)
 	destroy_workqueue(passthru_wq);
 }
 
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
+{
+	struct nvme_ctrl *ctrl;
+	int ret = -EINVAL;
+	void *old;
+
+	mutex_lock(&subsys->lock);
+	if (!subsys->passthru_ctrl_path)
+		goto out_unlock;
+	if (subsys->passthru_ctrl)
+		goto out_unlock;
+
+	if (subsys->nr_namespaces) {
+		pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+		goto out_unlock;
+	}
+
+	ctrl = nvme_ctrl_get_by_path(subsys->passthru_ctrl_path);
+	if (IS_ERR(ctrl)) {
+		ret = PTR_ERR(ctrl);
+		pr_err("failed to open nvme controller %s\n",
+		       subsys->passthru_ctrl_path);
+
+		goto out_unlock;
+	}
+
+	old = xa_cmpxchg(&passthru_subsystems, ctrl->cntlid, NULL,
+			 subsys, GFP_KERNEL);
+	if (xa_is_err(old)) {
+		ret = xa_err(old);
+		goto out_put_ctrl;
+	}
+
+	if (old)
+		goto out_put_ctrl;
+
+	subsys->passthru_ctrl = ctrl;
+
+	mutex_unlock(&subsys->lock);
+	return 0;
+
+out_put_ctrl:
+	nvme_put_ctrl(ctrl);
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
+}
+
+static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+	if (subsys->passthru_ctrl) {
+		xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
+		nvme_put_ctrl(subsys->passthru_ctrl);
+	}
+	subsys->passthru_ctrl = NULL;
+}
+
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+	mutex_lock(&subsys->lock);
+	__nvmet_passthru_ctrl_disable(subsys);
+	mutex_unlock(&subsys->lock);
+}
+
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+	mutex_lock(&subsys->lock);
+	__nvmet_passthru_ctrl_disable(subsys);
+	kfree(subsys->passthru_ctrl_path);
+	mutex_unlock(&subsys->lock);
+}
+
 static void nvmet_passthru_req_complete(struct nvmet_req *req,
 		struct request *rq, u16 status)
 {
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index daec1240307c..2c287d13ed83 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -230,6 +230,7 @@ struct nvmet_subsys {
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
+	char			*passthru_ctrl_path;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
@@ -509,6 +510,9 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 
 int nvmet_passthru_init(void);
 void nvmet_passthru_destroy(void);
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys);
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys);
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys);
 u16 nvmet_parse_passthru_cmd(struct nvmet_req *req);
 
 static inline
@@ -526,6 +530,12 @@ static inline int nvmet_passthru_init(void)
 static inline void nvmet_passthru_destroy(void)
 {
 }
+static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+}
+static inline void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+}
 static inline u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
 {
 	return 0;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (6 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 06/12] nvmet-passthru: add enable/disable helpers Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-10 11:04   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data() Logan Gunthorpe
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates,
	Chaitanya Kulkarni, Logan Gunthorpe

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Right now, data_len is calculated before the transfer len after we
parse the command, With passthru interface we allow VUCs (Vendor-Unique
Commands). In order to make the code simple and compact, instead of
assigning the data len or each VUC in the command parse function
just use the transfer len as it is. This may result in error if expected
data_len != transfer_len.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
[logang@deltatee.com:
   * added definition of VUC to the commit message and comment
   * use nvmet_req_passthru_ctrl() helper seeing we can't dereference
     subsys->passthru_ctrl if CONFIG_NVME_TARGET_PASSTHRU is not set]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 986b2511d284..f9d46354f9ae 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -942,7 +942,16 @@ EXPORT_SYMBOL_GPL(nvmet_req_uninit);
 
 void nvmet_req_execute(struct nvmet_req *req)
 {
-	if (unlikely(req->data_len != req->transfer_len)) {
+	/*
+	 * data_len is calculated before the transfer len after we parse
+	 * the command, With passthru interface we allow VUC (Vendor-Unique
+	 * Commands)'s. In order to make the code simple and compact,
+	 * instead of assinging the dala len for each VUC in the command
+	 * parse function just use the transfer len as it is. This may
+	 * result in error if expected data_len != transfer_len.
+	 */
+	if (!(req->sq->ctrl && nvmet_req_passthru_ctrl(req)) &&
+	    unlikely(req->data_len != req->transfer_len)) {
 		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		nvmet_req_complete(req, NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR);
 	} else
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data()
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (7 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-10 11:07   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 09/12] nvmet-configfs: introduce passthru configfs interface Logan Gunthorpe
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

With passthru, the data_len is no longer guaranteed to be set
for all requests. Therefore, we should not check for it to be
non-zero. Instead check if the SGL length is zero and map
when appropriate.

None of the other transports check data_len which is verified
in core code.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index d535080b781f..1e2d92f818ad 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -320,7 +320,7 @@ static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd)
 	struct nvme_sgl_desc *sgl = &cmd->req.cmd->common.dptr.sgl;
 	u32 len = le32_to_cpu(sgl->length);
 
-	if (!cmd->req.data_len)
+	if (!len)
 		return 0;
 
 	if (sgl->type == ((NVME_SGL_FMT_DATA_DESC << 4) |
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 09/12] nvmet-configfs: introduce passthru configfs interface
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (8 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data() Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat() Logan Gunthorpe
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

When CONFIG_NVME_TARGET_PASSTHRU as 'passthru' directory will
be added to each subsystem. The directory is similar to a namespace
and has two attributes: device_path and enable. The user must set the
path to the nvme controller's char device and write '1' to enable the
subsystem to use passthru.

Any given subsystem is prevented from enabling both a regular namespace
and the passthru device. If one is enabled, enabling the other will
produce an error.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/configfs.c | 99 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h    |  1 +
 2 files changed, 100 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..302689081cd7 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -615,6 +615,103 @@ static const struct config_item_type nvmet_namespaces_type = {
 	.ct_owner		= THIS_MODULE,
 };
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+
+static ssize_t nvmet_passthru_device_path_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", subsys->passthru_ctrl_path);
+}
+
+static ssize_t nvmet_passthru_device_path_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	size_t len;
+	int ret;
+
+	mutex_lock(&subsys->lock);
+
+	ret = -EBUSY;
+	if (subsys->passthru_ctrl)
+		goto out_unlock;
+
+	ret = -EINVAL;
+	len = strcspn(page, "\n");
+	if (!len)
+		goto out_unlock;
+
+	kfree(subsys->passthru_ctrl_path);
+	ret = -ENOMEM;
+	subsys->passthru_ctrl_path = kstrndup(page, len, GFP_KERNEL);
+	if (!subsys->passthru_ctrl_path)
+		goto out_unlock;
+
+	mutex_unlock(&subsys->lock);
+
+	return count;
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
+}
+CONFIGFS_ATTR(nvmet_passthru_, device_path);
+
+static ssize_t nvmet_passthru_enable_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return sprintf(page, "%d\n", subsys->passthru_ctrl ? 1 : 0);
+}
+
+static ssize_t nvmet_passthru_enable_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	bool enable;
+	int ret = 0;
+
+	if (strtobool(page, &enable))
+		return -EINVAL;
+
+	if (enable)
+		ret = nvmet_passthru_ctrl_enable(subsys);
+	else
+		nvmet_passthru_ctrl_disable(subsys);
+
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_passthru_, enable);
+
+static struct configfs_attribute *nvmet_passthru_attrs[] = {
+	&nvmet_passthru_attr_device_path,
+	&nvmet_passthru_attr_enable,
+	NULL,
+};
+
+static const struct config_item_type nvmet_passthru_type = {
+	.ct_attrs		= nvmet_passthru_attrs,
+	.ct_owner		= THIS_MODULE,
+};
+
+static void nvmet_add_passthru_group(struct nvmet_subsys *subsys)
+{
+	config_group_init_type_name(&subsys->passthru_group,
+				    "passthru", &nvmet_passthru_type);
+	configfs_add_default_group(&subsys->passthru_group,
+				   &subsys->group);
+}
+
+#else /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static void nvmet_add_passthru_group(struct nvmet_subsys *subsys)
+{
+}
+
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
 static int nvmet_port_subsys_allow_link(struct config_item *parent,
 		struct config_item *target)
 {
@@ -915,6 +1012,8 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 	configfs_add_default_group(&subsys->allowed_hosts_group,
 			&subsys->group);
 
+	nvmet_add_passthru_group(subsys);
+
 	return &subsys->group;
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 2c287d13ed83..ba7690979661 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -231,6 +231,7 @@ struct nvmet_subsys {
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
 	char			*passthru_ctrl_path;
+	struct config_group	passthru_group;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat()
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (9 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 09/12] nvmet-configfs: introduce passthru configfs interface Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-10 10:05   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait() Logan Gunthorpe
  2019-10-09 19:25 ` [PATCH v9 12/12] nvmet-passthru: support block accounting Logan Gunthorpe
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

Instead of checking blk_rq_is_passthruough() for every call to
blk_do_io_stat(), don't set RQF_IO_STAT for passthrough requests.
This should be equivalent, and opens the possibility of passthrough
requests specifically requesting statistics tracking.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-mq.c | 2 +-
 block/blk.h    | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ec791156e9cc..618814fcc8a7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -319,7 +319,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->cmd_flags = op;
 	if (data->flags & BLK_MQ_REQ_PREEMPT)
 		rq->rq_flags |= RQF_PREEMPT;
-	if (blk_queue_io_stat(data->q))
+	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
 		rq->rq_flags |= RQF_IO_STAT;
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_HLIST_NODE(&rq->hash);
diff --git a/block/blk.h b/block/blk.h
index 47fba9362e60..6b7ebc2e61b8 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -243,13 +243,12 @@ int blk_dev_init(void);
  *
  *	a) it's attached to a gendisk, and
  *	b) the queue had IO stats enabled when this request was started, and
- *	c) it's a file system request
+ *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)
  */
 static inline bool blk_do_io_stat(struct request *rq)
 {
 	return rq->rq_disk &&
-	       (rq->rq_flags & RQF_IO_STAT) &&
-		!blk_rq_is_passthrough(rq);
+	       (rq->rq_flags & RQF_IO_STAT);
 }
 
 static inline void req_set_nomerge(struct request_queue *q, struct request *req)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait()
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (10 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat() Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  2019-10-10 10:06   ` Christoph Hellwig
  2019-10-09 19:25 ` [PATCH v9 12/12] nvmet-passthru: support block accounting Logan Gunthorpe
  12 siblings, 1 reply; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

All existing users of blk_execute_rq[_nowait]() are for passthrough
commands and will thus be rejected by blk_do_io_stat().

This allows passthrough requests to opt-in to IO accounting by setting
RQF_IO_STAT.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index 1db44ca0f4a6..e20a852ae432 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -55,6 +55,8 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	rq->rq_disk = bd_disk;
 	rq->end_io = done;
 
+	blk_account_io_start(rq, true);
+
 	/*
 	 * don't check dying flag for MQ because the request won't
 	 * be reused after dying flag is set
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v9 12/12] nvmet-passthru: support block accounting
  2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (11 preceding siblings ...)
  2019-10-09 19:25 ` [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait() Logan Gunthorpe
@ 2019-10-09 19:25 ` Logan Gunthorpe
  12 siblings, 0 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-09 19:25 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, linux-block, linux-fsdevel
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates, Logan Gunthorpe

Support block disk accounting by setting the RQF_IO_STAT flag
and gendisk in the request.

After this change, IO counts will be reflected correctly in
/proc/diskstats for drives being used by passthru.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/io-cmd-passthru.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c
index c482e55f0fb8..37d06ebcbd0f 100644
--- a/drivers/nvme/target/io-cmd-passthru.c
+++ b/drivers/nvme/target/io-cmd-passthru.c
@@ -413,6 +413,9 @@ static struct request *nvmet_passthru_blk_make_request(struct nvmet_req *req,
 	if (unlikely(IS_ERR(rq)))
 		return rq;
 
+	if (blk_queue_io_stat(q))
+		rq->rq_flags |= RQF_IO_STAT;
+
 	if (req->sg_cnt) {
 		ret = nvmet_passthru_map_sg(req, rq);
 		if (unlikely(ret)) {
@@ -477,7 +480,7 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 
 	rq->end_io_data = req;
 	if (req->sq->qid != 0) {
-		blk_execute_rq_nowait(rq->q, NULL, rq, 0,
+		blk_execute_rq_nowait(rq->q, ns->disk, rq, 0,
 				      nvmet_passthru_req_done);
 	} else {
 		req->p.rq = rq;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path()
  2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
@ 2019-10-09 22:13   ` Keith Busch
  2019-10-10 11:37   ` Christoph Hellwig
  1 sibling, 0 replies; 26+ messages in thread
From: Keith Busch @ 2019-10-09 22:13 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:18PM -0600, Logan Gunthorpe wrote:
> nvme_ctrl_get_by_path() is analagous to blkdev_get_by_path() except it
> gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0).
> It makes use of filp_open() to open the file and uses the private
> data to obtain a pointer to the struct nvme_ctrl. If the fops of the
> file do not match, -EINVAL is returned.
> 
> The purpose of this function is to support NVMe-OF target passthru.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Looks fine.

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces
  2019-10-09 19:25 ` [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces Logan Gunthorpe
@ 2019-10-09 22:14   ` Keith Busch
  0 siblings, 0 replies; 26+ messages in thread
From: Keith Busch @ 2019-10-09 22:14 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:19PM -0600, Logan Gunthorpe wrote:
> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 
> 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@wdc.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Looks fine.

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event()
  2019-10-09 19:25 ` [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event() Logan Gunthorpe
@ 2019-10-09 22:17   ` Keith Busch
  0 siblings, 0 replies; 26+ messages in thread
From: Keith Busch @ 2019-10-09 22:17 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:20PM -0600, Logan Gunthorpe wrote:
> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 
> Change the return value for nvmet_add_async_event().
> 
> This change is needed for the target passthru code which will
> submit async events on namespaces changes and can fail the command
> should adding the event fail (on -ENOMEM).
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> [logang@deltatee.com:
>  * fleshed out commit message
>  * change to using int as a return type instead of bool
> ]
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---

Looks fine, but let's remove the version comments out of commit log if
we're applying this one.

Reviewed-by: Keith Busch <kbusch@kernel.org>

>  drivers/nvme/target/core.c  | 6 ++++--
>  drivers/nvme/target/nvmet.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 3a67e244e568..d6dcb86d8be7 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -173,14 +173,14 @@ static void nvmet_async_event_work(struct work_struct *work)
>  	}
>  }
>  
> -void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
> +int 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 -ENOMEM;
>  
>  	aen->event_type = event_type;
>  	aen->event_info = event_info;
> @@ -191,6 +191,8 @@ void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
>  	mutex_unlock(&ctrl->lock);
>  
>  	schedule_work(&ctrl->async_event_work);
> +
> +	return 0;
>  }
>  
>  static void nvmet_add_to_changed_ns_log(struct nvmet_ctrl *ctrl, __le32 nsid)
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index c51f8dd01dc4..3d313a6452cc 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -441,7 +441,7 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
>  		struct nvmet_subsys *subsys);
>  void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
>  		struct nvmet_host *host);
> -void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
> +int nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
>  		u8 event_info, u8 log_page);
>  
>  #define NVMET_QUEUE_SIZE	1024
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static
  2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
@ 2019-10-09 22:18   ` Keith Busch
  2019-10-10 11:50   ` Christoph Hellwig
  1 sibling, 0 replies; 26+ messages in thread
From: Keith Busch @ 2019-10-09 22:18 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:21PM -0600, Logan Gunthorpe wrote:
> This function will be needed by the upcoming passthru code.
> 
> Passthru will need an emulated version of identify_desclist which
> copies the eui64, uuid and nguid from the passed-thru controller into
> the request SGL.
> 
> [chaitanya.kulkarni@wdc.com: this was factored out of a patch
>  originally authored by Chaitanya]
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---

Looks fine

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat()
  2019-10-09 19:25 ` [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat() Logan Gunthorpe
@ 2019-10-10 10:05   ` Christoph Hellwig
  2019-10-10 17:56     ` Logan Gunthorpe
  0 siblings, 1 reply; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 10:05 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

> @@ -319,7 +319,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>  	rq->cmd_flags = op;
>  	if (data->flags & BLK_MQ_REQ_PREEMPT)
>  		rq->rq_flags |= RQF_PREEMPT;
> -	if (blk_queue_io_stat(data->q))
> +	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
>  		rq->rq_flags |= RQF_IO_STAT;

This needs a comment why we don't account passthrough requests by
default.  And I'm really curious about the answer, because I don't
know it myself.

>   *	a) it's attached to a gendisk, and
>   *	b) the queue had IO stats enabled when this request was started, and
> - *	c) it's a file system request
> + *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)

c) should just go away now based on your changes.

>  static inline bool blk_do_io_stat(struct request *rq)
>  {
>  	return rq->rq_disk &&
> -	       (rq->rq_flags & RQF_IO_STAT) &&
> -		!blk_rq_is_passthrough(rq);
> +	       (rq->rq_flags & RQF_IO_STAT);

The check can be collapsed onto a single line now.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait()
  2019-10-09 19:25 ` [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait() Logan Gunthorpe
@ 2019-10-10 10:06   ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 10:06 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:29PM -0600, Logan Gunthorpe wrote:
> All existing users of blk_execute_rq[_nowait]() are for passthrough
> commands and will thus be rejected by blk_do_io_stat().
> 
> This allows passthrough requests to opt-in to IO accounting by setting
> RQF_IO_STAT.

This kinda goes along with the previous patch, so I suggest you
merge them.  I also think you just want to send that merged patch off
directly to Jens ASAP.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl
  2019-10-09 19:25 ` [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl Logan Gunthorpe
@ 2019-10-10 11:04   ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 11:04 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:25PM -0600, Logan Gunthorpe wrote:
> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 
> Right now, data_len is calculated before the transfer len after we
> parse the command, With passthru interface we allow VUCs (Vendor-Unique
> Commands). In order to make the code simple and compact, instead of
> assigning the data len or each VUC in the command parse function
> just use the transfer len as it is. This may result in error if expected
> data_len != transfer_len.

This is a pretty horrible hack.  I think instead we'll need to kill off
the data_len field entirely if we want to go down that route.  Something
like the untested patch below:

---
From a732b65e280823433a2e12c762339fdb121f7729 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 10 Oct 2019 12:19:31 +0200
Subject: nvmet: remove the ->data_len field

Instead check the length inside the commands themselves.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/admin-cmd.c   | 128 +++++++++++++++++-------------
 drivers/nvme/target/core.c        |  12 +--
 drivers/nvme/target/discovery.c   |  63 ++++++++-------
 drivers/nvme/target/fabrics-cmd.c |  15 +++-
 drivers/nvme/target/fc.c          |   4 +-
 drivers/nvme/target/io-cmd-bdev.c |  19 +++--
 drivers/nvme/target/io-cmd-file.c |  21 +++--
 drivers/nvme/target/loop.c        |   2 +-
 drivers/nvme/target/nvmet.h       |  10 ++-
 drivers/nvme/target/rdma.c        |   4 +-
 drivers/nvme/target/tcp.c         |  13 +--
 11 files changed, 169 insertions(+), 122 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 831a062d27cb..05922fa22c66 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -31,7 +31,7 @@ u64 nvmet_get_log_page_offset(struct nvme_command *cmd)
 
 static void nvmet_execute_get_log_page_noop(struct nvmet_req *req)
 {
-	nvmet_req_complete(req, nvmet_zero_sgl(req, 0, req->data_len));
+	nvmet_req_complete(req, nvmet_zero_sgl(req, 0, req->transfer_len));
 }
 
 static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
@@ -134,7 +134,7 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 	u16 status = NVME_SC_INTERNAL;
 	unsigned long flags;
 
-	if (req->data_len != sizeof(*log))
+	if (req->transfer_len != sizeof(*log))
 		goto out;
 
 	log = kzalloc(sizeof(*log), GFP_KERNEL);
@@ -196,7 +196,7 @@ static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 	u16 status = NVME_SC_INTERNAL;
 	size_t len;
 
-	if (req->data_len != NVME_MAX_CHANGED_NAMESPACES * sizeof(__le32))
+	if (req->transfer_len != NVME_MAX_CHANGED_NAMESPACES * sizeof(__le32))
 		goto out;
 
 	mutex_lock(&ctrl->lock);
@@ -206,7 +206,7 @@ static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 		len = ctrl->nr_changed_ns * sizeof(__le32);
 	status = nvmet_copy_to_sgl(req, 0, ctrl->changed_ns_list, len);
 	if (!status)
-		status = nvmet_zero_sgl(req, len, req->data_len - len);
+		status = nvmet_zero_sgl(req, len, req->transfer_len - len);
 	ctrl->nr_changed_ns = 0;
 	nvmet_clear_aen_bit(req, NVME_AEN_BIT_NS_ATTR);
 	mutex_unlock(&ctrl->lock);
@@ -282,6 +282,36 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_get_log_page(struct nvmet_req *req)
+{
+	if (!nvmet_check_data_len(req, nvmet_get_log_page_len(req->cmd)))
+		return;
+
+	switch (req->cmd->get_log_page.lid) {
+	case NVME_LOG_ERROR:
+		return nvmet_execute_get_log_page_error(req);
+	case NVME_LOG_SMART:
+		return nvmet_execute_get_log_page_smart(req);
+	case NVME_LOG_FW_SLOT:
+		/*
+		 * We only support a single firmware slot which always is
+		 * active, so we can zero out the whole firmware slot log and
+		 * still claim to fully implement this mandatory log page.
+		 */
+		return nvmet_execute_get_log_page_noop(req);
+	case NVME_LOG_CHANGED_NS:
+		return nvmet_execute_get_log_changed_ns(req);
+	case NVME_LOG_CMD_EFFECTS:
+		return nvmet_execute_get_log_cmd_effects_ns(req);
+	case NVME_LOG_ANA:
+		return nvmet_execute_get_log_page_ana(req);
+	}
+	pr_err("unhandled lid %d on qid %d\n",
+		req->cmd->get_log_page.lid, req->sq->qid);
+	req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
+	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
+}
+
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -565,6 +595,28 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_identify(struct nvmet_req *req)
+{
+	if (!nvmet_check_data_len(req, NVME_IDENTIFY_DATA_SIZE))
+		return;
+
+	switch (req->cmd->identify.cns) {
+	case NVME_ID_CNS_NS:
+		return nvmet_execute_identify_ns(req);
+	case NVME_ID_CNS_CTRL:
+		return nvmet_execute_identify_ctrl(req);
+	case NVME_ID_CNS_NS_ACTIVE_LIST:
+		return nvmet_execute_identify_nslist(req);
+	case NVME_ID_CNS_NS_DESC_LIST:
+		return nvmet_execute_identify_desclist(req);
+	}
+
+	pr_err("unhandled identify cns %d on qid %d\n",
+		req->cmd->identify.cns, req->sq->qid);
+	req->error_loc = offsetof(struct nvme_identify, cns);
+	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
+}
+
 /*
  * A "minimum viable" abort implementation: the command is mandatory in the
  * spec, but we are not required to do any useful work.  We couldn't really
@@ -574,6 +626,8 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
  */
 static void nvmet_execute_abort(struct nvmet_req *req)
 {
+	if (!nvmet_check_data_len(req, 0))
+		return;
 	nvmet_set_result(req, 1);
 	nvmet_req_complete(req, 0);
 }
@@ -658,6 +712,9 @@ static void nvmet_execute_set_features(struct nvmet_req *req)
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	switch (cdw10 & 0xff) {
 	case NVME_FEAT_NUM_QUEUES:
 		nvmet_set_result(req,
@@ -721,6 +778,9 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	switch (cdw10 & 0xff) {
 	/*
 	 * These features are mandatory in the spec, but we don't
@@ -785,6 +845,9 @@ void nvmet_execute_async_event(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	mutex_lock(&ctrl->lock);
 	if (ctrl->nr_async_event_cmds >= NVMET_ASYNC_EVENTS) {
 		mutex_unlock(&ctrl->lock);
@@ -801,6 +864,9 @@ void nvmet_execute_keep_alive(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	pr_debug("ctrl %d update keep-alive timer for %d secs\n",
 		ctrl->cntlid, ctrl->kato);
 
@@ -819,71 +885,25 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 	switch (cmd->common.opcode) {
 	case nvme_admin_get_log_page:
-		req->data_len = nvmet_get_log_page_len(cmd);
-
-		switch (cmd->get_log_page.lid) {
-		case NVME_LOG_ERROR:
-			req->execute = nvmet_execute_get_log_page_error;
-			return 0;
-		case NVME_LOG_SMART:
-			req->execute = nvmet_execute_get_log_page_smart;
-			return 0;
-		case NVME_LOG_FW_SLOT:
-			/*
-			 * We only support a single firmware slot which always
-			 * is active, so we can zero out the whole firmware slot
-			 * log and still claim to fully implement this mandatory
-			 * log page.
-			 */
-			req->execute = nvmet_execute_get_log_page_noop;
-			return 0;
-		case NVME_LOG_CHANGED_NS:
-			req->execute = nvmet_execute_get_log_changed_ns;
-			return 0;
-		case NVME_LOG_CMD_EFFECTS:
-			req->execute = nvmet_execute_get_log_cmd_effects_ns;
-			return 0;
-		case NVME_LOG_ANA:
-			req->execute = nvmet_execute_get_log_page_ana;
-			return 0;
-		}
-		break;
+		req->execute = nvmet_execute_get_log_page;
+		return 0;;
 	case nvme_admin_identify:
-		req->data_len = NVME_IDENTIFY_DATA_SIZE;
-		switch (cmd->identify.cns) {
-		case NVME_ID_CNS_NS:
-			req->execute = nvmet_execute_identify_ns;
-			return 0;
-		case NVME_ID_CNS_CTRL:
-			req->execute = nvmet_execute_identify_ctrl;
-			return 0;
-		case NVME_ID_CNS_NS_ACTIVE_LIST:
-			req->execute = nvmet_execute_identify_nslist;
-			return 0;
-		case NVME_ID_CNS_NS_DESC_LIST:
-			req->execute = nvmet_execute_identify_desclist;
-			return 0;
-		}
-		break;
+		req->execute = nvmet_execute_identify;
+		return 0;
 	case nvme_admin_abort_cmd:
 		req->execute = nvmet_execute_abort;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_set_features:
 		req->execute = nvmet_execute_set_features;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_get_features:
 		req->execute = nvmet_execute_get_features;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_async_event:
 		req->execute = nvmet_execute_async_event;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_keep_alive:
 		req->execute = nvmet_execute_keep_alive;
-		req->data_len = 0;
 		return 0;
 	}
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3a67e244e568..f7da0e50beeb 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -930,15 +930,17 @@ void nvmet_req_uninit(struct nvmet_req *req)
 }
 EXPORT_SYMBOL_GPL(nvmet_req_uninit);
 
-void nvmet_req_execute(struct nvmet_req *req)
+bool nvmet_check_data_len(struct nvmet_req *req, size_t data_len)
 {
-	if (unlikely(req->data_len != req->transfer_len)) {
+	if (unlikely(data_len != req->transfer_len)) {
 		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		nvmet_req_complete(req, NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR);
-	} else
-		req->execute(req);
+		return false;
+	}
+
+	return true;
 }
-EXPORT_SYMBOL_GPL(nvmet_req_execute);
+EXPORT_SYMBOL_GPL(nvmet_check_data_len);
 
 int nvmet_req_alloc_sgl(struct nvmet_req *req)
 {
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 3764a8900850..a1028f26bff6 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -157,7 +157,7 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	return entries;
 }
 
-static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
+static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 {
 	const int entry_size = sizeof(struct nvmf_disc_rsp_page_entry);
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -171,6 +171,16 @@ static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
 	u16 status = 0;
 	void *buffer;
 
+	if (!nvmet_check_data_len(req, data_len))
+		return;
+
+	if (req->cmd->get_log_page.lid != NVME_LOG_DISC) {
+		req->error_loc =
+			offsetof(struct nvme_get_log_page_command, lid);
+		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		goto out;
+	}
+
 	/* Spec requires dword aligned offsets */
 	if (offset & 0x3) {
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
@@ -227,12 +237,21 @@ static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
+static void nvmet_execute_disc_identify(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, NVME_IDENTIFY_DATA_SIZE))
+		return;
+
+	if (req->cmd->identify.cns != NVME_ID_CNS_CTRL) {
+		req->error_loc = offsetof(struct nvme_identify, cns);
+		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		goto out;
+	}
+
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
 		status = NVME_SC_INTERNAL;
@@ -273,6 +292,9 @@ static void nvmet_execute_disc_set_features(struct nvmet_req *req)
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 stat;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	switch (cdw10 & 0xff) {
 	case NVME_FEAT_KATO:
 		stat = nvmet_set_feat_kato(req);
@@ -296,6 +318,9 @@ static void nvmet_execute_disc_get_features(struct nvmet_req *req)
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 stat = 0;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	switch (cdw10 & 0xff) {
 	case NVME_FEAT_KATO:
 		nvmet_get_feat_kato(req);
@@ -328,53 +353,27 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 	switch (cmd->common.opcode) {
 	case nvme_admin_set_features:
 		req->execute = nvmet_execute_disc_set_features;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_get_features:
 		req->execute = nvmet_execute_disc_get_features;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_async_event:
 		req->execute = nvmet_execute_async_event;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_keep_alive:
 		req->execute = nvmet_execute_keep_alive;
-		req->data_len = 0;
 		return 0;
 	case nvme_admin_get_log_page:
-		req->data_len = nvmet_get_log_page_len(cmd);
-
-		switch (cmd->get_log_page.lid) {
-		case NVME_LOG_DISC:
-			req->execute = nvmet_execute_get_disc_log_page;
-			return 0;
-		default:
-			pr_err("unsupported get_log_page lid %d\n",
-			       cmd->get_log_page.lid);
-			req->error_loc =
-				offsetof(struct nvme_get_log_page_command, lid);
-			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
-		}
+		req->execute = nvmet_execute_disc_get_log_page;
+		return 0;
 	case nvme_admin_identify:
-		req->data_len = NVME_IDENTIFY_DATA_SIZE;
-		switch (cmd->identify.cns) {
-		case NVME_ID_CNS_CTRL:
-			req->execute =
-				nvmet_execute_identify_disc_ctrl;
-			return 0;
-		default:
-			pr_err("unsupported identify cns %d\n",
-			       cmd->identify.cns);
-			req->error_loc = offsetof(struct nvme_identify, cns);
-			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
-		}
+		req->execute = nvmet_execute_disc_identify;
+		return 0;
 	default:
 		pr_err("unhandled cmd %d\n", cmd->common.opcode);
 		req->error_loc = offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
-
 }
 
 int __init nvmet_init_discovery(void)
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index d16b55ffe79f..f7297473d9eb 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -12,6 +12,9 @@ static void nvmet_execute_prop_set(struct nvmet_req *req)
 	u64 val = le64_to_cpu(req->cmd->prop_set.value);
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	if (req->cmd->prop_set.attrib & 1) {
 		req->error_loc =
 			offsetof(struct nvmf_property_set_command, attrib);
@@ -38,6 +41,9 @@ static void nvmet_execute_prop_get(struct nvmet_req *req)
 	u16 status = 0;
 	u64 val = 0;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	if (req->cmd->prop_get.attrib & 1) {
 		switch (le32_to_cpu(req->cmd->prop_get.offset)) {
 		case NVME_REG_CAP:
@@ -82,11 +88,9 @@ u16 nvmet_parse_fabrics_cmd(struct nvmet_req *req)
 
 	switch (cmd->fabrics.fctype) {
 	case nvme_fabrics_type_property_set:
-		req->data_len = 0;
 		req->execute = nvmet_execute_prop_set;
 		break;
 	case nvme_fabrics_type_property_get:
-		req->data_len = 0;
 		req->execute = nvmet_execute_prop_get;
 		break;
 	default:
@@ -147,6 +151,9 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = NULL;
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, sizeof(struct nvmf_connect_data)))
+		return;
+
 	d = kmalloc(sizeof(*d), GFP_KERNEL);
 	if (!d) {
 		status = NVME_SC_INTERNAL;
@@ -211,6 +218,9 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 	u16 qid = le16_to_cpu(c->qid);
 	u16 status = 0;
 
+	if (!nvmet_check_data_len(req, sizeof(struct nvmf_connect_data)))
+		return;
+
 	d = kmalloc(sizeof(*d), GFP_KERNEL);
 	if (!d) {
 		status = NVME_SC_INTERNAL;
@@ -281,7 +291,6 @@ u16 nvmet_parse_connect_cmd(struct nvmet_req *req)
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
-	req->data_len = sizeof(struct nvmf_connect_data);
 	if (cmd->connect.qid == 0)
 		req->execute = nvmet_execute_admin_connect;
 	else
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index ce8d819f86cc..7f9c11138b93 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -2015,7 +2015,7 @@ nvmet_fc_fod_op_done(struct nvmet_fc_fcp_iod *fod)
 		}
 
 		/* data transfer complete, resume with nvmet layer */
-		nvmet_req_execute(&fod->req);
+		fod->req.execute(&fod->req);
 		break;
 
 	case NVMET_FCOP_READDATA:
@@ -2231,7 +2231,7 @@ nvmet_fc_handle_fcp_rqst(struct nvmet_fc_tgtport *tgtport,
 	 * can invoke the nvmet_layer now. If read data, cmd completion will
 	 * push the data
 	 */
-	nvmet_req_execute(&fod->req);
+	fod->req.execute(&fod->req);
 	return;
 
 transport_error:
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 32008d85172b..2d62347573fa 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -150,6 +150,9 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 	sector_t sector;
 	int op, op_flags = 0, i;
 
+	if (!nvmet_check_data_len(req, nvmet_rw_len(req)))
+		return;
+
 	if (!req->sg_cnt) {
 		nvmet_req_complete(req, 0);
 		return;
@@ -170,7 +173,7 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 	sector = le64_to_cpu(req->cmd->rw.slba);
 	sector <<= (req->ns->blksize_shift - 9);
 
-	if (req->data_len <= NVMET_MAX_INLINE_DATA_LEN) {
+	if (req->transfer_len <= NVMET_MAX_INLINE_DATA_LEN) {
 		bio = &req->b.inline_bio;
 		bio_init(bio, req->inline_bvec, ARRAY_SIZE(req->inline_bvec));
 	} else {
@@ -207,6 +210,9 @@ static void nvmet_bdev_execute_flush(struct nvmet_req *req)
 {
 	struct bio *bio = &req->b.inline_bio;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	bio_init(bio, req->inline_bvec, ARRAY_SIZE(req->inline_bvec));
 	bio_set_dev(bio, req->ns->bdev);
 	bio->bi_private = req;
@@ -274,6 +280,9 @@ static void nvmet_bdev_execute_discard(struct nvmet_req *req)
 
 static void nvmet_bdev_execute_dsm(struct nvmet_req *req)
 {
+	if (!nvmet_check_data_len(req, nvmet_dsm_len(req)))
+		return;
+
 	switch (le32_to_cpu(req->cmd->dsm.attributes)) {
 	case NVME_DSMGMT_AD:
 		nvmet_bdev_execute_discard(req);
@@ -295,6 +304,9 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 	sector_t nr_sector;
 	int ret;
 
+	if (!nvmet_check_data_len(req, 0))
+		return;
+
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
@@ -319,20 +331,15 @@ u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req)
 	case nvme_cmd_read:
 	case nvme_cmd_write:
 		req->execute = nvmet_bdev_execute_rw;
-		req->data_len = nvmet_rw_len(req);
 		return 0;
 	case nvme_cmd_flush:
 		req->execute = nvmet_bdev_execute_flush;
-		req->data_len = 0;
 		return 0;
 	case nvme_cmd_dsm:
 		req->execute = nvmet_bdev_execute_dsm;
-		req->data_len = (le32_to_cpu(cmd->dsm.nr) + 1) *
-			sizeof(struct nvme_dsm_range);
 		return 0;
 	case nvme_cmd_write_zeroes:
 		req->execute = nvmet_bdev_execute_write_zeroes;
-		req->data_len = 0;
 		return 0;
 	default:
 		pr_err("unhandled cmd %d on qid %d\n", cmd->common.opcode,
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 05453f5d1448..07fb6cba59b5 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -126,7 +126,7 @@ static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
 			mempool_free(req->f.bvec, req->ns->bvec_pool);
 	}
 
-	if (unlikely(ret != req->data_len))
+	if (unlikely(ret != req->transfer_len))
 		status = errno_to_nvme_status(req, ret);
 	nvmet_req_complete(req, status);
 }
@@ -146,7 +146,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 		is_sync = true;
 
 	pos = le64_to_cpu(req->cmd->rw.slba) << req->ns->blksize_shift;
-	if (unlikely(pos + req->data_len > req->ns->size)) {
+	if (unlikely(pos + req->transfer_len > req->ns->size)) {
 		nvmet_req_complete(req, errno_to_nvme_status(req, -ENOSPC));
 		return true;
 	}
@@ -173,7 +173,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 		nr_bvec--;
 	}
 
-	if (WARN_ON_ONCE(total_len != req->data_len)) {
+	if (WARN_ON_ONCE(total_len != req->transfer_len)) {
 		ret = -EIO;
 		goto complete;
 	}
@@ -232,6 +232,9 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 {
 	ssize_t nr_bvec = req->sg_cnt;
 
+	if (!nvmet_check_data_len(req, nvmet_rw_len(req)))
+		return;
+
 	if (!req->sg_cnt || !nr_bvec) {
 		nvmet_req_complete(req, 0);
 		return;
@@ -273,6 +276,8 @@ static void nvmet_file_flush_work(struct work_struct *w)
 
 static void nvmet_file_execute_flush(struct nvmet_req *req)
 {
+	if (!nvmet_check_data_len(req, 0))
+		return;
 	INIT_WORK(&req->f.work, nvmet_file_flush_work);
 	schedule_work(&req->f.work);
 }
@@ -331,6 +336,9 @@ static void nvmet_file_dsm_work(struct work_struct *w)
 
 static void nvmet_file_execute_dsm(struct nvmet_req *req)
 {
+	if (!nvmet_check_data_len(req, nvmet_dsm_len(req)))
+		return;
+
 	INIT_WORK(&req->f.work, nvmet_file_dsm_work);
 	schedule_work(&req->f.work);
 }
@@ -359,6 +367,8 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 
 static void nvmet_file_execute_write_zeroes(struct nvmet_req *req)
 {
+	if (!nvmet_check_data_len(req, 0))
+		return;
 	INIT_WORK(&req->f.work, nvmet_file_write_zeroes_work);
 	schedule_work(&req->f.work);
 }
@@ -371,20 +381,15 @@ u16 nvmet_file_parse_io_cmd(struct nvmet_req *req)
 	case nvme_cmd_read:
 	case nvme_cmd_write:
 		req->execute = nvmet_file_execute_rw;
-		req->data_len = nvmet_rw_len(req);
 		return 0;
 	case nvme_cmd_flush:
 		req->execute = nvmet_file_execute_flush;
-		req->data_len = 0;
 		return 0;
 	case nvme_cmd_dsm:
 		req->execute = nvmet_file_execute_dsm;
-		req->data_len = (le32_to_cpu(cmd->dsm.nr) + 1) *
-			sizeof(struct nvme_dsm_range);
 		return 0;
 	case nvme_cmd_write_zeroes:
 		req->execute = nvmet_file_execute_write_zeroes;
-		req->data_len = 0;
 		return 0;
 	default:
 		pr_err("unhandled cmd for file ns %d on qid %d\n",
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 748a39fca771..710db727f110 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -126,7 +126,7 @@ static void nvme_loop_execute_work(struct work_struct *work)
 	struct nvme_loop_iod *iod =
 		container_of(work, struct nvme_loop_iod, work);
 
-	nvmet_req_execute(&iod->req);
+	iod->req.execute(&iod->req);
 }
 
 static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd01dc4..46df45e837c9 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -304,8 +304,6 @@ struct nvmet_req {
 		} f;
 	};
 	int			sg_cnt;
-	/* data length as parsed from the command: */
-	size_t			data_len;
 	/* data length as parsed from the SGL descriptor: */
 	size_t			transfer_len;
 
@@ -375,7 +373,7 @@ u16 nvmet_parse_fabrics_cmd(struct nvmet_req *req);
 bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 		struct nvmet_sq *sq, const struct nvmet_fabrics_ops *ops);
 void nvmet_req_uninit(struct nvmet_req *req);
-void nvmet_req_execute(struct nvmet_req *req);
+bool nvmet_check_data_len(struct nvmet_req *req, size_t data_len);
 void nvmet_req_complete(struct nvmet_req *req, u16 status);
 int nvmet_req_alloc_sgl(struct nvmet_req *req);
 void nvmet_req_free_sgl(struct nvmet_req *req);
@@ -495,6 +493,12 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 			req->ns->blksize_shift;
 }
 
+static inline u32 nvmet_dsm_len(struct nvmet_req *req)
+{
+	return (le32_to_cpu(req->cmd->dsm.nr) + 1) *
+		sizeof(struct nvme_dsm_range);
+}
+
 u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
 
 /* Convert a 32-bit number to a 16-bit 0's based number */
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 36d906a7f70d..248e68da2e13 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -603,7 +603,7 @@ static void nvmet_rdma_read_data_done(struct ib_cq *cq, struct ib_wc *wc)
 		return;
 	}
 
-	nvmet_req_execute(&rsp->req);
+	rsp->req.execute(&rsp->req);
 }
 
 static void nvmet_rdma_use_inline_sg(struct nvmet_rdma_rsp *rsp, u32 len,
@@ -746,7 +746,7 @@ static bool nvmet_rdma_execute_command(struct nvmet_rdma_rsp *rsp)
 				queue->cm_id->port_num, &rsp->read_cqe, NULL))
 			nvmet_req_complete(&rsp->req, NVME_SC_DATA_XFER_ERROR);
 	} else {
-		nvmet_req_execute(&rsp->req);
+		rsp->req.execute(&rsp->req);
 	}
 
 	return true;
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index d535080b781f..1c1e743cb628 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -320,7 +320,7 @@ static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd)
 	struct nvme_sgl_desc *sgl = &cmd->req.cmd->common.dptr.sgl;
 	u32 len = le32_to_cpu(sgl->length);
 
-	if (!cmd->req.data_len)
+	if (!len)
 		return 0;
 
 	if (sgl->type == ((NVME_SGL_FMT_DATA_DESC << 4) |
@@ -813,13 +813,14 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
 static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
 		struct nvmet_tcp_cmd *cmd, struct nvmet_req *req)
 {
+	size_t data_len;
 	int ret;
 
 	/* recover the expected data transfer length */
-	req->data_len = le32_to_cpu(req->cmd->common.dptr.sgl.length);
+	data_len = le32_to_cpu(req->cmd->common.dptr.sgl.length);
 
 	if (!nvme_is_write(cmd->req.cmd) ||
-	    req->data_len > cmd->req.port->inline_data_size) {
+	    data_len > cmd->req.port->inline_data_size) {
 		nvmet_prepare_receive_pdu(queue);
 		return;
 	}
@@ -932,7 +933,7 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 		goto out;
 	}
 
-	nvmet_req_execute(&queue->cmd->req);
+	queue->cmd->req.execute(&queue->cmd->req);
 out:
 	nvmet_prepare_receive_pdu(queue);
 	return ret;
@@ -1052,7 +1053,7 @@ static int nvmet_tcp_try_recv_data(struct nvmet_tcp_queue *queue)
 			nvmet_tcp_prep_recv_ddgst(cmd);
 			return 0;
 		}
-		nvmet_req_execute(&cmd->req);
+		cmd->req.execute(&cmd->req);
 	}
 
 	nvmet_prepare_receive_pdu(queue);
@@ -1092,7 +1093,7 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
 
 	if (!(cmd->flags & NVMET_TCP_F_INIT_FAILED) &&
 	    cmd->rbytes_done == cmd->req.transfer_len)
-		nvmet_req_execute(&cmd->req);
+		cmd->req.execute(&cmd->req);
 	ret = 0;
 out:
 	nvmet_prepare_receive_pdu(queue);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data()
  2019-10-09 19:25 ` [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data() Logan Gunthorpe
@ 2019-10-10 11:07   ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 11:07 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:26PM -0600, Logan Gunthorpe wrote:
> With passthru, the data_len is no longer guaranteed to be set
> for all requests. Therefore, we should not check for it to be
> non-zero. Instead check if the SGL length is zero and map
> when appropriate.
> 
> None of the other transports check data_len which is verified
> in core code.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

I think the issue here is deeper.  Yes, this patch is correct, but
nvmet-tcp has another use of req.data_len in
nvmet_tcp_handle_req_failure, which looks completely bogus.  Please
try to audit that as well and send out fixes to the list separately
from this series, as both look like potentially serious bugs.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path()
  2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
  2019-10-09 22:13   ` Keith Busch
@ 2019-10-10 11:37   ` Christoph Hellwig
  1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 11:37 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

> +struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path)
> +{
> +	struct nvme_ctrl *ctrl;
> +	struct file *f;
> +
> +	f = filp_open(path, O_RDWR, 0);
> +	if (IS_ERR(f))
> +		return ERR_CAST(f);
> +
> +	if (f->f_op != &nvme_dev_fops) {
> +		ctrl = ERR_PTR(-EINVAL);
> +		goto out_close;
> +	}
> +
> +	ctrl = f->private_data;
> +	nvme_get_ctrl(ctrl);
> +
> +out_close:
> +	filp_close(f, NULL);
> +
> +	return ctrl;

No need for the empty line here.  Also can you make sure this new
code (and all the new exports) are only enabled if
CONFIG_NVME_TARGET_PASSTHRU is set.  Preferably by having a little
block at the end of this file with this function and the extra
exports with a big fat comment that they are only for nvmet-passthrough.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static
  2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
  2019-10-09 22:18   ` Keith Busch
@ 2019-10-10 11:50   ` Christoph Hellwig
  1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 11:50 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

On Wed, Oct 09, 2019 at 01:25:21PM -0600, Logan Gunthorpe wrote:
> This function will be needed by the upcoming passthru code.
> 
> Passthru will need an emulated version of identify_desclist which
> copies the eui64, uuid and nguid from the passed-thru controller into
> the request SGL.

I don't like the way this is handled.  We should avoid faking up
behavior not supported if this really is a passthrough interface.

For this particular case this means:

 1) report the vs field that the actual controller reports
 2) if that is below 1.2.1 bump it to that, but no further
    (and maybe print a warning)
 3) don't emulate the namespace descriptor CNS ever

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
  2019-10-09 19:25 ` [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com> Logan Gunthorpe
@ 2019-10-10 12:34   ` Christoph Hellwig
  0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2019-10-10 12:34 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Christoph Hellwig, Sagi Grimberg, Keith Busch, Jens Axboe,
	Chaitanya Kulkarni, Max Gurtovoy, Stephen Bates

Just a first round of comments.

> The new file io-cmd-passthru.c handles passthru cmd parsing and execution.
> In the passthru mode, we create a block layer request from the nvmet
> request and map the data on to the block layer request. For handling the
> side effects of the passthru admin commands we add two functions similar
> to the nvme_passthru[start|end]() functions present in the nvme-core.
> Only admin commands on a white list are let through which includes
> vendor unique commands.

I think we need to do work in the core instead and offer a nvme_execute_rq
that handles all that work, and which also is used by the ioctl code.
While we're at it, nvme_submit_io seems to lack the
nvme_passthru_start/end handling, so we'd also fix that while we are at
it.

> @@ -896,6 +896,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_req_passthru_ctrl(req))
> +		status = nvmet_parse_passthru_cmd(req);
>  	else if (likely(req->sq->qid != 0))
>  		status = nvmet_parse_io_cmd(req);
>  	else if (nvme_is_fabrics(req->cmd))

This is turning into a mess (mostly not the fault of this patch, but
that is the final straw).  Moreover there is way to much magic in
nvmet_parse_passthru_cmd that we better share with the existing
code.  See the patch below for what I'd like to see.  This should
probably be split into a prep patch I can submit directly and the
passthrough bits on top of it.

> diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c

This file doesn't contains I/O command handling only, it should
be renamed to passthru.c

> +int nvmet_passthru_init(void)
> +{
> +	passthru_wq = alloc_workqueue("nvmet-passthru-wq", WQ_MEM_RECLAIM, 0);
> +	if (!passthru_wq)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void nvmet_passthru_destroy(void)
> +{
> +	destroy_workqueue(passthru_wq);
> +}

Please keep the init/exit code at the end of the file.

> +
> +static void nvmet_passthru_req_complete(struct nvmet_req *req,
> +		struct request *rq, u16 status)
> +{
> +	nvmet_req_complete(req, status);
> +
> +	if (rq)
> +		blk_put_request(rq);
> +}

No real need for the empty line.  Also I can't see how rq could ever
be NULL here.  In fact I don't really see much of a point in this
helper - just calling the rwo functions directly makes the callers
easier to read for me personally.

> +
> +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;
> +
> +	req->cqe->result.u32 = nvme_req(rq)->result.u32;

This will lose any 64-bit results (not there yet, but we have
pending TPs).  Just assign the actual result union and we are safe
for any future additions.

> +static u16 nvmet_passthru_override_format_nvm(struct nvmet_req *req)
> +{
> +	int lbaf = le32_to_cpu(req->cmd->format.cdw10) & 0x0000000F;
> +	int nsid = le32_to_cpu(req->cmd->format.nsid);
> +	u16 status = NVME_SC_SUCCESS;
> +	struct nvme_id_ns *id;
> +	int ret;
> +
> +	ret = nvme_identify_ns(nvmet_req_passthru_ctrl(req), nsid, &id);
> +	if (ret)
> +		return NVME_SC_INTERNAL;
> +	/*
> +	 * XXX: Please update this code once NVMeOF target starts supporting
> +	 * metadata. We don't support ns lba format with metadata over fabrics
> +	 * right now, so report an 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;

We already filter out all formats with metadata in
nvmet_passthru_override_id_ns, so I see no point in rejecting
formats here that the host can't even have seen.

> +static void nvmet_passthru_set_mdts(struct nvmet_ctrl *ctrl,
> +				    struct nvme_id_ctrl *id)
> +{
> +	struct nvme_ctrl *pctrl = ctrl->subsys->passthru_ctrl;
> +	u32 max_hw_sectors;
> +	int page_shift;
> +
> +	/*
> +	 * The passthru NVMe driver may have a limit on the number
> +	 * of segments which depends on the host's memory fragementation.
> +	 * To solve this, ensure mdts is limitted to the pages equal to
> +	 * the number of segments.
> +	 */
> +
> +	max_hw_sectors = min_not_zero(pctrl->max_segments << (PAGE_SHIFT - 9),
> +				      pctrl->max_hw_sectors);

No need for the blank line, and pleae use up all 80 chars for comments.

> +	nvmet_passthru_set_mdts(ctrl, id);

Any reason that is a separate function and not inlined here?

> +	/* don't support host memory buffer */
> +	id->hmpre = 0;
> +	id->hmmin = 0;

What about CMB/PMR?

> +	/*
> +	 * When passsthru controller is setup using nvme-loop transport it will
> +	 * export the passthru ctrl subsysnqn (PCIe NVMe ctrl) and will fail in
> +	 * the nvme/host/core.c in the nvme_init_subsystem()->nvme_active_ctrl()
> +	 * code path with duplicate ctr subsynqn. In order to prevent that we
> +	 * mask the passthru-ctrl subsysnqn with the target ctrl subsysnqn.
> +	 */
> +	memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));

I don't think this is a good idea.  It will break multipathing when you
export two ports of the original controller.  The whole idea of
overwriting ctrlid and subsysnqn will also lead to huge problems with
persistent reservations.  I think we need to pass through the subnqn
and cntlid unmodified.

> +	/* Support multipath connections with fabrics */
> +	id->cmic |= 1 << 1;

I don't think we can just overwrite this, we need to use the original
controllers values.

> +	if (status)
> +		goto out_free;
> +
> +	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;

This probably wants a similar annotation as all the other metadata
related bits.  Especially as Max is looking into metadata support for
RDMA, and people are working on PCIe frontends for the target code.

> +	/*
> +	 * Admin Commands have side effects and it is better to handle those
> +	 * side effects in the submission thread context than in the request
> +	 * completion path, which is in the interrupt context. Also in this
> +	 * way, we keep the passhru admin command code path consistent with the
> +	 * nvme/host/core.c sync command submission APIs/IOCTLs and use
> +	 * nvme_passthru_start/end() to handle side effects consistently.
> +	 */
> +	blk_execute_rq(req->p.rq->q, NULL, req->p.rq, 0);

Can we please only do the synchronous execution for the cases where
we actually need to override something.

> +static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
> +{
> +	int sg_cnt = req->sg_cnt;
> +	struct scatterlist *sg;
> +	int op = REQ_OP_READ;
> +	int op_flags = 0;
> +	struct bio *bio;
> +	int i, ret;
> +
> +	if (req->cmd->common.opcode == nvme_cmd_flush) {
> +		op_flags = REQ_FUA;
> +	} else if (nvme_is_write(req->cmd)) {
> +		op = REQ_OP_WRITE;
> +		op_flags = REQ_SYNC | REQ_IDLE;
> +	}

This looks weird.  Normally this should be REQ_OP_DRV_IN/REQ_OP_DRV_OUT.

> +	for_each_sg(req->sg, sg, req->sg_cnt, i) {
> +		if (bio_add_page(bio, sg_page(sg), sg->length,
> +				 sg->offset) != sg->length) {
> +			ret = blk_rq_append_bio(rq, &bio);
> +			if (unlikely(ret))
> +				return ret;
> +
> +			bio_set_op_attrs(bio, op, op_flags);

bio_set_op_attrs is deprecated, please just open code it.

> +	/*
> +	 * We don't support fused cmds, also nvme-pci driver uses its own
> +	 * sgl_threshold parameter to decide whether to use SGLs or PRPs hence
> +	 * turn off those bits in the flags.
> +	 */
> +	req->cmd->common.flags &= ~(NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND |
> +			NVME_CMD_SGL_ALL);

I think this belongs into nvme_setup_cmd as is affects all pass
through commands.

> +	rq = nvmet_passthru_blk_make_request(req, ns, GFP_KERNEL);
> +	if (unlikely(IS_ERR(rq))) {

IS_ERR contains an implicitl unlikely.  But I also don't see why
nvmet_passthru_blk_make_request even exists.  Merging it into the
caller would seems more reasonable to me.

> +	if (unlikely(blk_rq_nr_phys_segments(rq) > queue_max_segments(rq->q) ||
> +	    (blk_rq_payload_bytes(rq) >> 9) > queue_max_hw_sectors(rq->q))) {

I' split the two unrelated checks into two if statements for
readability.

> +	case nvme_admin_set_features:
> +		switch (le32_to_cpu(req->cmd->features.fid)) {
> +		case NVME_FEAT_ASYNC_EVENT:
> +		case NVME_FEAT_KATO:
> +		case NVME_FEAT_NUM_QUEUES:
> +			status = nvmet_parse_admin_cmd(req);
> +			break;
> +		default:
> +			req->execute = nvmet_passthru_execute_cmd;
> +		}
> +		break;

We'll need to treat get_features equally.

> +	/* 4. By default, blacklist all admin commands */
> +	default:
> +
> +		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +		req->execute = NULL;
> +		break;
> +	}

That seems odd.  There is plenty of other useful admin commands.

Yes, we need to ignore the PCIe specific ones:

 - Create I/O Completion Queue
 - Create I/O Submission Queue
 - Delete I/O Completion Queue
 - Delete I/O Submission Queue
 - Doorbell Buffer Configuration
 - Virtualization Management

but all others seem perfectly valid to pass through.

>  /* Convert a 32-bit number to a 16-bit 0's based number */
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index f61d6906e59d..94e730b5d0a3 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -816,6 +816,7 @@ enum nvme_admin_opcode {
>  	nvme_admin_security_recv	= 0x82,
>  	nvme_admin_sanitize_nvm		= 0x84,
>  	nvme_admin_get_lba_status	= 0x86,
> +	nvme_admin_vendor_unique_start	= 0xC0,

They are called vendor specific commands.

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 67b6642bb628..de24c140b547 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -652,7 +652,7 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 	return 0;
 }
 
-static void nvmet_execute_set_features(struct nvmet_req *req)
+void nvmet_execute_set_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
@@ -813,10 +813,18 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 	struct nvme_command *cmd = req->cmd;
 	u16 ret;
 
+	if (nvme_is_fabrics(cmd))
+		return nvmet_parse_fabrics_cmd(req);
+	if (req->sq->ctrl->subsys->type == NVME_NQN_DISC)
+		return nvmet_parse_discovery_cmd(req);
+
 	ret = nvmet_check_ctrl_status(req, cmd);
 	if (unlikely(ret))
 		return ret;
 
+	if (nvmet_req_passthru_ctrl(req))
+		return nvmet_parse_passthru_admin_cmd(req);
+
 	switch (cmd->common.opcode) {
 	case nvme_admin_get_log_page:
 		req->data_len = nvmet_get_log_page_len(cmd);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index f9d46354f9ae..3a5b7e42a158 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -839,6 +839,9 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 	if (unlikely(ret))
 		return ret;
 
+	if (nvmet_req_passthru_ctrl(req))
+		return nvmet_setup_passthru_command(req);
+
 	req->ns = nvmet_find_namespace(req->sq->ctrl, cmd->rw.nsid);
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
@@ -900,16 +903,10 @@ 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: */
+		/* will return an error for any non-connect command: */
 		status = nvmet_parse_connect_cmd(req);
-	else if (nvmet_req_passthru_ctrl(req))
-		status = nvmet_parse_passthru_cmd(req);
 	else if (likely(req->sq->qid != 0))
 		status = nvmet_parse_io_cmd(req);
-	else if (nvme_is_fabrics(req->cmd))
-		status = nvmet_parse_fabrics_cmd(req);
-	else if (req->sq->ctrl->subsys->type == NVME_NQN_DISC)
-		status = nvmet_parse_discovery_cmd(req);
 	else
 		status = nvmet_parse_admin_cmd(req);
 
diff --git a/drivers/nvme/target/io-cmd-passthru.c b/drivers/nvme/target/io-cmd-passthru.c
index 37d06ebcbd0f..fbf3508ea8ea 100644
--- a/drivers/nvme/target/io-cmd-passthru.c
+++ b/drivers/nvme/target/io-cmd-passthru.c
@@ -557,6 +557,12 @@ static void nvmet_passthru_emulate_id_desclist(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+u16 nvmet_setup_passthru_command(struct nvmet_req *req)
+{
+	req->execute = nvmet_passthru_execute_cmd;
+	return NVME_SC_SUCCESS;
+}
+
 /*
  * In the passthru mode we support three types for commands:-
  * 1. Commands which are black-listed.
@@ -564,84 +570,55 @@ static void nvmet_passthru_emulate_id_desclist(struct nvmet_req *req)
  * 3. Commands which are emulated in the target code, since we can't rely
  *    on passthru-ctrl and cannot route through the target code.
  */
-static u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
+u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 {
 	struct nvme_command *cmd = req->cmd;
-	u16 status = 0;
 
-	if (cmd->common.opcode >= nvme_admin_vendor_unique_start) {
-		/*
-		 * Passthru all vendor unique commands
-		 */
-		req->execute = nvmet_passthru_execute_cmd;
-		return status;
-	}
+	/*
+	 * Passthru all vendor unique commands
+	 */
+	if (cmd->common.opcode >= nvme_admin_vendor_unique_start)
+		return nvmet_setup_passthru_command(req);
 
 	switch (cmd->common.opcode) {
-	/* 2. commands which are routed through target code */
 	case nvme_admin_async_event:
-	/*
-	 * Right now we don't monitor any events for the passthru controller.
-	 * Instead generate asyn event notice for the ns-mgmt/format/attach
-	 * commands so that host can update it's ns-inventory.
-	 */
-		/* fallthru */
+		req->execute = nvmet_execute_async_event;
+		req->data_len = 0;
+		return 0;
 	case nvme_admin_keep_alive:
-	/*
-	 * 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.
-	 */
-		status = nvmet_parse_admin_cmd(req);
-		break;
+		/*
+		 * 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.
+		 */
+		req->execute = nvmet_execute_keep_alive;
+		req->data_len = 0;
+		return 0;
 	case nvme_admin_set_features:
 		switch (le32_to_cpu(req->cmd->features.fid)) {
 		case NVME_FEAT_ASYNC_EVENT:
 		case NVME_FEAT_KATO:
 		case NVME_FEAT_NUM_QUEUES:
-			status = nvmet_parse_admin_cmd(req);
-			break;
+			/* XXX: we'll need to do the same for get_features! */
+			req->execute = nvmet_execute_set_features;
+			req->data_len = 0;
+			return 0;
 		default:
-			req->execute = nvmet_passthru_execute_cmd;
+			return nvmet_setup_passthru_command(req);
 		}
 		break;
-	/* 3. commands which are emulated in the passthru code */
 	case nvme_admin_identify:
 		switch (req->cmd->identify.cns) {
 		case NVME_ID_CNS_NS_DESC_LIST:
 			req->execute = nvmet_passthru_emulate_id_desclist;
-			break;
+			req->data_len = 0;
+			return 0;
 		default:
-			req->execute = nvmet_passthru_execute_cmd;
+			return nvmet_setup_passthru_command(req);
 		}
 		break;
-	/* 4. By default, blacklist all admin commands */
 	default:
-
-		status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
-		req->execute = NULL;
-		break;
+		/* By default, blacklist all admin commands */
+		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
-
-	return status;
-}
-
-u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
-{
-	int ret;
-
-	if (unlikely(req->cmd->common.opcode == nvme_fabrics_command))
-		return nvmet_parse_fabrics_cmd(req);
-	else if (unlikely(req->sq->ctrl->subsys->type == NVME_NQN_DISC))
-		return nvmet_parse_discovery_cmd(req);
-
-	ret = nvmet_check_ctrl_status(req, req->cmd);
-	if (unlikely(ret))
-		return ret;
-
-	if (unlikely(req->sq->qid == 0))
-		return nvmet_parse_passthru_admin_cmd(req);
-
-	req->execute = nvmet_passthru_execute_cmd;
-	return NVME_SC_SUCCESS;
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index ba7690979661..200ec07507cd 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -390,6 +390,7 @@ void nvmet_req_complete(struct nvmet_req *req, u16 status);
 int nvmet_req_alloc_sgl(struct nvmet_req *req);
 void nvmet_req_free_sgl(struct nvmet_req *req);
 
+void nvmet_execute_set_features(struct nvmet_req *req);
 void nvmet_execute_keep_alive(struct nvmet_req *req);
 
 void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq, u16 qid,
@@ -508,22 +509,19 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 }
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
-
 int nvmet_passthru_init(void);
 void nvmet_passthru_destroy(void);
 void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys);
 int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys);
 void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys);
-u16 nvmet_parse_passthru_cmd(struct nvmet_req *req);
+u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req);
+u16 nvmet_setup_passthru_command(struct nvmet_req *req);
 
-static inline
-struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
 {
 	return subsys->passthru_ctrl;
 }
-
 #else /* CONFIG_NVME_TARGET_PASSTHRU */
-
 static inline int nvmet_passthru_init(void)
 {
 	return 0;
@@ -541,12 +539,10 @@ static inline u16 nvmet_parse_passthru_cmd(struct nvmet_req *req)
 {
 	return 0;
 }
-static inline
-struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
 {
 	return NULL;
 }
-
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 
 static inline struct nvme_ctrl *nvmet_req_passthru_ctrl(struct nvmet_req *req)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat()
  2019-10-10 10:05   ` Christoph Hellwig
@ 2019-10-10 17:56     ` Logan Gunthorpe
  0 siblings, 0 replies; 26+ messages in thread
From: Logan Gunthorpe @ 2019-10-10 17:56 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, linux-nvme, linux-block, linux-fsdevel,
	Sagi Grimberg, Keith Busch, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy, Stephen Bates

Hey,

Thanks for the thorough review, lots here to go through. I'll address it
all as I have time and try to get the prep work done first, as soon as I
can.

On 2019-10-10 4:05 a.m., Christoph Hellwig wrote:
>> @@ -319,7 +319,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>>  	rq->cmd_flags = op;
>>  	if (data->flags & BLK_MQ_REQ_PREEMPT)
>>  		rq->rq_flags |= RQF_PREEMPT;
>> -	if (blk_queue_io_stat(data->q))
>> +	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
>>  		rq->rq_flags |= RQF_IO_STAT;
> 
> This needs a comment why we don't account passthrough requests by
> default.  And I'm really curious about the answer, because I don't
> know it myself.

Yes, sadly, I don't know this answer either but the comment made it
appear that someone did it deliberately. Digging into git blame suggests
that it just evolved that way. The check was originally added in 2005
here with blk_fs_request():

commit d72d904a5367 ("[BLOCK] Update read/write block io statistics at
completion time")

blk_fs_request() evolved to become blk_rq_is_passthrough() but I suspect
no one ever considered whether we want to account the passthru requests.

So, I'll leave this restriction out and see if anyone complains.

Logan

>>   *	a) it's attached to a gendisk, and
>>   *	b) the queue had IO stats enabled when this request was started, and
>> - *	c) it's a file system request
>> + *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)
> 
> c) should just go away now based on your changes.
> 
>>  static inline bool blk_do_io_stat(struct request *rq)
>>  {
>>  	return rq->rq_disk &&
>> -	       (rq->rq_flags & RQF_IO_STAT) &&
>> -		!blk_rq_is_passthrough(rq);
>> +	       (rq->rq_flags & RQF_IO_STAT);
> 
> The check can be collapsed onto a single line now.
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
2019-10-09 22:13   ` Keith Busch
2019-10-10 11:37   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces Logan Gunthorpe
2019-10-09 22:14   ` Keith Busch
2019-10-09 19:25 ` [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event() Logan Gunthorpe
2019-10-09 22:17   ` Keith Busch
2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
2019-10-09 22:18   ` Keith Busch
2019-10-10 11:50   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com> Logan Gunthorpe
2019-10-10 12:34   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 05/12] nvmet-passthru: add passthru code to process commands Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 06/12] nvmet-passthru: add enable/disable helpers Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl Logan Gunthorpe
2019-10-10 11:04   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data() Logan Gunthorpe
2019-10-10 11:07   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 09/12] nvmet-configfs: introduce passthru configfs interface Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat() Logan Gunthorpe
2019-10-10 10:05   ` Christoph Hellwig
2019-10-10 17:56     ` Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait() Logan Gunthorpe
2019-10-10 10:06   ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 12/12] nvmet-passthru: support block accounting Logan Gunthorpe

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox