[PATCH 0/8] ublk: simplify NEED_GET_DATA handling and request lookup

[PATCH 0/8] ublk: simplify NEED_GET_DATA handling and request lookup

[Caleb Sander Mateos]

Remove accesses to ublk_io's cmd field after the I/O request is posted to the
ublk server. This allows the cmd field to be overlapped with a pointer to the
struct request, avoiding several blk_mq_tag_to_rq() lookups.

Fix a couple of typos noticed along the way.

Caleb Sander Mateos (7):
  ublk: fix "immepdately" typo in comment
  ublk: remove misleading "ubq" in "ubq_complete_io_cmd()"
  ublk: don't log uring_cmd cmd_op in ublk_dispatch_req()
  ublk: factor out ublk_start_io() helper
  ublk: don't call ublk_dispatch_req() for NEED_GET_DATA
  ublk: check UBLK_IO_FLAG_OWNED_BY_SRV in ublk_abort_queue()
  ublk: store request pointer in ublk_io

Uday Shankar (1):
  ublk: factor out ublk_commit_and_fetch

 drivers/block/ublk_drv.c | 234 ++++++++++++++++++++-------------------
 1 file changed, 120 insertions(+), 114 deletions(-)

-- 
2.45.2

[PATCH 1/8] ublk: factor out ublk_commit_and_fetch

[Caleb Sander Mateos]

From: Uday Shankar <ushankar@purestorage.com>

Move the logic for the UBLK_IO_COMMIT_AND_FETCH_REQ opcode into its own
function. This also allows us to mark ublk_queue pointers as const for
that operation, which can help prevent data races since we may allow
concurrent operation on one ublk_queue in the future. Also open code
ublk_commit_completion in ublk_commit_and_fetch to reduce the number of
parameters/avoid a redundant lookup.

Suggested-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Uday Shankar <ushankar@purestorage.com>
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
[Restore __ublk_ch_uring_cmd() req variable used in commit d6aa0c178bf8
("ublk: call ublk_dispatch_req() for handling UBLK_U_IO_NEED_GET_DATA")]
---
 drivers/block/ublk_drv.c | 90 +++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index a183aa7648c3..7809ed585e1c 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1587,34 +1587,10 @@ static int ublk_ch_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	pfn = virt_to_phys(ublk_queue_cmd_buf(ub, q_id)) >> PAGE_SHIFT;
 	return remap_pfn_range(vma, vma->vm_start, pfn, sz, vma->vm_page_prot);
 }
 
-static void ublk_commit_completion(struct ublk_device *ub,
-		const struct ublksrv_io_cmd *ub_cmd)
-{
-	u32 qid = ub_cmd->q_id, tag = ub_cmd->tag;
-	struct ublk_queue *ubq = ublk_get_queue(ub, qid);
-	struct ublk_io *io = &ubq->ios[tag];
-	struct request *req;
-
-	/* now this cmd slot is owned by nbd driver */
-	io->flags &= ~UBLK_IO_FLAG_OWNED_BY_SRV;
-	io->res = ub_cmd->result;
-
-	/* find the io request and complete */
-	req = blk_mq_tag_to_rq(ub->tag_set.tags[qid], tag);
-	if (WARN_ON_ONCE(unlikely(!req)))
-		return;
-
-	if (req_op(req) == REQ_OP_ZONE_APPEND)
-		req->__sector = ub_cmd->zone_append_lba;
-
-	if (likely(!blk_should_fake_timeout(req->q)))
-		ublk_put_req_ref(ubq, req);
-}
-
 static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io,
 		struct request *req)
 {
 	WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE);
 
@@ -2019,10 +1995,51 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
 out:
 	mutex_unlock(&ub->mutex);
 	return ret;
 }
 
+static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
+				 struct ublk_io *io, struct io_uring_cmd *cmd,
+				 const struct ublksrv_io_cmd *ub_cmd)
+{
+	struct blk_mq_tags *tags = ubq->dev->tag_set.tags[ub_cmd->q_id];
+	struct request *req = blk_mq_tag_to_rq(tags, ub_cmd->tag);
+
+	if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
+		return -EINVAL;
+
+	if (ublk_need_map_io(ubq)) {
+		/*
+		 * COMMIT_AND_FETCH_REQ has to provide IO buffer if
+		 * NEED GET DATA is not enabled or it is Read IO.
+		 */
+		if (!ub_cmd->addr && (!ublk_need_get_data(ubq) ||
+					req_op(req) == REQ_OP_READ))
+			return -EINVAL;
+	} else if (req_op(req) != REQ_OP_ZONE_APPEND && ub_cmd->addr) {
+		/*
+		 * User copy requires addr to be unset when command is
+		 * not zone append
+		 */
+		return -EINVAL;
+	}
+
+	ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
+
+	/* now this cmd slot is owned by ublk driver */
+	io->flags &= ~UBLK_IO_FLAG_OWNED_BY_SRV;
+	io->res = ub_cmd->result;
+
+	if (req_op(req) == REQ_OP_ZONE_APPEND)
+		req->__sector = ub_cmd->zone_append_lba;
+
+	if (likely(!blk_should_fake_timeout(req->q)))
+		ublk_put_req_ref(ubq, req);
+
+	return 0;
+}
+
 static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 			       unsigned int issue_flags,
 			       const struct ublksrv_io_cmd *ub_cmd)
 {
 	struct ublk_device *ub = cmd->file->private_data;
@@ -2077,34 +2094,13 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 		ret = ublk_fetch(cmd, ubq, io, ub_cmd->addr);
 		if (ret)
 			goto out;
 		break;
 	case UBLK_IO_COMMIT_AND_FETCH_REQ:
-		req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
-
-		if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
-			goto out;
-
-		if (ublk_need_map_io(ubq)) {
-			/*
-			 * COMMIT_AND_FETCH_REQ has to provide IO buffer if
-			 * NEED GET DATA is not enabled or it is Read IO.
-			 */
-			if (!ub_cmd->addr && (!ublk_need_get_data(ubq) ||
-						req_op(req) == REQ_OP_READ))
-				goto out;
-		} else if (req_op(req) != REQ_OP_ZONE_APPEND && ub_cmd->addr) {
-			/*
-			 * User copy requires addr to be unset when command is
-			 * not zone append
-			 */
-			ret = -EINVAL;
+		ret = ublk_commit_and_fetch(ubq, io, cmd, ub_cmd);
+		if (ret)
 			goto out;
-		}
-
-		ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
-		ublk_commit_completion(ub, ub_cmd);
 		break;
 	case UBLK_IO_NEED_GET_DATA:
 		if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
 			goto out;
 		ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
-- 
2.45.2

[PATCH 2/8] ublk: fix "immepdately" typo in comment

[Caleb Sander Mateos]

Looks like "immediately" was intended.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 7809ed585e1c..44ba6c9d8929 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1180,11 +1180,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 	}
 
 	if (ublk_need_get_data(ubq) && ublk_need_map_req(req)) {
 		/*
 		 * We have not handled UBLK_IO_NEED_GET_DATA command yet,
-		 * so immepdately pass UBLK_IO_RES_NEED_GET_DATA to ublksrv
+		 * so immediately pass UBLK_IO_RES_NEED_GET_DATA to ublksrv
 		 * and notify it.
 		 */
 		if (!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA)) {
 			io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
 			pr_devel("%s: need get data. op %d, qid %d tag %d io_flags %x\n",
-- 
2.45.2

Re: [PATCH 2/8] ublk: fix "immepdately" typo in comment

[Ming Lei]

On Sat, Apr 26, 2025 at 10:57:57PM -0600, Caleb Sander Mateos wrote:
> Looks like "immediately" was intended.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH 3/8] ublk: remove misleading "ubq" in "ubq_complete_io_cmd()"

[Caleb Sander Mateos]

ubq_complete_io_cmd() doesn't interact with a ublk queue, so "ubq" in
the name is confusing. Most likely "ubq" was meant to be "ublk".

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 44ba6c9d8929..4967a5d72029 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1123,12 +1123,12 @@ static void ublk_complete_rq(struct kref *ref)
 	struct request *req = blk_mq_rq_from_pdu(data);
 
 	__ublk_complete_rq(req);
 }
 
