[PATCHv2 0/5] nvme io_uring_cmd polling enhancements

[PATCHv2 0/5] nvme io_uring_cmd polling enhancements

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Similar idea as the original v1, but even better.

This series originally aimed at improve polling without payloads, and
fix a potential bug when polling multipath. This ended up significantly
simplifying the nvme uring_cmd handling in the process.

In order to ensure we're polling the correct thing, we need the original
request, but we can't depend on the "cookie" since the field has
multi-purpose. But we have a free spot for the request in the driver's
"pdu", so the driver just needs to detangle the "bio" occupying the same
spot.

Keith Busch (5):
  block: add request polling helper
  nvme: simplify passthrough bio cleanup
  nvme: unify nvme request end_io
  nvme: use blk-mq polling for uring commands
  io_uring: remove uring_cmd cookie

 block/blk-mq.c                |  18 +++++
 drivers/nvme/host/ioctl.c     | 137 ++++++----------------------------
 drivers/nvme/host/multipath.c |   2 +-
 drivers/nvme/host/nvme.h      |   2 -
 include/linux/blk-mq.h        |   2 +
 include/linux/io_uring.h      |   8 +-
 io_uring/uring_cmd.c          |   1 -
 7 files changed, 47 insertions(+), 123 deletions(-)

-- 
2.34.1

[PATCHv2 1/5] block: add request polling helper

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This will be used by drivers that allocate polling requests.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-mq.c         | 18 ++++++++++++++++++
 include/linux/blk-mq.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1b304f66f4e8e..67707a488b7e5 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4737,6 +4737,24 @@ int blk_mq_poll(struct request_queue *q, blk_qc_t cookie, struct io_comp_batch *
 	return 0;
 }
 