-static void ubq_complete_io_cmd(struct ublk_io *io, int res,
-				unsigned issue_flags)
+static void ublk_complete_io_cmd(struct ublk_io *io, int res,
+				 unsigned issue_flags)
 {
 	/* mark this cmd owned by ublksrv */
 	io->flags |= UBLK_IO_FLAG_OWNED_BY_SRV;
 
 	/*
@@ -1188,11 +1188,12 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		if (!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA)) {
 			io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
 			pr_devel("%s: need get data. op %d, qid %d tag %d io_flags %x\n",
 					__func__, io->cmd->cmd_op, ubq->q_id,
 					req->tag, io->flags);
-			ubq_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA, issue_flags);
+			ublk_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA,
+					     issue_flags);
 			return;
 		}
 		/*
 		 * We have handled UBLK_IO_NEED_GET_DATA command,
 		 * so clear UBLK_IO_FLAG_NEED_GET_DATA now and just
@@ -1227,11 +1228,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		ublk_get_iod(ubq, req->tag)->nr_sectors =
 			mapped_bytes >> 9;
 	}
 
 	ublk_init_req_ref(ubq, req);
-	ubq_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
+	ublk_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
 }
 
 static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd,
 			   unsigned int issue_flags)
 {
-- 
2.45.2

Re: [PATCH 3/8] ublk: remove misleading "ubq" in "ubq_complete_io_cmd()"

[Ming Lei]

On Sat, Apr 26, 2025 at 10:57:58PM -0600, Caleb Sander Mateos wrote:
> ubq_complete_io_cmd() doesn't interact with a ublk queue, so "ubq" in
> the name is confusing. Most likely "ubq" was meant to be "ublk".
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH 4/8] ublk: don't log uring_cmd cmd_op in ublk_dispatch_req()

[Caleb Sander Mateos]

cmd_op is either UBLK_U_IO_FETCH_REQ, UBLK_U_IO_COMMIT_AND_FETCH_REQ,
or UBLK_U_IO_NEED_GET_DATA. Which one isn't particularly interesting
is already recorded by the log line in __ublk_ch_uring_cmd().

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 4967a5d72029..01fc92051754 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1159,12 +1159,12 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 {
 	int tag = req->tag;
 	struct ublk_io *io = &ubq->ios[tag];
 	unsigned int mapped_bytes;
 
-	pr_devel("%s: complete: op %d, qid %d tag %d io_flags %x addr %llx\n",
-			__func__, io->cmd->cmd_op, ubq->q_id, req->tag, io->flags,
+	pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
+			__func__, ubq->q_id, req->tag, io->flags,
 			ublk_get_iod(ubq, req->tag)->addr);
 
 	/*
 	 * Task is exiting if either:
 	 *
@@ -1185,13 +1185,12 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		 * so immediately pass UBLK_IO_RES_NEED_GET_DATA to ublksrv
 		 * and notify it.
 		 */
 		if (!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA)) {
 			io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
-			pr_devel("%s: need get data. op %d, qid %d tag %d io_flags %x\n",
-					__func__, io->cmd->cmd_op, ubq->q_id,
-					req->tag, io->flags);
+			pr_devel("%s: need get data. qid %d tag %d io_flags %x\n",
+					__func__, ubq->q_id, req->tag, io->flags);
 			ublk_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA,
 					     issue_flags);
 			return;
 		}
 		/*
@@ -1200,12 +1199,12 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		 * do the copy work.
 		 */
 		io->flags &= ~UBLK_IO_FLAG_NEED_GET_DATA;
 		/* update iod->addr because ublksrv may have passed a new io buffer */
 		ublk_get_iod(ubq, req->tag)->addr = io->addr;
-		pr_devel("%s: update iod->addr: op %d, qid %d tag %d io_flags %x addr %llx\n",
-				__func__, io->cmd->cmd_op, ubq->q_id, req->tag, io->flags,
+		pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
+				__func__, ubq->q_id, req->tag, io->flags,
 				ublk_get_iod(ubq, req->tag)->addr);
 	}
 
 	mapped_bytes = ublk_map_io(ubq, req, io);
 
-- 
2.45.2

Re: [PATCH 4/8] ublk: don't log uring_cmd cmd_op in ublk_dispatch_req()

[Ming Lei]

On Sat, Apr 26, 2025 at 10:57:59PM -0600, Caleb Sander Mateos wrote:
> cmd_op is either UBLK_U_IO_FETCH_REQ, UBLK_U_IO_COMMIT_AND_FETCH_REQ,
> or UBLK_U_IO_NEED_GET_DATA. Which one isn't particularly interesting
> is already recorded by the log line in __ublk_ch_uring_cmd().
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH 5/8] ublk: factor out ublk_start_io() helper

[Caleb Sander Mateos]

In preparation for calling it from outside ublk_dispatch_req(), factor
out the code responsible for setting up an incoming ublk I/O request.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 01fc92051754..90a38a82f8cc 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
 		blk_mq_requeue_request(rq, false);
 	else
 		blk_mq_end_request(rq, BLK_STS_IOERR);
 }
 
+static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
+			  struct ublk_io *io)
+{
+	unsigned mapped_bytes = ublk_map_io(ubq, req, io);
+
+	/* partially mapped, update io descriptor */
+	if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
+		/*
+		 * Nothing mapped, retry until we succeed.
+		 *
+		 * We may never succeed in mapping any bytes here because
+		 * of OOM. TODO: reserve one buffer with single page pinned
+		 * for providing forward progress guarantee.
+		 */
+		if (unlikely(!mapped_bytes)) {
+			blk_mq_requeue_request(req, false);
+			blk_mq_delay_kick_requeue_list(req->q,
+					UBLK_REQUEUE_DELAY_MS);
+			return;
+		}
+
+		ublk_get_iod(ubq, req->tag)->nr_sectors =
+			mapped_bytes >> 9;
+	}
+
+	ublk_init_req_ref(ubq, req);
+}
+
 static void ublk_dispatch_req(struct ublk_queue *ubq,
 			      struct request *req,
 			      unsigned int issue_flags)
 {
 	int tag = req->tag;
 	struct ublk_io *io = &ubq->ios[tag];
-	unsigned int mapped_bytes;
 
 	pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
 			__func__, ubq->q_id, req->tag, io->flags,
 			ublk_get_iod(ubq, req->tag)->addr);
 
@@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
 				__func__, ubq->q_id, req->tag, io->flags,
 				ublk_get_iod(ubq, req->tag)->addr);
 	}
 
-	mapped_bytes = ublk_map_io(ubq, req, io);
-
-	/* partially mapped, update io descriptor */
-	if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
-		/*
-		 * Nothing mapped, retry until we succeed.
-		 *
-		 * We may never succeed in mapping any bytes here because
-		 * of OOM. TODO: reserve one buffer with single page pinned
-		 * for providing forward progress guarantee.
-		 */
-		if (unlikely(!mapped_bytes)) {
-			blk_mq_requeue_request(req, false);
-			blk_mq_delay_kick_requeue_list(req->q,
-					UBLK_REQUEUE_DELAY_MS);
-			return;
-		}
-
-		ublk_get_iod(ubq, req->tag)->nr_sectors =
-			mapped_bytes >> 9;
-	}
-
-	ublk_init_req_ref(ubq, req);
+	ublk_start_io(ubq, req, io);
 	ublk_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
 }
 
 static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd,
 			   unsigned int issue_flags)
-- 
2.45.2

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Ming Lei]

On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> In preparation for calling it from outside ublk_dispatch_req(), factor
> out the code responsible for setting up an incoming ublk I/O request.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
>  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 01fc92051754..90a38a82f8cc 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
>  		blk_mq_requeue_request(rq, false);
>  	else
>  		blk_mq_end_request(rq, BLK_STS_IOERR);
>  }
>  
> +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> +			  struct ublk_io *io)
> +{
> +	unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> +
> +	/* partially mapped, update io descriptor */
> +	if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> +		/*
> +		 * Nothing mapped, retry until we succeed.
> +		 *
> +		 * We may never succeed in mapping any bytes here because
> +		 * of OOM. TODO: reserve one buffer with single page pinned
> +		 * for providing forward progress guarantee.
> +		 */
> +		if (unlikely(!mapped_bytes)) {
> +			blk_mq_requeue_request(req, false);
> +			blk_mq_delay_kick_requeue_list(req->q,
> +					UBLK_REQUEUE_DELAY_MS);
> +			return;
> +		}
> +
> +		ublk_get_iod(ubq, req->tag)->nr_sectors =
> +			mapped_bytes >> 9;
> +	}
> +
> +	ublk_init_req_ref(ubq, req);
> +}
> +
>  static void ublk_dispatch_req(struct ublk_queue *ubq,
>  			      struct request *req,
>  			      unsigned int issue_flags)
>  {
>  	int tag = req->tag;
>  	struct ublk_io *io = &ubq->ios[tag];
> -	unsigned int mapped_bytes;
>  
>  	pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
>  			__func__, ubq->q_id, req->tag, io->flags,
>  			ublk_get_iod(ubq, req->tag)->addr);
>  
> @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
>  		pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
>  				__func__, ubq->q_id, req->tag, io->flags,
>  				ublk_get_iod(ubq, req->tag)->addr);
>  	}
>  
> -	mapped_bytes = ublk_map_io(ubq, req, io);
> -
> -	/* partially mapped, update io descriptor */
> -	if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> -		/*
> -		 * Nothing mapped, retry until we succeed.
> -		 *
> -		 * We may never succeed in mapping any bytes here because
> -		 * of OOM. TODO: reserve one buffer with single page pinned
> -		 * for providing forward progress guarantee.
> -		 */
> -		if (unlikely(!mapped_bytes)) {
> -			blk_mq_requeue_request(req, false);
> -			blk_mq_delay_kick_requeue_list(req->q,
> -					UBLK_REQUEUE_DELAY_MS);
> -			return;
> -		}

Here it needs to break ublk_dispatch_req() for not completing the
uring_cmd, however ublk_start_io() can't support it.


Thanks,
Ming

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Caleb Sander Mateos]

On Sun, Apr 27, 2025 at 6:05 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> > In preparation for calling it from outside ublk_dispatch_req(), factor
> > out the code responsible for setting up an incoming ublk I/O request.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > ---
> >  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 01fc92051754..90a38a82f8cc 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> >               blk_mq_requeue_request(rq, false);
> >       else
> >               blk_mq_end_request(rq, BLK_STS_IOERR);
> >  }
> >
> > +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> > +                       struct ublk_io *io)
> > +{
> > +     unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > +
> > +     /* partially mapped, update io descriptor */
> > +     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > +             /*
> > +              * Nothing mapped, retry until we succeed.
> > +              *
> > +              * We may never succeed in mapping any bytes here because
> > +              * of OOM. TODO: reserve one buffer with single page pinned
> > +              * for providing forward progress guarantee.
> > +              */
> > +             if (unlikely(!mapped_bytes)) {
> > +                     blk_mq_requeue_request(req, false);
> > +                     blk_mq_delay_kick_requeue_list(req->q,
> > +                                     UBLK_REQUEUE_DELAY_MS);
> > +                     return;
> > +             }
> > +
> > +             ublk_get_iod(ubq, req->tag)->nr_sectors =
> > +                     mapped_bytes >> 9;
> > +     }
> > +
> > +     ublk_init_req_ref(ubq, req);
> > +}
> > +
> >  static void ublk_dispatch_req(struct ublk_queue *ubq,
> >                             struct request *req,
> >                             unsigned int issue_flags)
> >  {
> >       int tag = req->tag;
> >       struct ublk_io *io = &ubq->ios[tag];
> > -     unsigned int mapped_bytes;
> >
> >       pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
> >                       __func__, ubq->q_id, req->tag, io->flags,
> >                       ublk_get_iod(ubq, req->tag)->addr);
> >
> > @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> >               pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
> >                               __func__, ubq->q_id, req->tag, io->flags,
> >                               ublk_get_iod(ubq, req->tag)->addr);
> >       }
> >
> > -     mapped_bytes = ublk_map_io(ubq, req, io);
> > -
> > -     /* partially mapped, update io descriptor */
> > -     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > -             /*
> > -              * Nothing mapped, retry until we succeed.
> > -              *
> > -              * We may never succeed in mapping any bytes here because
> > -              * of OOM. TODO: reserve one buffer with single page pinned
> > -              * for providing forward progress guarantee.
> > -              */
> > -             if (unlikely(!mapped_bytes)) {
> > -                     blk_mq_requeue_request(req, false);
> > -                     blk_mq_delay_kick_requeue_list(req->q,
> > -                                     UBLK_REQUEUE_DELAY_MS);
> > -                     return;
> > -             }
>
> Here it needs to break ublk_dispatch_req() for not completing the
> uring_cmd, however ublk_start_io() can't support it.

Good catch. How about I change ublk_start_io() to return a bool
indicating whether the I/O was successfully started?