+int blk_rq_poll(struct request *rq, struct io_comp_batch *iob,
+		unsigned int poll_flags)
+{
+	struct request_queue *q = rq->q;
+	int ret;
+
+	if (!blk_rq_is_poll(rq))
+		return 0;
+	if (!percpu_ref_tryget(&q->q_usage_counter))
+		return 0;
+
+	ret = blk_mq_poll(q, blk_rq_to_qc(rq), iob, poll_flags);
+	blk_queue_exit(q);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(blk_rq_poll);
+
 unsigned int blk_mq_rq_cpu(struct request *rq)
 {
 	return rq->mq_ctx->cpu;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 06caacd77ed66..579818fa1f91d 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -722,6 +722,8 @@ int blk_mq_alloc_sq_tag_set(struct blk_mq_tag_set *set,
 void blk_mq_free_tag_set(struct blk_mq_tag_set *set);
 
 void blk_mq_free_request(struct request *rq);
+int blk_rq_poll(struct request *rq, struct io_comp_batch *iob,
+		unsigned int poll_flags);
 
 bool blk_mq_queue_inflight(struct request_queue *q);
 
-- 
2.34.1

[PATCHv2 2/5] nvme: simplify passthrough bio cleanup

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Set the bio's bi_end_io to handle the cleanup so that uring_cmd doesn't
need this complex pdu->{bio,req} switchero and restore.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index d24ea2e051564..278c57ee0db91 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -159,6 +159,11 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 	return req;
 }
 
+static void nvme_uring_bio_end_io(struct bio *bio)
+{
+	blk_rq_unmap_user(bio);
+}
+
 static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
 		u32 meta_seed, void **metap, struct io_uring_cmd *ioucmd,
@@ -204,6 +209,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		*metap = meta;
 	}
 
+	bio->bi_end_io = nvme_uring_bio_end_io;
 	return ret;
 
 out_unmap:
@@ -249,8 +255,6 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	if (meta)
 		ret = nvme_finish_user_metadata(req, meta_buffer, meta,
 						meta_len, ret);
-	if (bio)
-		blk_rq_unmap_user(bio);
 	blk_mq_free_request(req);
 
 	if (effects)
@@ -443,10 +447,7 @@ struct nvme_uring_data {
  * Expect build errors if this grows larger than that.
  */
 struct nvme_uring_cmd_pdu {
-	union {
-		struct bio *bio;
-		struct request *req;
-	};
+	struct request *req;
 	u32 meta_len;
 	u32 nvme_status;
 	union {
@@ -482,8 +483,6 @@ static void nvme_uring_task_meta_cb(struct io_uring_cmd *ioucmd,
 	if (pdu->meta_len)
 		status = nvme_finish_user_metadata(req, pdu->u.meta_buffer,
 					pdu->u.meta, pdu->meta_len, status);
-	if (req->bio)
-		blk_rq_unmap_user(req->bio);
 	blk_mq_free_request(req);
 
 	io_uring_cmd_done(ioucmd, status, result, issue_flags);
@@ -494,9 +493,6 @@ static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd,
 {
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 
-	if (pdu->bio)
-		blk_rq_unmap_user(pdu->bio);
-
 	io_uring_cmd_done(ioucmd, pdu->nvme_status, pdu->u.result, issue_flags);
 }
 
@@ -507,7 +503,6 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 	void *cookie = READ_ONCE(ioucmd->cookie);
 
-	req->bio = pdu->bio;
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		pdu->nvme_status = -EINTR;
 	else
@@ -533,9 +528,6 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io_meta(struct request *req,
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 	void *cookie = READ_ONCE(ioucmd->cookie);
 
-	req->bio = pdu->bio;
-	pdu->req = req;
-
 	/*
 	 * For iopoll, complete it directly.
 	 * Otherwise, move the completion to task work.
@@ -624,8 +616,8 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			req->bio->bi_opf |= REQ_POLLED;
 		}
 	}
-	/* to free bio on completion, as req->bio will be null at that time */
-	pdu->bio = req->bio;
+
+	pdu->req = req;
 	pdu->meta_len = d.metadata_len;
 	req->end_io_data = ioucmd;
 	if (pdu->meta_len) {
-- 
2.34.1

[PATCHv2 3/5] nvme: unify nvme request end_io

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

We can finish the metadata copy inline with the completion. After that,
there's really nothing else different between the meta and non-meta data
end_io callbacks, so unify them.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 57 +++++++--------------------------------
 1 file changed, 9 insertions(+), 48 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 278c57ee0db91..a1e0a14dadedc 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -465,29 +465,6 @@ static inline struct nvme_uring_cmd_pdu *nvme_uring_cmd_pdu(
 	return (struct nvme_uring_cmd_pdu *)&ioucmd->pdu;
 }
 
-static void nvme_uring_task_meta_cb(struct io_uring_cmd *ioucmd,
-				    unsigned issue_flags)
-{
-	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
-	struct request *req = pdu->req;
-	int status;
-	u64 result;
-
-	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
-		status = -EINTR;
-	else
-		status = nvme_req(req)->status;
-
-	result = le64_to_cpu(nvme_req(req)->result.u64);
-
-	if (pdu->meta_len)
-		status = nvme_finish_user_metadata(req, pdu->u.meta_buffer,
-					pdu->u.meta, pdu->meta_len, status);
-	blk_mq_free_request(req);
-
-	io_uring_cmd_done(ioucmd, status, result, issue_flags);
-}
-
 static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd,
 			       unsigned issue_flags)
 {
@@ -502,11 +479,16 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	struct io_uring_cmd *ioucmd = req->end_io_data;
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 	void *cookie = READ_ONCE(ioucmd->cookie);
+	int status = nvme_req(req)->status;
 
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
-		pdu->nvme_status = -EINTR;
-	else
-		pdu->nvme_status = nvme_req(req)->status;
+		status = -EINTR;
+
+	if (pdu->meta_len)
+		status = nvme_finish_user_metadata(req, pdu->u.meta_buffer,
+					pdu->u.meta, pdu->meta_len, status);
+
+	pdu->nvme_status = status;
 	pdu->u.result = le64_to_cpu(nvme_req(req)->result.u64);
 
 	/*
@@ -521,25 +503,6 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	return RQ_END_IO_FREE;
 }
 
-static enum rq_end_io_ret nvme_uring_cmd_end_io_meta(struct request *req,
-						     blk_status_t err)
-{
-	struct io_uring_cmd *ioucmd = req->end_io_data;
-	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
-	void *cookie = READ_ONCE(ioucmd->cookie);
-
-	/*
-	 * For iopoll, complete it directly.
-	 * Otherwise, move the completion to task work.
-	 */
-	if (cookie != NULL && blk_rq_is_poll(req))
-		nvme_uring_task_meta_cb(ioucmd, IO_URING_F_UNLOCKED);
-	else
-		io_uring_cmd_complete_in_task(ioucmd, nvme_uring_task_meta_cb);
-
-	return RQ_END_IO_NONE;
-}
-
 static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		struct io_uring_cmd *ioucmd, unsigned int issue_flags, bool vec)
 {
@@ -620,12 +583,10 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	pdu->req = req;
 	pdu->meta_len = d.metadata_len;
 	req->end_io_data = ioucmd;
+	req->end_io = nvme_uring_cmd_end_io;
 	if (pdu->meta_len) {
 		pdu->u.meta = meta;
 		pdu->u.meta_buffer = nvme_to_user_ptr(d.metadata);
-		req->end_io = nvme_uring_cmd_end_io_meta;
-	} else {
-		req->end_io = nvme_uring_cmd_end_io;
 	}
 	blk_execute_rq_nowait(req, false);
 	return -EIOCBQUEUED;
-- 
2.34.1

[PATCHv2 4/5] nvme: use blk-mq polling for uring commands

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Drivers can now poll requests directly, and the nvme uring command
unconditionally saves the request now, so use that. We just need to
clear it on completion to prevent any races among multiple threads
polling the same queues during a tagset teardown.

The first advantage is that unshared and multipath namespaces can use
the same polling callback, and multipath is guaranteed to get the same
queue as the command was submitted on. Previously multipath polling might
check a different path and poll the wrong info.

The other advantage is that we don't need a bio payload in order to
poll, allowing commands like 'flush' and 'write zeroes' to be submitted
on the same high priority queue as read and write commands.

This can also allow for a future optimization for the driver since we no
longer need to create special hidden block devices to back nvme-generic
char dev's with unsupported command sets.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c     | 58 +++++------------------------------
 drivers/nvme/host/multipath.c |  2 +-
 drivers/nvme/host/nvme.h      |  2 --
 3 files changed, 9 insertions(+), 53 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index a1e0a14dadedc..3604166752e4b 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -478,9 +478,9 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 {
 	struct io_uring_cmd *ioucmd = req->end_io_data;
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
-	void *cookie = READ_ONCE(ioucmd->cookie);
 	int status = nvme_req(req)->status;
 
+	WRITE_ONCE(pdu->req, NULL);
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		status = -EINTR;
 
@@ -495,7 +495,7 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	 * For iopoll, complete it directly.
 	 * Otherwise, move the completion to task work.
 	 */
-	if (cookie != NULL && blk_rq_is_poll(req))
+	if (blk_rq_is_poll(req))
 		nvme_uring_task_cb(ioucmd, IO_URING_F_UNLOCKED);
 	else
 		io_uring_cmd_complete_in_task(ioucmd, nvme_uring_task_cb);
@@ -554,7 +554,6 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (issue_flags & IO_URING_F_IOPOLL)
 		rq_flags |= REQ_POLLED;
 
-retry:
 	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
@@ -568,19 +567,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			return ret;
 	}
 
-	if (issue_flags & IO_URING_F_IOPOLL && rq_flags & REQ_POLLED) {
-		if (unlikely(!req->bio)) {
-			/* we can't poll this, so alloc regular req instead */
-			blk_mq_free_request(req);
-			rq_flags &= ~REQ_POLLED;
-			goto retry;
-		} else {
-			WRITE_ONCE(ioucmd->cookie, req->bio);
-			req->bio->bi_opf |= REQ_POLLED;
-		}
-	}
-
-	pdu->req = req;
+	WRITE_ONCE(pdu->req, req);
 	pdu->meta_len = d.metadata_len;
 	req->end_io_data = ioucmd;
 	req->end_io = nvme_uring_cmd_end_io;
@@ -735,18 +722,14 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
 				 struct io_comp_batch *iob,
 				 unsigned int poll_flags)
 {
-	struct bio *bio;
+	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
+	struct request *req;
 	int ret = 0;
-	struct nvme_ns *ns;
-	struct request_queue *q;
 
 	rcu_read_lock();
-	bio = READ_ONCE(ioucmd->cookie);
-	ns = container_of(file_inode(ioucmd->file)->i_cdev,
-			struct nvme_ns, cdev);
-	q = ns->queue;
-	if (test_bit(QUEUE_FLAG_POLL, &q->queue_flags) && bio && bio->bi_bdev)
-		ret = bio_poll(bio, iob, poll_flags);
+	req = READ_ONCE(pdu->req);
+	if (req && blk_rq_is_poll(req))
+		ret = blk_rq_poll(req, iob, poll_flags);
 	rcu_read_unlock();
 	return ret;
 }
@@ -838,31 +821,6 @@ int nvme_ns_head_chr_uring_cmd(struct io_uring_cmd *ioucmd,
 	srcu_read_unlock(&head->srcu, srcu_idx);
 	return ret;
 }
-
-int nvme_ns_head_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
-				      struct io_comp_batch *iob,
-				      unsigned int poll_flags)
-{
-	struct cdev *cdev = file_inode(ioucmd->file)->i_cdev;
-	struct nvme_ns_head *head = container_of(cdev, struct nvme_ns_head, cdev);
-	int srcu_idx = srcu_read_lock(&head->srcu);
-	struct nvme_ns *ns = nvme_find_path(head);
-	struct bio *bio;
-	int ret = 0;
-	struct request_queue *q;
-
-	if (ns) {
-		rcu_read_lock();
-		bio = READ_ONCE(ioucmd->cookie);
-		q = ns->queue;
-		if (test_bit(QUEUE_FLAG_POLL, &q->queue_flags) && bio
-				&& bio->bi_bdev)
-			ret = bio_poll(bio, iob, poll_flags);
-		rcu_read_unlock();
-	}
-	srcu_read_unlock(&head->srcu, srcu_idx);
-	return ret;
-}
 #endif /* CONFIG_NVME_MULTIPATH */
 
 int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9171452e2f6d4..f17be1c72f4de 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -470,7 +470,7 @@ static const struct file_operations nvme_ns_head_chr_fops = {
 	.unlocked_ioctl	= nvme_ns_head_chr_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.uring_cmd	= nvme_ns_head_chr_uring_cmd,
-	.uring_cmd_iopoll = nvme_ns_head_chr_uring_cmd_iopoll,
+	.uring_cmd_iopoll = nvme_ns_chr_uring_cmd_iopoll,
 };
 
 static int nvme_add_ns_head_cdev(struct nvme_ns_head *head)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bf46f122e9e1e..ca4ea89333660 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -847,8 +847,6 @@ long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg);
 int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
 		struct io_comp_batch *iob, unsigned int poll_flags);
-int nvme_ns_head_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
-		struct io_comp_batch *iob, unsigned int poll_flags);
 int nvme_ns_chr_uring_cmd(struct io_uring_cmd *ioucmd,
 		unsigned int issue_flags);
 int nvme_ns_head_chr_uring_cmd(struct io_uring_cmd *ioucmd,
-- 
2.34.1

[PATCHv2 5/5] io_uring: remove uring_cmd cookie

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

No users of this field anymore, so remove it.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/io_uring.h | 8 ++------
 io_uring/uring_cmd.c     | 1 -
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
index 35b9328ca3352..235307bf6a072 100644
--- a/include/linux/io_uring.h
+++ b/include/linux/io_uring.h
@@ -25,12 +25,8 @@ enum io_uring_cmd_flags {
 struct io_uring_cmd {
 	struct file	*file;
 	const void	*cmd;
-	union {
-		/* callback to defer completions to task context */
-		void (*task_work_cb)(struct io_uring_cmd *cmd, unsigned);
-		/* used for polled completion */
-		void *cookie;
-	};
+	/* callback to defer completions to task context */
+	void (*task_work_cb)(struct io_uring_cmd *cmd, unsigned);
 	u32		cmd_op;
 	u32		flags;
 	u8		pdu[32]; /* available inline for free use */
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index f7a96bc76ea13..94586f691984c 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -127,7 +127,6 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 			return -EOPNOTSUPP;
 		issue_flags |= IO_URING_F_IOPOLL;
 		req->iopoll_completed = 0;
-		WRITE_ONCE(ioucmd->cookie, NULL);
 	}
 
 	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
-- 
2.34.1

Re: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on next-20230406]
[cannot apply to linus/master v6.3-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/block-add-request-polling-helper/20230408-031926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20230407191636.2631046-3-kbusch%40meta.com
patch subject: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230408/202304080846.C2qDQtUd-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/9a32e7ca02dd8cff559b273fe161b5347b5b5c97
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Keith-Busch/block-add-request-polling-helper/20230408-031926
        git checkout 9a32e7ca02dd8cff559b273fe161b5347b5b5c97
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/nvme/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304080846.C2qDQtUd-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/nvme/host/ioctl.c: In function 'nvme_submit_user_cmd':
>> drivers/nvme/host/ioctl.c:232:21: warning: variable 'bio' set but not used [-Wunused-but-set-variable]
     232 |         struct bio *bio;
         |                     ^~~
   drivers/nvme/host/ioctl.c: In function 'nvme_uring_cmd_end_io_meta':
   drivers/nvme/host/ioctl.c:528:36: warning: unused variable 'pdu' [-Wunused-variable]
     528 |         struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
         |                                    ^~~


vim +/bio +232 drivers/nvme/host/ioctl.c

2405252a680e21 Christoph Hellwig 2021-04-10  222  
bcad2565b5d647 Christoph Hellwig 2022-05-11  223  static int nvme_submit_user_cmd(struct request_queue *q,
7b7fdb8e2dbc15 Christoph Hellwig 2023-01-08  224  		struct nvme_command *cmd, u64 ubuffer, unsigned bufflen,
7b7fdb8e2dbc15 Christoph Hellwig 2023-01-08  225  		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
7b7fdb8e2dbc15 Christoph Hellwig 2023-01-08  226  		u64 *result, unsigned timeout, unsigned int flags)
bcad2565b5d647 Christoph Hellwig 2022-05-11  227  {
62281b9ed671be Christoph Hellwig 2022-12-14  228  	struct nvme_ns *ns = q->queuedata;
bc8fb906b0ff93 Keith Busch       2022-09-19  229  	struct nvme_ctrl *ctrl;
bcad2565b5d647 Christoph Hellwig 2022-05-11  230  	struct request *req;
bcad2565b5d647 Christoph Hellwig 2022-05-11  231  	void *meta = NULL;
bcad2565b5d647 Christoph Hellwig 2022-05-11 @232  	struct bio *bio;
bc8fb906b0ff93 Keith Busch       2022-09-19  233  	u32 effects;
bcad2565b5d647 Christoph Hellwig 2022-05-11  234  	int ret;
bcad2565b5d647 Christoph Hellwig 2022-05-11  235  
470e900c8036ff Kanchan Joshi     2022-09-30  236  	req = nvme_alloc_user_request(q, cmd, 0, 0);
bcad2565b5d647 Christoph Hellwig 2022-05-11  237  	if (IS_ERR(req))
bcad2565b5d647 Christoph Hellwig 2022-05-11  238  		return PTR_ERR(req);
bcad2565b5d647 Christoph Hellwig 2022-05-11  239  
470e900c8036ff Kanchan Joshi     2022-09-30  240  	req->timeout = timeout;
470e900c8036ff Kanchan Joshi     2022-09-30  241  	if (ubuffer && bufflen) {
470e900c8036ff Kanchan Joshi     2022-09-30  242  		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
7b7fdb8e2dbc15 Christoph Hellwig 2023-01-08  243  				meta_len, meta_seed, &meta, NULL, flags);
470e900c8036ff Kanchan Joshi     2022-09-30  244  		if (ret)
470e900c8036ff Kanchan Joshi     2022-09-30  245  			return ret;
470e900c8036ff Kanchan Joshi     2022-09-30  246  	}
470e900c8036ff Kanchan Joshi     2022-09-30  247  
bcad2565b5d647 Christoph Hellwig 2022-05-11  248  	bio = req->bio;
bc8fb906b0ff93 Keith Busch       2022-09-19  249  	ctrl = nvme_req(req)->ctrl;
bcad2565b5d647 Christoph Hellwig 2022-05-11  250  
62281b9ed671be Christoph Hellwig 2022-12-14  251  	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
62281b9ed671be Christoph Hellwig 2022-12-14  252  	ret = nvme_execute_rq(req, false);
2405252a680e21 Christoph Hellwig 2021-04-10  253  	if (result)
2405252a680e21 Christoph Hellwig 2021-04-10  254  		*result = le64_to_cpu(nvme_req(req)->result.u64);
bcad2565b5d647 Christoph Hellwig 2022-05-11  255  	if (meta)
bcad2565b5d647 Christoph Hellwig 2022-05-11  256  		ret = nvme_finish_user_metadata(req, meta_buffer, meta,
bcad2565b5d647 Christoph Hellwig 2022-05-11  257  						meta_len, ret);
2405252a680e21 Christoph Hellwig 2021-04-10  258  	blk_mq_free_request(req);
bc8fb906b0ff93 Keith Busch       2022-09-19  259  
bc8fb906b0ff93 Keith Busch       2022-09-19  260  	if (effects)
bc8fb906b0ff93 Keith Busch       2022-09-19  261  		nvme_passthru_end(ctrl, effects, cmd, ret);
bc8fb906b0ff93 Keith Busch       2022-09-19  262  
2405252a680e21 Christoph Hellwig 2021-04-10  263  	return ret;
2405252a680e21 Christoph Hellwig 2021-04-10  264  }
2405252a680e21 Christoph Hellwig 2021-04-10  265  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

On Fri, Apr 07, 2023 at 12:16:33PM -0700, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Set the bio's bi_end_io to handle the cleanup so that uring_cmd doesn't
>need this complex pdu->{bio,req} switchero and restore.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/ioctl.c | 26 +++++++++-----------------
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>index d24ea2e051564..278c57ee0db91 100644
>--- a/drivers/nvme/host/ioctl.c
>+++ b/drivers/nvme/host/ioctl.c
>@@ -159,6 +159,11 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
> 	return req;
> }
>
>+static void nvme_uring_bio_end_io(struct bio *bio)
>+{
>+	blk_rq_unmap_user(bio);
>+}
>+
> static int nvme_map_user_request(struct request *req, u64 ubuffer,
> 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
> 		u32 meta_seed, void **metap, struct io_uring_cmd *ioucmd,
>@@ -204,6 +209,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
> 		*metap = meta;
> 	}
>
>+	bio->bi_end_io = nvme_uring_bio_end_io;
> 	return ret;
>
> out_unmap:
>@@ -249,8 +255,6 @@ static int nvme_submit_user_cmd(struct request_queue *q,
> 	if (meta)
> 		ret = nvme_finish_user_metadata(req, meta_buffer, meta,
> 						meta_len, ret);
>-	if (bio)
>-		blk_rq_unmap_user(bio);

Is it safe to call blk_rq_unamp_user in irq context?
Agree that current code does some complex stuff, but that's to ensure
what the code-comment [1] says.

Also for polled-io, new-code will hit this warn-on [2] on calling
bio_put_percpu_cache.

[1] 
623  *    A matching blk_rq_unmap_user() must be issued at the end of I/O, while
624  *    still in process context.
625  */
626 int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
627                         struct rq_map_data *map_data,


[2]
773         if ((bio->bi_opf & REQ_POLLED) && !WARN_ON_ONCE(in_interrupt())) {
774                 bio->bi_next = cache->free_list;

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv2 3/5] nvme: unify nvme request end_io

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

On Fri, Apr 07, 2023 at 12:16:34PM -0700, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>We can finish the metadata copy inline with the completion. After that,
>there's really nothing else different between the meta and non-meta data
>end_io callbacks, so unify them.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/ioctl.c | 57 +++++++--------------------------------
> 1 file changed, 9 insertions(+), 48 deletions(-)
>
>diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>index 278c57ee0db91..a1e0a14dadedc 100644
>--- a/drivers/nvme/host/ioctl.c
>+++ b/drivers/nvme/host/ioctl.c
>@@ -465,29 +465,6 @@ static inline struct nvme_uring_cmd_pdu *nvme_uring_cmd_pdu(
> 	return (struct nvme_uring_cmd_pdu *)&ioucmd->pdu;
> }
>
>-static void nvme_uring_task_meta_cb(struct io_uring_cmd *ioucmd,
>-				    unsigned issue_flags)
>-{
>-	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
>-	struct request *req = pdu->req;
>-	int status;
>-	u64 result;
>-
>-	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>-		status = -EINTR;
>-	else
>-		status = nvme_req(req)->status;
>-
>-	result = le64_to_cpu(nvme_req(req)->result.u64);
>-
>-	if (pdu->meta_len)
>-		status = nvme_finish_user_metadata(req, pdu->u.meta_buffer,
>-					pdu->u.meta, pdu->meta_len, status);
>-	blk_mq_free_request(req);
>-
>-	io_uring_cmd_done(ioucmd, status, result, issue_flags);
>-}
>-
> static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd,
> 			       unsigned issue_flags)
> {
>@@ -502,11 +479,16 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
> 	struct io_uring_cmd *ioucmd = req->end_io_data;
> 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
> 	void *cookie = READ_ONCE(ioucmd->cookie);
>+	int status = nvme_req(req)->status;
>
> 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>-		pdu->nvme_status = -EINTR;
>-	else
>-		pdu->nvme_status = nvme_req(req)->status;
>+		status = -EINTR;
>+
>+	if (pdu->meta_len)
>+		status = nvme_finish_user_metadata(req, pdu->u.meta_buffer,
>+					pdu->u.meta, pdu->meta_len, status);

nvme_finish_user_metadata does copy_to_user.
Here also the attempt was not to touch that memory in interrupt context.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on next-20230406]
[cannot apply to linus/master v6.3-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/block-add-request-polling-helper/20230408-031926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20230407191636.2631046-3-kbusch%40meta.com
patch subject: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20230411/202304110443.e026C3nq-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/9a32e7ca02dd8cff559b273fe161b5347b5b5c97
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Keith-Busch/block-add-request-polling-helper/20230408-031926
        git checkout 9a32e7ca02dd8cff559b273fe161b5347b5b5c97
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/nvme/host/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304110443.e026C3nq-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/nvme/host/ioctl.c: In function 'nvme_submit_user_cmd':
   drivers/nvme/host/ioctl.c:232:21: warning: variable 'bio' set but not used [-Wunused-but-set-variable]
     232 |         struct bio *bio;
         |                     ^~~
   drivers/nvme/host/ioctl.c: In function 'nvme_uring_cmd_end_io_meta':
>> drivers/nvme/host/ioctl.c:528:36: warning: unused variable 'pdu' [-Wunused-variable]
     528 |         struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
         |                                    ^~~


vim +/pdu +528 drivers/nvme/host/ioctl.c

c0a7ba77e81b84 Jens Axboe    2022-09-21  523  
c0a7ba77e81b84 Jens Axboe    2022-09-21  524  static enum rq_end_io_ret nvme_uring_cmd_end_io_meta(struct request *req,
c0a7ba77e81b84 Jens Axboe    2022-09-21  525  						     blk_status_t err)
c0a7ba77e81b84 Jens Axboe    2022-09-21  526  {
c0a7ba77e81b84 Jens Axboe    2022-09-21  527  	struct io_uring_cmd *ioucmd = req->end_io_data;
c0a7ba77e81b84 Jens Axboe    2022-09-21 @528  	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
c0a7ba77e81b84 Jens Axboe    2022-09-21  529  	void *cookie = READ_ONCE(ioucmd->cookie);
c0a7ba77e81b84 Jens Axboe    2022-09-21  530  
c0a7ba77e81b84 Jens Axboe    2022-09-21  531  	/*
c0a7ba77e81b84 Jens Axboe    2022-09-21  532  	 * For iopoll, complete it directly.
c0a7ba77e81b84 Jens Axboe    2022-09-21  533  	 * Otherwise, move the completion to task work.
c0a7ba77e81b84 Jens Axboe    2022-09-21  534  	 */
c0a7ba77e81b84 Jens Axboe    2022-09-21  535  	if (cookie != NULL && blk_rq_is_poll(req))
9d2789ac9d60c0 Jens Axboe    2023-03-20  536  		nvme_uring_task_meta_cb(ioucmd, IO_URING_F_UNLOCKED);
c0a7ba77e81b84 Jens Axboe    2022-09-21  537  	else
c0a7ba77e81b84 Jens Axboe    2022-09-21  538  		io_uring_cmd_complete_in_task(ioucmd, nvme_uring_task_meta_cb);
c0a7ba77e81b84 Jens Axboe    2022-09-21  539  
de671d6116b521 Jens Axboe    2022-09-21  540  	return RQ_END_IO_NONE;
456cba386e94f2 Kanchan Joshi 2022-05-11  541  }
456cba386e94f2 Kanchan Joshi 2022-05-11  542  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCHv2 2/5] nvme: simplify passthrough bio cleanup

[Keith Busch]

On Mon, Apr 10, 2023 at 04:55:03PM +0530, Kanchan Joshi wrote:
> On Fri, Apr 07, 2023 at 12:16:33PM -0700, Keith Busch wrote:
> > +static void nvme_uring_bio_end_io(struct bio *bio)
> > +{
> > +	blk_rq_unmap_user(bio);
> > +}
> > +
> > static int nvme_map_user_request(struct request *req, u64 ubuffer,
> > 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
> > 		u32 meta_seed, void **metap, struct io_uring_cmd *ioucmd,
> > @@ -204,6 +209,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
> > 		*metap = meta;
> > 	}
> > 
> > +	bio->bi_end_io = nvme_uring_bio_end_io;
> > 	return ret;
> > 
> > out_unmap:
> > @@ -249,8 +255,6 @@ static int nvme_submit_user_cmd(struct request_queue *q,
> > 	if (meta)
> > 		ret = nvme_finish_user_metadata(req, meta_buffer, meta,
> > 						meta_len, ret);
> > -	if (bio)
> > -		blk_rq_unmap_user(bio);
> 
> Is it safe to call blk_rq_unamp_user in irq context?

Doh! I boxed my thinking into the polling mode that completely neglected the
more common use case. Thanks, now back to the drawing board for me...