Thanks,
Caleb

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Caleb Sander Mateos]

On Mon, Apr 28, 2025 at 7:28 AM Caleb Sander Mateos
<csander@purestorage.com> wrote:
>
> On Sun, Apr 27, 2025 at 6:05 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> > > In preparation for calling it from outside ublk_dispatch_req(), factor
> > > out the code responsible for setting up an incoming ublk I/O request.
> > >
> > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
> > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 01fc92051754..90a38a82f8cc 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> > >               blk_mq_requeue_request(rq, false);
> > >       else
> > >               blk_mq_end_request(rq, BLK_STS_IOERR);
> > >  }
> > >
> > > +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> > > +                       struct ublk_io *io)
> > > +{
> > > +     unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > > +
> > > +     /* partially mapped, update io descriptor */
> > > +     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > +             /*
> > > +              * Nothing mapped, retry until we succeed.
> > > +              *
> > > +              * We may never succeed in mapping any bytes here because
> > > +              * of OOM. TODO: reserve one buffer with single page pinned
> > > +              * for providing forward progress guarantee.
> > > +              */
> > > +             if (unlikely(!mapped_bytes)) {
> > > +                     blk_mq_requeue_request(req, false);
> > > +                     blk_mq_delay_kick_requeue_list(req->q,
> > > +                                     UBLK_REQUEUE_DELAY_MS);
> > > +                     return;
> > > +             }
> > > +
> > > +             ublk_get_iod(ubq, req->tag)->nr_sectors =
> > > +                     mapped_bytes >> 9;
> > > +     }
> > > +
> > > +     ublk_init_req_ref(ubq, req);
> > > +}
> > > +
> > >  static void ublk_dispatch_req(struct ublk_queue *ubq,
> > >                             struct request *req,
> > >                             unsigned int issue_flags)
> > >  {
> > >       int tag = req->tag;
> > >       struct ublk_io *io = &ubq->ios[tag];
> > > -     unsigned int mapped_bytes;
> > >
> > >       pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
> > >                       __func__, ubq->q_id, req->tag, io->flags,
> > >                       ublk_get_iod(ubq, req->tag)->addr);
> > >
> > > @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > >               pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
> > >                               __func__, ubq->q_id, req->tag, io->flags,
> > >                               ublk_get_iod(ubq, req->tag)->addr);
> > >       }
> > >
> > > -     mapped_bytes = ublk_map_io(ubq, req, io);
> > > -
> > > -     /* partially mapped, update io descriptor */
> > > -     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > -             /*
> > > -              * Nothing mapped, retry until we succeed.
> > > -              *
> > > -              * We may never succeed in mapping any bytes here because
> > > -              * of OOM. TODO: reserve one buffer with single page pinned
> > > -              * for providing forward progress guarantee.
> > > -              */
> > > -             if (unlikely(!mapped_bytes)) {
> > > -                     blk_mq_requeue_request(req, false);
> > > -                     blk_mq_delay_kick_requeue_list(req->q,
> > > -                                     UBLK_REQUEUE_DELAY_MS);
> > > -                     return;
> > > -             }
> >
> > Here it needs to break ublk_dispatch_req() for not completing the
> > uring_cmd, however ublk_start_io() can't support it.
>
> Good catch. How about I change ublk_start_io() to return a bool
> indicating whether the I/O was successfully started?

Thinking a bit more about this, is the existing behavior of returning
early from ublk_dispatch_req() correct for UBLK_IO_NEED_GET_DATA? It
makes sense for the initial ublk_dispatch_req() because the req will
be requeued without consuming the ublk fetch request, allowing it to
be reused for a subsequent I/O. But for UBLK_IO_NEED_GET_DATA, doesn't
it mean the io_uring_cmd will never complete? I would think it would
be better to return an error code in this case.

Best,
Caleb

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Ming Lei]

On Mon, Apr 28, 2025 at 08:12:52AM -0700, Caleb Sander Mateos wrote:
> On Mon, Apr 28, 2025 at 7:28 AM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > On Sun, Apr 27, 2025 at 6:05 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> > > > In preparation for calling it from outside ublk_dispatch_req(), factor
> > > > out the code responsible for setting up an incoming ublk I/O request.
> > > >
> > > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > > ---
> > > >  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
> > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > index 01fc92051754..90a38a82f8cc 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> > > >               blk_mq_requeue_request(rq, false);
> > > >       else
> > > >               blk_mq_end_request(rq, BLK_STS_IOERR);
> > > >  }
> > > >
> > > > +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> > > > +                       struct ublk_io *io)
> > > > +{
> > > > +     unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > > > +
> > > > +     /* partially mapped, update io descriptor */
> > > > +     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > +             /*
> > > > +              * Nothing mapped, retry until we succeed.
> > > > +              *
> > > > +              * We may never succeed in mapping any bytes here because
> > > > +              * of OOM. TODO: reserve one buffer with single page pinned
> > > > +              * for providing forward progress guarantee.
> > > > +              */
> > > > +             if (unlikely(!mapped_bytes)) {
> > > > +                     blk_mq_requeue_request(req, false);
> > > > +                     blk_mq_delay_kick_requeue_list(req->q,
> > > > +                                     UBLK_REQUEUE_DELAY_MS);
> > > > +                     return;
> > > > +             }
> > > > +
> > > > +             ublk_get_iod(ubq, req->tag)->nr_sectors =
> > > > +                     mapped_bytes >> 9;
> > > > +     }
> > > > +
> > > > +     ublk_init_req_ref(ubq, req);
> > > > +}
> > > > +
> > > >  static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > >                             struct request *req,
> > > >                             unsigned int issue_flags)
> > > >  {
> > > >       int tag = req->tag;
> > > >       struct ublk_io *io = &ubq->ios[tag];
> > > > -     unsigned int mapped_bytes;
> > > >
> > > >       pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
> > > >                       __func__, ubq->q_id, req->tag, io->flags,
> > > >                       ublk_get_iod(ubq, req->tag)->addr);
> > > >
> > > > @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > >               pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
> > > >                               __func__, ubq->q_id, req->tag, io->flags,
> > > >                               ublk_get_iod(ubq, req->tag)->addr);
> > > >       }
> > > >
> > > > -     mapped_bytes = ublk_map_io(ubq, req, io);
> > > > -
> > > > -     /* partially mapped, update io descriptor */
> > > > -     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > -             /*
> > > > -              * Nothing mapped, retry until we succeed.
> > > > -              *
> > > > -              * We may never succeed in mapping any bytes here because
> > > > -              * of OOM. TODO: reserve one buffer with single page pinned
> > > > -              * for providing forward progress guarantee.
> > > > -              */
> > > > -             if (unlikely(!mapped_bytes)) {
> > > > -                     blk_mq_requeue_request(req, false);
> > > > -                     blk_mq_delay_kick_requeue_list(req->q,
> > > > -                                     UBLK_REQUEUE_DELAY_MS);
> > > > -                     return;
> > > > -             }
> > >
> > > Here it needs to break ublk_dispatch_req() for not completing the
> > > uring_cmd, however ublk_start_io() can't support it.
> >
> > Good catch. How about I change ublk_start_io() to return a bool
> > indicating whether the I/O was successfully started?

That is doable.

> 
> Thinking a bit more about this, is the existing behavior of returning
> early from ublk_dispatch_req() correct for UBLK_IO_NEED_GET_DATA? It

The requeue isn't related with UBLK_IO_NEED_GET_DATA actually, when
UBLK_IO_FLAG_NEED_GET_DATA is cleared.

It is usually caused by running out of pages, so we have to requeue until
ublk_map_io() can make progress.

> makes sense for the initial ublk_dispatch_req() because the req will
> be requeued without consuming the ublk fetch request, allowing it to
> be reused for a subsequent I/O. But for UBLK_IO_NEED_GET_DATA, doesn't
> it mean the io_uring_cmd will never complete? I would think it would
> be better to return an error code in this case.

The same request will be requeued and re-dispatched to ublk driver after
a short delay, so the uring_cmd won't be never complete.

Anyway, it isn't another story, which shouldn't be added into this
cleanup patch.

Thanks,
Ming

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Caleb Sander Mateos]

On Mon, Apr 28, 2025 at 9:05 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Mon, Apr 28, 2025 at 08:12:52AM -0700, Caleb Sander Mateos wrote:
> > On Mon, Apr 28, 2025 at 7:28 AM Caleb Sander Mateos
> > <csander@purestorage.com> wrote:
> > >
> > > On Sun, Apr 27, 2025 at 6:05 AM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> > > > > In preparation for calling it from outside ublk_dispatch_req(), factor
> > > > > out the code responsible for setting up an incoming ublk I/O request.
> > > > >
> > > > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > > > ---
> > > > >  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
> > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > > index 01fc92051754..90a38a82f8cc 100644
> > > > > --- a/drivers/block/ublk_drv.c
> > > > > +++ b/drivers/block/ublk_drv.c
> > > > > @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> > > > >               blk_mq_requeue_request(rq, false);
> > > > >       else
> > > > >               blk_mq_end_request(rq, BLK_STS_IOERR);
> > > > >  }
> > > > >
> > > > > +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> > > > > +                       struct ublk_io *io)
> > > > > +{
> > > > > +     unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > > > > +
> > > > > +     /* partially mapped, update io descriptor */
> > > > > +     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > > +             /*
> > > > > +              * Nothing mapped, retry until we succeed.
> > > > > +              *
> > > > > +              * We may never succeed in mapping any bytes here because
> > > > > +              * of OOM. TODO: reserve one buffer with single page pinned
> > > > > +              * for providing forward progress guarantee.
> > > > > +              */
> > > > > +             if (unlikely(!mapped_bytes)) {
> > > > > +                     blk_mq_requeue_request(req, false);
> > > > > +                     blk_mq_delay_kick_requeue_list(req->q,
> > > > > +                                     UBLK_REQUEUE_DELAY_MS);
> > > > > +                     return;
> > > > > +             }
> > > > > +
> > > > > +             ublk_get_iod(ubq, req->tag)->nr_sectors =
> > > > > +                     mapped_bytes >> 9;
> > > > > +     }
> > > > > +
> > > > > +     ublk_init_req_ref(ubq, req);
> > > > > +}
> > > > > +
> > > > >  static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > > >                             struct request *req,
> > > > >                             unsigned int issue_flags)
> > > > >  {
> > > > >       int tag = req->tag;
> > > > >       struct ublk_io *io = &ubq->ios[tag];
> > > > > -     unsigned int mapped_bytes;
> > > > >
> > > > >       pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
> > > > >                       __func__, ubq->q_id, req->tag, io->flags,
> > > > >                       ublk_get_iod(ubq, req->tag)->addr);
> > > > >
> > > > > @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > > >               pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
> > > > >                               __func__, ubq->q_id, req->tag, io->flags,
> > > > >                               ublk_get_iod(ubq, req->tag)->addr);
> > > > >       }
> > > > >
> > > > > -     mapped_bytes = ublk_map_io(ubq, req, io);
> > > > > -
> > > > > -     /* partially mapped, update io descriptor */
> > > > > -     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > > -             /*
> > > > > -              * Nothing mapped, retry until we succeed.
> > > > > -              *
> > > > > -              * We may never succeed in mapping any bytes here because
> > > > > -              * of OOM. TODO: reserve one buffer with single page pinned
> > > > > -              * for providing forward progress guarantee.
> > > > > -              */
> > > > > -             if (unlikely(!mapped_bytes)) {
> > > > > -                     blk_mq_requeue_request(req, false);
> > > > > -                     blk_mq_delay_kick_requeue_list(req->q,
> > > > > -                                     UBLK_REQUEUE_DELAY_MS);
> > > > > -                     return;
> > > > > -             }
> > > >
> > > > Here it needs to break ublk_dispatch_req() for not completing the
> > > > uring_cmd, however ublk_start_io() can't support it.
> > >
> > > Good catch. How about I change ublk_start_io() to return a bool
> > > indicating whether the I/O was successfully started?
>
> That is doable.
>
> >
> > Thinking a bit more about this, is the existing behavior of returning
> > early from ublk_dispatch_req() correct for UBLK_IO_NEED_GET_DATA? It
>
> The requeue isn't related with UBLK_IO_NEED_GET_DATA actually, when
> UBLK_IO_FLAG_NEED_GET_DATA is cleared.
>
> It is usually caused by running out of pages, so we have to requeue until
> ublk_map_io() can make progress.
>
> > makes sense for the initial ublk_dispatch_req() because the req will
> > be requeued without consuming the ublk fetch request, allowing it to
> > be reused for a subsequent I/O. But for UBLK_IO_NEED_GET_DATA, doesn't
> > it mean the io_uring_cmd will never complete? I would think it would
> > be better to return an error code in this case.
>
> The same request will be requeued and re-dispatched to ublk driver after
> a short delay, so the uring_cmd won't be never complete.

I am referring to the UBLK_IO_NEED_GET_DATA uring_cmd, not the FETCH
one. Doesn't the early return in ublk_dispatch_req() mean
ublk_complete_io_cmd() won't be called? How else can the
UBLK_IO_NEED_GET_DATA complete?

>
> Anyway, it isn't another story, which shouldn't be added into this
> cleanup patch.

I agree it belongs in a separate patch.

Best,
Caleb

Re: [PATCH 5/8] ublk: factor out ublk_start_io() helper

[Caleb Sander Mateos]

On Tue, Apr 29, 2025 at 7:55 AM Caleb Sander Mateos
<csander@purestorage.com> wrote:
>
> On Mon, Apr 28, 2025 at 9:05 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Mon, Apr 28, 2025 at 08:12:52AM -0700, Caleb Sander Mateos wrote:
> > > On Mon, Apr 28, 2025 at 7:28 AM Caleb Sander Mateos
> > > <csander@purestorage.com> wrote:
> > > >
> > > > On Sun, Apr 27, 2025 at 6:05 AM Ming Lei <ming.lei@redhat.com> wrote:
> > > > >
> > > > > On Sat, Apr 26, 2025 at 10:58:00PM -0600, Caleb Sander Mateos wrote:
> > > > > > In preparation for calling it from outside ublk_dispatch_req(), factor
> > > > > > out the code responsible for setting up an incoming ublk I/O request.
> > > > > >
> > > > > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > > > > ---
> > > > > >  drivers/block/ublk_drv.c | 53 ++++++++++++++++++++++------------------
> > > > > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > > > index 01fc92051754..90a38a82f8cc 100644
> > > > > > --- a/drivers/block/ublk_drv.c
> > > > > > +++ b/drivers/block/ublk_drv.c
> > > > > > @@ -1151,17 +1151,44 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> > > > > >               blk_mq_requeue_request(rq, false);
> > > > > >       else
> > > > > >               blk_mq_end_request(rq, BLK_STS_IOERR);
> > > > > >  }
> > > > > >
> > > > > > +static void ublk_start_io(struct ublk_queue *ubq, struct request *req,
> > > > > > +                       struct ublk_io *io)
> > > > > > +{
> > > > > > +     unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > > > > > +
> > > > > > +     /* partially mapped, update io descriptor */
> > > > > > +     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > > > +             /*
> > > > > > +              * Nothing mapped, retry until we succeed.
> > > > > > +              *
> > > > > > +              * We may never succeed in mapping any bytes here because
> > > > > > +              * of OOM. TODO: reserve one buffer with single page pinned
> > > > > > +              * for providing forward progress guarantee.
> > > > > > +              */
> > > > > > +             if (unlikely(!mapped_bytes)) {
> > > > > > +                     blk_mq_requeue_request(req, false);
> > > > > > +                     blk_mq_delay_kick_requeue_list(req->q,
> > > > > > +                                     UBLK_REQUEUE_DELAY_MS);
> > > > > > +                     return;
> > > > > > +             }
> > > > > > +
> > > > > > +             ublk_get_iod(ubq, req->tag)->nr_sectors =
> > > > > > +                     mapped_bytes >> 9;
> > > > > > +     }
> > > > > > +
> > > > > > +     ublk_init_req_ref(ubq, req);
> > > > > > +}
> > > > > > +
> > > > > >  static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > > > >                             struct request *req,
> > > > > >                             unsigned int issue_flags)
> > > > > >  {
> > > > > >       int tag = req->tag;
> > > > > >       struct ublk_io *io = &ubq->ios[tag];
> > > > > > -     unsigned int mapped_bytes;
> > > > > >
> > > > > >       pr_devel("%s: complete: qid %d tag %d io_flags %x addr %llx\n",
> > > > > >                       __func__, ubq->q_id, req->tag, io->flags,
> > > > > >                       ublk_get_iod(ubq, req->tag)->addr);
> > > > > >
> > > > > > @@ -1204,33 +1231,11 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > > > >               pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
> > > > > >                               __func__, ubq->q_id, req->tag, io->flags,
> > > > > >                               ublk_get_iod(ubq, req->tag)->addr);
> > > > > >       }
> > > > > >
> > > > > > -     mapped_bytes = ublk_map_io(ubq, req, io);
> > > > > > -
> > > > > > -     /* partially mapped, update io descriptor */
> > > > > > -     if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > > > > -             /*
> > > > > > -              * Nothing mapped, retry until we succeed.
> > > > > > -              *
> > > > > > -              * We may never succeed in mapping any bytes here because
> > > > > > -              * of OOM. TODO: reserve one buffer with single page pinned
> > > > > > -              * for providing forward progress guarantee.
> > > > > > -              */
> > > > > > -             if (unlikely(!mapped_bytes)) {
> > > > > > -                     blk_mq_requeue_request(req, false);
> > > > > > -                     blk_mq_delay_kick_requeue_list(req->q,
> > > > > > -                                     UBLK_REQUEUE_DELAY_MS);
> > > > > > -                     return;
> > > > > > -             }
> > > > >
> > > > > Here it needs to break ublk_dispatch_req() for not completing the
> > > > > uring_cmd, however ublk_start_io() can't support it.
> > > >
> > > > Good catch. How about I change ublk_start_io() to return a bool
> > > > indicating whether the I/O was successfully started?
> >
> > That is doable.
> >
> > >
> > > Thinking a bit more about this, is the existing behavior of returning
> > > early from ublk_dispatch_req() correct for UBLK_IO_NEED_GET_DATA? It
> >
> > The requeue isn't related with UBLK_IO_NEED_GET_DATA actually, when
> > UBLK_IO_FLAG_NEED_GET_DATA is cleared.
> >
> > It is usually caused by running out of pages, so we have to requeue until
> > ublk_map_io() can make progress.
> >
> > > makes sense for the initial ublk_dispatch_req() because the req will
> > > be requeued without consuming the ublk fetch request, allowing it to
> > > be reused for a subsequent I/O. But for UBLK_IO_NEED_GET_DATA, doesn't
> > > it mean the io_uring_cmd will never complete? I would think it would
> > > be better to return an error code in this case.
> >
> > The same request will be requeued and re-dispatched to ublk driver after
> > a short delay, so the uring_cmd won't be never complete.
>
> I am referring to the UBLK_IO_NEED_GET_DATA uring_cmd, not the FETCH
> one. Doesn't the early return in ublk_dispatch_req() mean
> ublk_complete_io_cmd() won't be called? How else can the
> UBLK_IO_NEED_GET_DATA complete?
>
> >
> > Anyway, it isn't another story, which shouldn't be added into this
> > cleanup patch.
>
> I agree it belongs in a separate patch.

I am not going to fix it in this patch set. I am not sure what the
result value for UBLK_IO_NEED_GET_DATA should be if
ublk_copy_user_pages() returns 0. I also noticed that the return value
of import_ubuf() is ignored, which means that a bad userspace address
will result in an uninitialized struct iov_iter. I think ublk_map_io()
and ublk_unmap_io() may need to have more failure modes.

Best,
Caleb

[PATCH 6/8] ublk: don't call ublk_dispatch_req() for NEED_GET_DATA

[Caleb Sander Mateos]

ublk_dispatch_req() currently handles 3 different cases: incoming ublk
requests that don't need to wait for a data buffer, incoming requests
that do need to wait for a buffer, and resuming those requests once the
buffer is provided. But the call site that provides a data buffer
(UBLK_IO_NEED_GET_DATA) is separate from those for incoming requests.

So simplify the function by splitting the UBLK_IO_NEED_GET_DATA case
into its own function ublk_get_data(). This avoids several redundant
checks in the UBLK_IO_NEED_GET_DATA case, and streamlines the incoming
request cases.

Don't call ublk_fill_io_cmd() for UBLK_IO_NEED_GET_DATA, as it's no
longer necessary to set io->cmd or the UBLK_IO_FLAG_ACTIVE flag for
ublk_dispatch_req().

Since UBLK_IO_NEED_GET_DATA no longer relies on ublk_dispatch_req()
calling io_uring_cmd_done(), return the UBLK_IO_RES_OK status directly
from the ->uring_cmd() handler.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 49 ++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 90a38a82f8cc..ea63cee1786e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1210,29 +1210,16 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		/*
 		 * We have not handled UBLK_IO_NEED_GET_DATA command yet,
 		 * so immediately pass UBLK_IO_RES_NEED_GET_DATA to ublksrv
 		 * and notify it.
 		 */
-		if (!(io->flags & UBLK_IO_FLAG_NEED_GET_DATA)) {
-			io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
-			pr_devel("%s: need get data. qid %d tag %d io_flags %x\n",
-					__func__, ubq->q_id, req->tag, io->flags);
-			ublk_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA,
-					     issue_flags);
-			return;
-		}
-		/*
-		 * We have handled UBLK_IO_NEED_GET_DATA command,
-		 * so clear UBLK_IO_FLAG_NEED_GET_DATA now and just
-		 * do the copy work.
-		 */
-		io->flags &= ~UBLK_IO_FLAG_NEED_GET_DATA;
-		/* update iod->addr because ublksrv may have passed a new io buffer */
-		ublk_get_iod(ubq, req->tag)->addr = io->addr;
-		pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
-				__func__, ubq->q_id, req->tag, io->flags,
-				ublk_get_iod(ubq, req->tag)->addr);
+		io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
+		pr_devel("%s: need get data. qid %d tag %d io_flags %x\n",
+				__func__, ubq->q_id, req->tag, io->flags);
+		ublk_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA,
+				     issue_flags);
+		return;
 	}
 
 	ublk_start_io(ubq, req, io);
 	ublk_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
 }
@@ -2041,10 +2028,28 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
 		ublk_put_req_ref(ubq, req);
 
 	return 0;
 }
 
+static void ublk_get_data(struct ublk_queue *ubq, struct ublk_io *io,
+			  struct request *req)
+{
+	/*
+	 * We have handled UBLK_IO_NEED_GET_DATA command,
+	 * so clear UBLK_IO_FLAG_NEED_GET_DATA now and just
+	 * do the copy work.
+	 */
+	io->flags &= ~UBLK_IO_FLAG_NEED_GET_DATA;
+	/* update iod->addr because ublksrv may have passed a new io buffer */
+	ublk_get_iod(ubq, req->tag)->addr = io->addr;
+	pr_devel("%s: update iod->addr: qid %d tag %d io_flags %x addr %llx\n",
+			__func__, ubq->q_id, req->tag, io->flags,
+			ublk_get_iod(ubq, req->tag)->addr);
+
+	ublk_start_io(ubq, req, io);
+}
+
 static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 			       unsigned int issue_flags,
 			       const struct ublksrv_io_cmd *ub_cmd)
 {
 	struct ublk_device *ub = cmd->file->private_data;
@@ -2106,14 +2111,14 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 			goto out;
 		break;
 	case UBLK_IO_NEED_GET_DATA:
 		if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
 			goto out;
-		ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
+		io->addr = ub_cmd->addr;
 		req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
-		ublk_dispatch_req(ubq, req, issue_flags);
-		return -EIOCBQUEUED;
+		ublk_get_data(ubq, io, req);
+		return UBLK_IO_RES_OK;
 	default:
 		goto out;
 	}
 	ublk_prep_cancel(cmd, issue_flags, ubq, tag);
 	return -EIOCBQUEUED;
-- 
2.45.2

Re: [PATCH 6/8] ublk: don't call ublk_dispatch_req() for NEED_GET_DATA

[Ming Lei]

On Sat, Apr 26, 2025 at 10:58:01PM -0600, Caleb Sander Mateos wrote:
> ublk_dispatch_req() currently handles 3 different cases: incoming ublk
> requests that don't need to wait for a data buffer, incoming requests
> that do need to wait for a buffer, and resuming those requests once the
> buffer is provided. But the call site that provides a data buffer
> (UBLK_IO_NEED_GET_DATA) is separate from those for incoming requests.
> 
> So simplify the function by splitting the UBLK_IO_NEED_GET_DATA case
> into its own function ublk_get_data(). This avoids several redundant
> checks in the UBLK_IO_NEED_GET_DATA case, and streamlines the incoming
> request cases.
> 
> Don't call ublk_fill_io_cmd() for UBLK_IO_NEED_GET_DATA, as it's no
> longer necessary to set io->cmd or the UBLK_IO_FLAG_ACTIVE flag for
> ublk_dispatch_req().
> 
> Since UBLK_IO_NEED_GET_DATA no longer relies on ublk_dispatch_req()
> calling io_uring_cmd_done(), return the UBLK_IO_RES_OK status directly
> from the ->uring_cmd() handler.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

thanks,
Ming

[PATCH 7/8] ublk: check UBLK_IO_FLAG_OWNED_BY_SRV in ublk_abort_queue()

[Caleb Sander Mateos]

ublk_abort_queue() currently checks whether the UBLK_IO_FLAG_ACTIVE flag
is cleared to tell whether to abort each ublk_io in the queue. But it's
possible for a ublk_io to not be ACTIVE but also not have a request in
flight, such as when no fetch request has yet been submitted for a tag
or when a fetch request is cancelled. So ublk_abort_queue() must
additionally check for an inflight request.

Simplify this code by checking for UBLK_IO_FLAG_OWNED_BY_SRV instead,
which indicates precisely whether a request is currently inflight.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index ea63cee1786e..2296fa677d0d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1607,20 +1607,15 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
 	int i;
 
 	for (i = 0; i < ubq->q_depth; i++) {
 		struct ublk_io *io = &ubq->ios[i];
 
-		if (!(io->flags & UBLK_IO_FLAG_ACTIVE)) {
+		if (io->flags & UBLK_IO_FLAG_OWNED_BY_SRV) {
 			struct request *rq;
 
-			/*
-			 * Either we fail the request or ublk_rq_task_work_cb
-			 * will do it
-			 */
 			rq = blk_mq_tag_to_rq(ub->tag_set.tags[ubq->q_id], i);
-			if (rq && blk_mq_request_started(rq))
-				__ublk_fail_req(ubq, io, rq);
+			__ublk_fail_req(ubq, io, rq);
 		}
 	}
 }
 
 /* Must be called when queue is frozen */
-- 
2.45.2

Re: [PATCH 7/8] ublk: check UBLK_IO_FLAG_OWNED_BY_SRV in ublk_abort_queue()

[Ming Lei]

On Sat, Apr 26, 2025 at 10:58:02PM -0600, Caleb Sander Mateos wrote:
> ublk_abort_queue() currently checks whether the UBLK_IO_FLAG_ACTIVE flag
> is cleared to tell whether to abort each ublk_io in the queue. But it's
> possible for a ublk_io to not be ACTIVE but also not have a request in
> flight, such as when no fetch request has yet been submitted for a tag
> or when a fetch request is cancelled. So ublk_abort_queue() must
> additionally check for an inflight request.
> 
> Simplify this code by checking for UBLK_IO_FLAG_OWNED_BY_SRV instead,
> which indicates precisely whether a request is currently inflight.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

[PATCH 8/8] ublk: store request pointer in ublk_io

[Caleb Sander Mateos]

A ublk_io is converted to a request in several places in the I/O path by
looking up the (qid, tag) on the ublk device's tagset. This involves a
bunch of pointer dereferences and a bounds check of the tag.

To make this conversion cheaper, store the request pointer in ublk_io.
Overlap this storage with the io_uring_cmd pointer. This is safe because
the io_uring_cmd pointer is only valid if UBLK_IO_FLAG_ACTIVE is set on
the ublk_io, the request pointer is valid if UBLK_IO_FLAG_OWNED_BY_SRV,
and these flags are mutually exclusive.

Suggested-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 43 ++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2296fa677d0d..c69c234adb4e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -140,11 +140,16 @@ struct ublk_io {
 	/* userspace buffer address from io cmd */
 	__u64	addr;
 	unsigned int flags;
 	int res;
 
-	struct io_uring_cmd *cmd;
+	union {
+		/* valid if UBLK_IO_FLAG_ACTIVE is set */
+		struct io_uring_cmd *cmd;
+		/* valid if UBLK_IO_FLAG_OWNED_BY_SRV is set */
+		struct request *req;
+	};
 };
 
 struct ublk_queue {
 	int q_id;
 	int q_depth;
@@ -1123,24 +1128,29 @@ static void ublk_complete_rq(struct kref *ref)
 	struct request *req = blk_mq_rq_from_pdu(data);
 
 	__ublk_complete_rq(req);
 }
 
-static void ublk_complete_io_cmd(struct ublk_io *io, int res,
-				 unsigned issue_flags)
+static void ublk_complete_io_cmd(struct ublk_io *io, struct request *req,
+				 int res, unsigned issue_flags)
 {
+	/* read cmd first because req will overwrite it */
+	struct io_uring_cmd *cmd = io->cmd;
+
 	/* mark this cmd owned by ublksrv */
 	io->flags |= UBLK_IO_FLAG_OWNED_BY_SRV;
 
 	/*
 	 * clear ACTIVE since we are done with this sqe/cmd slot
 	 * We can only accept io cmd in case of being not active.
 	 */
 	io->flags &= ~UBLK_IO_FLAG_ACTIVE;
 
+	io->req = req;
+
 	/* tell ublksrv one io request is coming */
-	io_uring_cmd_done(io->cmd, res, 0, issue_flags);
+	io_uring_cmd_done(cmd, res, 0, issue_flags);
 }
 
 #define UBLK_REQUEUE_DELAY_MS	3
 
 static inline void __ublk_abort_rq(struct ublk_queue *ubq,
@@ -1213,17 +1223,17 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
 		 * and notify it.
 		 */
 		io->flags |= UBLK_IO_FLAG_NEED_GET_DATA;
 		pr_devel("%s: need get data. qid %d tag %d io_flags %x\n",
 				__func__, ubq->q_id, req->tag, io->flags);
-		ublk_complete_io_cmd(io, UBLK_IO_RES_NEED_GET_DATA,
+		ublk_complete_io_cmd(io, req, UBLK_IO_RES_NEED_GET_DATA,
 				     issue_flags);
 		return;
 	}
 
 	ublk_start_io(ubq, req, io);
-	ublk_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
+	ublk_complete_io_cmd(io, req, UBLK_IO_RES_OK, issue_flags);
 }
 
 static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd,
 			   unsigned int issue_flags)
 {
@@ -1607,16 +1617,12 @@ static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq)
 	int i;
 
 	for (i = 0; i < ubq->q_depth; i++) {
 		struct ublk_io *io = &ubq->ios[i];
 
-		if (io->flags & UBLK_IO_FLAG_OWNED_BY_SRV) {
-			struct request *rq;
-
-			rq = blk_mq_tag_to_rq(ub->tag_set.tags[ubq->q_id], i);
-			__ublk_fail_req(ubq, io, rq);
-		}
+		if (io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)
+			__ublk_fail_req(ubq, io, io->req);
 	}
 }
 
 /* Must be called when queue is frozen */
 static void ublk_mark_queue_canceling(struct ublk_queue *ubq)
@@ -1986,16 +1992,16 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
 
 static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
 				 struct ublk_io *io, struct io_uring_cmd *cmd,
 				 const struct ublksrv_io_cmd *ub_cmd)
 {
-	struct blk_mq_tags *tags = ubq->dev->tag_set.tags[ub_cmd->q_id];
-	struct request *req = blk_mq_tag_to_rq(tags, ub_cmd->tag);
+	struct request *req;
 
 	if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
 		return -EINVAL;
 
+	req = io->req;
 	if (ublk_need_map_io(ubq)) {
 		/*
 		 * COMMIT_AND_FETCH_REQ has to provide IO buffer if
 		 * NEED GET DATA is not enabled or it is Read IO.
 		 */
@@ -2023,13 +2029,14 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
 		ublk_put_req_ref(ubq, req);
 
 	return 0;
 }
 
-static void ublk_get_data(struct ublk_queue *ubq, struct ublk_io *io,
-			  struct request *req)
+static void ublk_get_data(struct ublk_queue *ubq, struct ublk_io *io)
 {
+	struct request *req = io->req;
+
 	/*
 	 * We have handled UBLK_IO_NEED_GET_DATA command,
 	 * so clear UBLK_IO_FLAG_NEED_GET_DATA now and just
 	 * do the copy work.
 	 */
@@ -2051,11 +2058,10 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 	struct ublk_queue *ubq;
 	struct ublk_io *io;
 	u32 cmd_op = cmd->cmd_op;
 	unsigned tag = ub_cmd->tag;
 	int ret = -EINVAL;
-	struct request *req;
 
 	pr_devel("%s: received: cmd op %d queue %d tag %d result %d\n",
 			__func__, cmd->cmd_op, ub_cmd->q_id, tag,
 			ub_cmd->result);
 
@@ -2107,12 +2113,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 		break;
 	case UBLK_IO_NEED_GET_DATA:
 		if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
 			goto out;
 		io->addr = ub_cmd->addr;
-		req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
-		ublk_get_data(ubq, io, req);
+		ublk_get_data(ubq, io);
 		return UBLK_IO_RES_OK;
 	default:
 		goto out;
 	}
 	ublk_prep_cancel(cmd, issue_flags, ubq, tag);
-- 
2.45.2

Re: [PATCH 8/8] ublk: store request pointer in ublk_io

[Ming Lei]

On Sat, Apr 26, 2025 at 10:58:03PM -0600, Caleb Sander Mateos wrote:
> A ublk_io is converted to a request in several places in the I/O path by
> looking up the (qid, tag) on the ublk device's tagset. This involves a
> bunch of pointer dereferences and a bounds check of the tag.
> 
> To make this conversion cheaper, store the request pointer in ublk_io.
> Overlap this storage with the io_uring_cmd pointer. This is safe because
> the io_uring_cmd pointer is only valid if UBLK_IO_FLAG_ACTIVE is set on
> the ublk_io, the request pointer is valid if UBLK_IO_FLAG_OWNED_BY_SRV,
> and these flags are mutually exclusive.

Yeah, it becomes exclusive after UBLK_IO_NEED_GET_DATA is cleaned as one
sync command.

> 
> Suggested-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

thanks,
Ming