Linux block layer
 help / color / mirror / Atom feed
* [PATCH v2 0/2] ublk: honor rq_affinity on request completion
@ 2026-10-06 19:03 Yoav Cohen
  2026-10-06 19:03 ` [PATCH v2 1/2] " Yoav Cohen
  2026-10-06 19:03 ` [PATCH v2 2/2] selftests: ublk: add rq_affinity test Yoav Cohen
  0 siblings, 2 replies; 7+ messages in thread
From: Yoav Cohen @ 2026-10-06 19:03 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-block, csander; +Cc: jholzman, omril, Yoav Cohen

ublk ends successfully completed requests inline on the server thread,
so rq_affinity never redirects completion work back to the submitting
CPU the way it does for NVMe, SCSI, virtio-blk, loop, nbd and rnbd.
Patch 1 fixes that and adds a feature flag so servers can detect
support. Patch 2 adds a selftest for it.

v2, addressing Ming Lei's review of v1:
- v1 only redirected the classic-copy-mode successful-read completion;
  every write, flush/discard, error, and zero-copy/user-copy/
  auto-buf-reg/shmem-zc I/O still completed inline as before, so
  rq_affinity had no effect for most traffic. ublk_end_rq() now
  re-derives which completion path __ublk_complete_rq() took from
  @req/@io state, so every outcome goes through
  blk_mq_complete_request_remote().
- v1 ran the new completion path unconditionally on any patched
  kernel. v2 adds UBLK_F_SUPPORT_RQ_AFFINITY, which a server must
  request at ADD_DEV time; without it, behavior is unchanged.

With an SPDK-style polling user-space block server at queue depth 96,
rq_affinity=2 gives 7-8% higher IOPS than today's always-inline
completion by offloading a saturated server CPU; rq_affinity=0 keeps
the old behavior for anyone who wants to opt out.

Yoav Cohen (2):
  ublk: honor rq_affinity on request completion
  selftests: ublk: add rq_affinity test

 drivers/block/ublk_drv.c                      | 98 ++++++++++++++-----
 include/uapi/linux/ublk_cmd.h                 |  8 ++
 tools/testing/selftests/ublk/Makefile         |  1 +
 tools/testing/selftests/ublk/kublk.c          |  6 +-
 .../testing/selftests/ublk/test_generic_18.sh | 42 ++++++++
 5 files changed, 132 insertions(+), 23 deletions(-)
 create mode 100755 tools/testing/selftests/ublk/test_generic_18.sh

-- 
2.50.1 (Apple Git-155)


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

* [PATCH v2 1/2] ublk: honor rq_affinity on request completion
  2026-10-06 19:03 [PATCH v2 0/2] ublk: honor rq_affinity on request completion Yoav Cohen
@ 2026-10-06 19:03 ` Yoav Cohen
  2026-10-06 21:53   ` Caleb Sander Mateos
  2026-10-09 14:04   ` Ming Lei
  2026-10-06 19:03 ` [PATCH v2 2/2] selftests: ublk: add rq_affinity test Yoav Cohen
  1 sibling, 2 replies; 7+ messages in thread
From: Yoav Cohen @ 2026-10-06 19:03 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-block, csander; +Cc: jholzman, omril, Yoav Cohen

ublk ends successfully completed requests inline on the server thread
that issued UBLK_IO_COMMIT_AND_FETCH_REQ, so rq_affinity never
redirects completion back to the submitting CPU, unlike NVMe, SCSI,
virtio-blk, loop, nbd and rnbd.

Route completion through blk_mq_complete_request_remote(), with a new
->complete() callback, ublk_end_rq(). This covers every completion
outcome -- reads, writes, flush/discard, errors, and the zero-copy/
user-copy/auto-buf-reg/shmem-zc paths that skip the copy-back step --
not just the classic-copy-mode successful-read case, since
ublk_end_rq() re-derives which of those __ublk_complete_rq() took from
@req and @io state rather than assuming one.

Gate it behind a new UBLK_F_SUPPORT_RQ_AFFINITY flag that a server
must request at ADD_DEV time; servers that don't ask for it keep
today's always-local completion unchanged. Add ublk_support_rq_affinity()
alongside the existing ublk_support_*() helpers, and check it before
calling blk_mq_complete_request_remote() at both call sites.

Signed-off-by: Yoav Cohen <yoav@nvidia.com>
---
 drivers/block/ublk_drv.c      | 98 +++++++++++++++++++++++++++--------
 include/uapi/linux/ublk_cmd.h |  8 +++
 2 files changed, 84 insertions(+), 22 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 66eb55e7162e..b94293296b73 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -90,7 +90,8 @@
 		| UBLK_F_BATCH_IO \
 		| UBLK_F_NO_AUTO_PART_SCAN \
 		| UBLK_F_SHMEM_ZC \
-		| UBLK_F_IO_DESC_SIZE)
+		| UBLK_F_IO_DESC_SIZE \
+		| UBLK_F_SUPPORT_RQ_AFFINITY)
 
 #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
 		| UBLK_F_USER_RECOVERY_REISSUE \
@@ -452,6 +453,11 @@ static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
 	return ubq->flags & UBLK_F_USER_COPY;
 }
 
+static inline bool ublk_support_rq_affinity(const struct ublk_queue *ubq)
+{
+	return ubq->flags & UBLK_F_SUPPORT_RQ_AFFINITY;
+}
+
 static inline bool ublk_dev_support_user_copy(const struct ublk_device *ub)
 {
 	return ub->dev_info.flags & UBLK_F_USER_COPY;
@@ -1468,6 +1474,14 @@ static inline bool ublk_need_unmap_req(const struct request *req)
 	       (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
 }
 
+/* Whether __ublk_complete_rq() takes the copy-back/partial-completion path */
+static inline bool ublk_rq_need_unmap(const struct ublk_queue *ubq,
+				      struct request *req)
+{
+	return ublk_need_map_io(ubq) && ublk_need_unmap_req(req) &&
+	       !ublk_iod_is_shmem_zc(ubq, req->tag);
+}
+
 static unsigned int ublk_map_io(const struct request *req,
 				const struct ublk_io *io)
 {
@@ -1549,13 +1563,62 @@ static void ublk_end_request(struct request *req, blk_status_t error)
 	local_bh_enable();
 }
 
+/*
+ * Update @req with its result and requeue it if it was only partially
+ * completed. Returns true if @req was requeued, in which case the caller
+ * must not touch it any further.
+ *
+ * Run bio->bi_end_io() with softirqs disabled. If the final fput happens
+ * off this path, then that will prevent ublk's blkdev_release() from
+ * being called on current's task work, see fput() implementation.
+ *
+ * This matters for the caller completing @req locally on the ublk
+ * server's own thread: it may already be holding disk->open_mutex, e.g.
+ * reading the partition table from bdev_open(), and an fput() running
+ * inline there could deadlock on it. Preferably we would not be doing
+ * IO with a mutex held that is also used for release, but this
+ * work-around will suffice for now. A caller reached instead via
+ * blk_mq_complete_request_remote()'s softirq/IPI redirect never runs on
+ * that thread, so it isn't exposed to this hazard, but disabling
+ * softirqs here is harmless for it too.
+ */
+static inline bool ublk_update_and_requeue(struct request *req,
+					   struct ublk_io *io)
+{
+	bool requeue;
+
+	local_bh_disable();
+	requeue = blk_update_request(req, BLK_STS_OK, io->res);
+	local_bh_enable();
+	if (requeue)
+		blk_mq_requeue_request(req, true);
+	return requeue;
+}
+
+static void ublk_end_rq(struct request *req)
+{
+	struct ublk_queue *ubq = req->mq_hctx->driver_data;
+	struct ublk_io *io = &ubq->ios[req->tag];
+
+	/* matches the two outcomes __ublk_complete_rq() may have redirected */
+	if (io->res >= 0 && ublk_rq_need_unmap(ubq, req)) {
+		if (!ublk_update_and_requeue(req, io) &&
+		    likely(!blk_should_fake_timeout(req->q)))
+			__blk_mq_end_request(req, BLK_STS_OK);
+		return;
+	}
+
+	ublk_end_request(req, io->res < 0 ? errno_to_blk_status(io->res) :
+					    BLK_STS_OK);
+}
+
 /* todo: handle partial completion */
 static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
 				      bool need_map, struct io_comp_batch *iob)
 {
+	struct ublk_queue *ubq = req->mq_hctx->driver_data;
 	unsigned int unmapped_bytes;
 	blk_status_t res = BLK_STS_OK;
-	bool requeue;
 
 	/* failed read IO if nothing is read */
 	if (!io->res && req_op(req) == REQ_OP_READ)
@@ -1568,7 +1631,7 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
 
 	/* shmem zero copy: no data to unmap, pages already shared */
 	if (!need_map || !ublk_need_unmap_req(req) ||
-	    ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))
+	    ublk_iod_is_shmem_zc(ubq, req->tag))
 		goto exit;
 
 	/* for READ request, writing data in iod->addr to rq buffers */
@@ -1582,31 +1645,18 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
 	if (unlikely(unmapped_bytes < io->res)) {
 		if (unlikely(!unmapped_bytes)) {
 			res = BLK_STS_IOERR;
+			io->res = -EIO;
 			goto exit;
 		}
 
 		io->res = unmapped_bytes;
 	}
 
-	/*
-	 * Run bio->bi_end_io() with softirqs disabled. If the final fput
-	 * happens off this path, then that will prevent ublk's blkdev_release()
-	 * from being called on current's task work, see fput() implementation.
-	 *
-	 * Otherwise, ublk server may not provide forward progress in case of
-	 * reading the partition table from bdev_open() with disk->open_mutex
-	 * held, and causes dead lock as we could already be holding
-	 * disk->open_mutex here.
-	 *
-	 * Preferably we would not be doing IO with a mutex held that is also
-	 * used for release, but this work-around will suffice for now.
-	 */
-	local_bh_disable();
-	requeue = blk_update_request(req, BLK_STS_OK, io->res);
-	local_bh_enable();
-	if (requeue)
-		blk_mq_requeue_request(req, true);
-	else if (likely(!blk_should_fake_timeout(req->q))) {
+	if (ublk_support_rq_affinity(ubq) && blk_mq_complete_request_remote(req))
+		return;
+
+	if (!ublk_update_and_requeue(req, io) &&
+	    likely(!blk_should_fake_timeout(req->q))) {
 		if (blk_mq_add_to_batch(req, iob, false, blk_mq_end_request_batch))
 			return;
 		__blk_mq_end_request(req, BLK_STS_OK);
@@ -1614,6 +1664,8 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
 
 	return;
 exit:
+	if (ublk_support_rq_affinity(ubq) && blk_mq_complete_request_remote(req))
+		return;
 	ublk_end_request(req, res);
 }
 
@@ -2347,6 +2399,7 @@ static const struct blk_mq_ops ublk_mq_ops = {
 	.queue_rqs      = ublk_queue_rqs,
 	.init_hctx	= ublk_init_hctx,
 	.timeout	= ublk_timeout,
+	.complete	= ublk_end_rq,
 };
 
 static const struct blk_mq_ops ublk_batch_mq_ops = {
@@ -2355,6 +2408,7 @@ static const struct blk_mq_ops ublk_batch_mq_ops = {
 	.queue_rqs      = ublk_batch_queue_rqs,
 	.init_hctx	= ublk_init_hctx,
 	.timeout	= ublk_timeout,
+	.complete	= ublk_end_rq,
 };
 
 static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index 33b25dd13965..47b5fba9519c 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -420,6 +420,14 @@ struct ublk_shmem_buf_reg {
 /* ublksrv_io_desc size is specified by ublksrv_ctrl_dev_info's io_desc_size */
 #define UBLK_F_IO_DESC_SIZE (1ULL << 20)
 
+/*
+ * Request completion honors the block device's rq_affinity setting
+ * (/sys/block/ublkbN/queue/rq_affinity): the submitting CPU's completion
+ * work can run there instead of always on the ublk server's CPU. Without
+ * this feature, rq_affinity has no effect on ublk devices.
+ */
+#define UBLK_F_SUPPORT_RQ_AFFINITY (1ULL << 21)
+
 /* device state */
 #define UBLK_S_DEV_DEAD	0
 #define UBLK_S_DEV_LIVE	1
-- 
2.50.1 (Apple Git-155)


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

* [PATCH v2 2/2] selftests: ublk: add rq_affinity test
  2026-10-06 19:03 [PATCH v2 0/2] ublk: honor rq_affinity on request completion Yoav Cohen
  2026-10-06 19:03 ` [PATCH v2 1/2] " Yoav Cohen
@ 2026-10-06 19:03 ` Yoav Cohen
  1 sibling, 0 replies; 7+ messages in thread
From: Yoav Cohen @ 2026-10-06 19:03 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-block, csander; +Cc: jholzman, omril, Yoav Cohen

Add UBLK_F_SUPPORT_RQ_AFFINITY to kublk's feature table and a
--rq_affinity flag to request it at ADD_DEV time, and add
test_generic_18.sh: writes and verifies data on a loop device with
I/O pinned to one CPU while cycling rq_affinity through 0, 1 and 2.
Skipped on kernels without UBLK_F_SUPPORT_RQ_AFFINITY.

Signed-off-by: Yoav Cohen <yoav@nvidia.com>
---
 tools/testing/selftests/ublk/Makefile         |  1 +
 tools/testing/selftests/ublk/kublk.c          |  6 ++-
 .../testing/selftests/ublk/test_generic_18.sh | 42 +++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/ublk/test_generic_18.sh

diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
index 37883e9d50ec..d12e9879b89a 100644
--- a/tools/testing/selftests/ublk/Makefile
+++ b/tools/testing/selftests/ublk/Makefile
@@ -19,6 +19,7 @@ TEST_PROGS += test_generic_12.sh
 TEST_PROGS += test_generic_13.sh
 TEST_PROGS += test_generic_16.sh
 TEST_PROGS += test_generic_17.sh
+TEST_PROGS += test_generic_18.sh
 
 TEST_PROGS += test_batch_01.sh
 TEST_PROGS += test_batch_02.sh
diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
index 2400b4615766..e3afef0e7628 100644
--- a/tools/testing/selftests/ublk/kublk.c
+++ b/tools/testing/selftests/ublk/kublk.c
@@ -2083,6 +2083,7 @@ static int cmd_dev_get_features(void)
 		FEAT_NAME(UBLK_F_NO_AUTO_PART_SCAN),
 		FEAT_NAME(UBLK_F_SHMEM_ZC),
 		FEAT_NAME(UBLK_F_IO_DESC_SIZE),
+		FEAT_NAME(UBLK_F_SUPPORT_RQ_AFFINITY),
 	};
 	struct ublk_dev *dev;
 	__u64 features = 0;
@@ -2180,7 +2181,7 @@ static void __cmd_create_help(char *exe, bool recovery)
 	printf("\t[--nthreads threads] [--per_io_tasks]\n");
 	printf("\t[--integrity_capable] [--integrity_reftag] [--metadata_size SIZE] "
 		 "[--pi_offset OFFSET] [--csum_type ip|t10dif|nvme] [--tag_size SIZE]\n");
-	printf("\t[--batch|-b] [--rotate_auto_buf] [--no_auto_part_scan]\n");
+	printf("\t[--batch|-b] [--rotate_auto_buf] [--no_auto_part_scan] [--rq_affinity]\n");
 	printf("\t[--io_desc_size SIZE]\n");
 	printf("\t[target options] [backfile1] [backfile2] ...\n");
 	printf("\tdefault: nr_queues=2(max 32), depth=128(max 1024), dev_id=-1(auto allocation)\n");
@@ -2260,6 +2261,7 @@ int main(int argc, char *argv[])
 		{ "batch",              0,      NULL, 'b'},
 		{ "rotate_auto_buf",	0,	NULL,  0 },
 		{ "no_auto_part_scan",	0,	NULL,  0 },
+		{ "rq_affinity",	0,	NULL,  0 },
 		{ "shmem_zc",		0,	NULL,  0  },
 		{ "htlb",		1,	NULL,  0  },
 		{ "rdonly_shmem_buf",	0,	NULL,  0  },
@@ -2407,6 +2409,8 @@ int main(int argc, char *argv[])
 				ctx.safe_stop = 1;
 			if (!strcmp(longopts[option_idx].name, "no_auto_part_scan"))
 				ctx.flags |= UBLK_F_NO_AUTO_PART_SCAN;
+			if (!strcmp(longopts[option_idx].name, "rq_affinity"))
+				ctx.flags |= UBLK_F_SUPPORT_RQ_AFFINITY;
 			if (!strcmp(longopts[option_idx].name, "shmem_zc"))
 				ctx.flags |= UBLK_F_SHMEM_ZC;
 			if (!strcmp(longopts[option_idx].name, "htlb"))
diff --git a/tools/testing/selftests/ublk/test_generic_18.sh b/tools/testing/selftests/ublk/test_generic_18.sh
new file mode 100755
index 000000000000..c899a5bf0364
--- /dev/null
+++ b/tools/testing/selftests/ublk/test_generic_18.sh
@@ -0,0 +1,42 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
+
+ERR_CODE=0
+
+if ! _have_program fio; then
+	exit "$UBLK_SKIP_CODE"
+fi
+
+if ! _have_feature "SUPPORT_RQ_AFFINITY"; then
+	exit "$UBLK_SKIP_CODE"
+fi
+
+_prep_test "loop" "write and verify I/O across rq_affinity settings"
+
+_create_backfile 0 256M
+
+dev_id=$(_add_ublk_dev -t loop --rq_affinity "${UBLK_BACKFILES[0]}")
+_check_add_dev $TID $?
+
+rq_affinity_path=/sys/block/ublkb"${dev_id}"/queue/rq_affinity
+
+for affinity in 0 1 2; do
+	echo "${affinity}" > "${rq_affinity_path}"
+
+	# Pin submission to a single CPU so rq_affinity has somewhere to
+	# redirect completion to.
+	taskset -c 0 fio --name=verify --rw=randwrite --direct=1 \
+		--ioengine=libaio --bs=8k --iodepth=32 --verify=crc32c \
+		--do_verify=1 --verify_state_save=0 \
+		--filename=/dev/ublkb"${dev_id}" --size=256M > /dev/null
+	if [ $? -ne 0 ]; then
+		echo "fio verify failed with rq_affinity=${affinity}"
+		ERR_CODE=255
+		break
+	fi
+done
+
+_cleanup_test
+_show_result $TID $ERR_CODE
-- 
2.50.1 (Apple Git-155)


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

* Re: [PATCH v2 1/2] ublk: honor rq_affinity on request completion
  2026-10-06 19:03 ` [PATCH v2 1/2] " Yoav Cohen
@ 2026-10-06 21:53   ` Caleb Sander Mateos
  2026-10-09 13:43     ` Ming Lei
  2026-10-09 14:04   ` Ming Lei
  1 sibling, 1 reply; 7+ messages in thread
From: Caleb Sander Mateos @ 2026-10-06 21:53 UTC (permalink / raw)
  To: Yoav Cohen; +Cc: Ming Lei, Jens Axboe, linux-block, jholzman, omril

On Tue, Oct 6, 2026 at 12:04 PM Yoav Cohen <yoav@nvidia.com> wrote:
>
> ublk ends successfully completed requests inline on the server thread
> that issued UBLK_IO_COMMIT_AND_FETCH_REQ, so rq_affinity never
> redirects completion back to the submitting CPU, unlike NVMe, SCSI,
> virtio-blk, loop, nbd and rnbd.
>
> Route completion through blk_mq_complete_request_remote(), with a new
> ->complete() callback, ublk_end_rq(). This covers every completion
> outcome -- reads, writes, flush/discard, errors, and the zero-copy/
> user-copy/auto-buf-reg/shmem-zc paths that skip the copy-back step --
> not just the classic-copy-mode successful-read case, since
> ublk_end_rq() re-derives which of those __ublk_complete_rq() took from
> @req and @io state rather than assuming one.
>
> Gate it behind a new UBLK_F_SUPPORT_RQ_AFFINITY flag that a server
> must request at ADD_DEV time; servers that don't ask for it keep
> today's always-local completion unchanged. Add ublk_support_rq_affinity()
> alongside the existing ublk_support_*() helpers, and check it before
> calling blk_mq_complete_request_remote() at both call sites.
>
> Signed-off-by: Yoav Cohen <yoav@nvidia.com>
> ---
>  drivers/block/ublk_drv.c      | 98 +++++++++++++++++++++++++++--------
>  include/uapi/linux/ublk_cmd.h |  8 +++
>  2 files changed, 84 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 66eb55e7162e..b94293296b73 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -90,7 +90,8 @@
>                 | UBLK_F_BATCH_IO \
>                 | UBLK_F_NO_AUTO_PART_SCAN \
>                 | UBLK_F_SHMEM_ZC \
> -               | UBLK_F_IO_DESC_SIZE)
> +               | UBLK_F_IO_DESC_SIZE \
> +               | UBLK_F_SUPPORT_RQ_AFFINITY)
>
>  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
>                 | UBLK_F_USER_RECOVERY_REISSUE \
> @@ -452,6 +453,11 @@ static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
>         return ubq->flags & UBLK_F_USER_COPY;
>  }
>
> +static inline bool ublk_support_rq_affinity(const struct ublk_queue *ubq)
> +{
> +       return ubq->flags & UBLK_F_SUPPORT_RQ_AFFINITY;
> +}
> +
>  static inline bool ublk_dev_support_user_copy(const struct ublk_device *ub)
>  {
>         return ub->dev_info.flags & UBLK_F_USER_COPY;
> @@ -1468,6 +1474,14 @@ static inline bool ublk_need_unmap_req(const struct request *req)
>                (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
>  }
>
> +/* Whether __ublk_complete_rq() takes the copy-back/partial-completion path */
> +static inline bool ublk_rq_need_unmap(const struct ublk_queue *ubq,
> +                                     struct request *req)
> +{
> +       return ublk_need_map_io(ubq) && ublk_need_unmap_req(req) &&
> +              !ublk_iod_is_shmem_zc(ubq, req->tag);
> +}
> +
>  static unsigned int ublk_map_io(const struct request *req,
>                                 const struct ublk_io *io)
>  {
> @@ -1549,13 +1563,62 @@ static void ublk_end_request(struct request *req, blk_status_t error)
>         local_bh_enable();
>  }
>
> +/*
> + * Update @req with its result and requeue it if it was only partially
> + * completed. Returns true if @req was requeued, in which case the caller
> + * must not touch it any further.
> + *
> + * Run bio->bi_end_io() with softirqs disabled. If the final fput happens
> + * off this path, then that will prevent ublk's blkdev_release() from
> + * being called on current's task work, see fput() implementation.
> + *
> + * This matters for the caller completing @req locally on the ublk
> + * server's own thread: it may already be holding disk->open_mutex, e.g.
> + * reading the partition table from bdev_open(), and an fput() running
> + * inline there could deadlock on it. Preferably we would not be doing
> + * IO with a mutex held that is also used for release, but this
> + * work-around will suffice for now. A caller reached instead via
> + * blk_mq_complete_request_remote()'s softirq/IPI redirect never runs on
> + * that thread, so it isn't exposed to this hazard, but disabling
> + * softirqs here is harmless for it too.
> + */
> +static inline bool ublk_update_and_requeue(struct request *req,
> +                                          struct ublk_io *io)
> +{
> +       bool requeue;
> +
> +       local_bh_disable();
> +       requeue = blk_update_request(req, BLK_STS_OK, io->res);
> +       local_bh_enable();
> +       if (requeue)
> +               blk_mq_requeue_request(req, true);
> +       return requeue;
> +}
> +
> +static void ublk_end_rq(struct request *req)
> +{
> +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> +       struct ublk_io *io = &ubq->ios[req->tag];
> +
> +       /* matches the two outcomes __ublk_complete_rq() may have redirected */
> +       if (io->res >= 0 && ublk_rq_need_unmap(ubq, req)) {
> +               if (!ublk_update_and_requeue(req, io) &&
> +                   likely(!blk_should_fake_timeout(req->q)))
> +                       __blk_mq_end_request(req, BLK_STS_OK);
> +               return;
> +       }
> +
> +       ublk_end_request(req, io->res < 0 ? errno_to_blk_status(io->res) :
> +                                           BLK_STS_OK);
> +}
> +
>  /* todo: handle partial completion */
>  static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
>                                       bool need_map, struct io_comp_batch *iob)
>  {
> +       struct ublk_queue *ubq = req->mq_hctx->driver_data;

mq_hctx->driver_data and ubq->flags are likely to both be cache misses
on a ublk server thread servicing many queues. req->mq_hctx is
probably a hit as it's in the same cache line as req->cmd_flags
accessed just before in ublk_ch_uring_cmd_local() for the req_op(req)
== REQ_OP_ZONE_APPEND check. It would be nice to avoid these
additional dereferences, can we prevent queue_rq_affinity_store() from
setting rq_affinity to a value other than 0 if
UBLK_F_SUPPORT_RQ_AFFINITY isn't enabled? Perhaps indicated via a new
BLK_FEAT_SUPPORTS_RQ_AFFINITY flag in struct queue_limits's features
field?

Best,
Caleb

>         unsigned int unmapped_bytes;
>         blk_status_t res = BLK_STS_OK;
> -       bool requeue;
>
>         /* failed read IO if nothing is read */
>         if (!io->res && req_op(req) == REQ_OP_READ)
> @@ -1568,7 +1631,7 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
>
>         /* shmem zero copy: no data to unmap, pages already shared */
>         if (!need_map || !ublk_need_unmap_req(req) ||
> -           ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))
> +           ublk_iod_is_shmem_zc(ubq, req->tag))
>                 goto exit;
>
>         /* for READ request, writing data in iod->addr to rq buffers */
> @@ -1582,31 +1645,18 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
>         if (unlikely(unmapped_bytes < io->res)) {
>                 if (unlikely(!unmapped_bytes)) {
>                         res = BLK_STS_IOERR;
> +                       io->res = -EIO;
>                         goto exit;
>                 }
>
>                 io->res = unmapped_bytes;
>         }
>
> -       /*
> -        * Run bio->bi_end_io() with softirqs disabled. If the final fput
> -        * happens off this path, then that will prevent ublk's blkdev_release()
> -        * from being called on current's task work, see fput() implementation.
> -        *
> -        * Otherwise, ublk server may not provide forward progress in case of
> -        * reading the partition table from bdev_open() with disk->open_mutex
> -        * held, and causes dead lock as we could already be holding
> -        * disk->open_mutex here.
> -        *
> -        * Preferably we would not be doing IO with a mutex held that is also
> -        * used for release, but this work-around will suffice for now.
> -        */
> -       local_bh_disable();
> -       requeue = blk_update_request(req, BLK_STS_OK, io->res);
> -       local_bh_enable();
> -       if (requeue)
> -               blk_mq_requeue_request(req, true);
> -       else if (likely(!blk_should_fake_timeout(req->q))) {
> +       if (ublk_support_rq_affinity(ubq) && blk_mq_complete_request_remote(req))
> +               return;
> +
> +       if (!ublk_update_and_requeue(req, io) &&
> +           likely(!blk_should_fake_timeout(req->q))) {
>                 if (blk_mq_add_to_batch(req, iob, false, blk_mq_end_request_batch))
>                         return;
>                 __blk_mq_end_request(req, BLK_STS_OK);
> @@ -1614,6 +1664,8 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
>
>         return;
>  exit:
> +       if (ublk_support_rq_affinity(ubq) && blk_mq_complete_request_remote(req))
> +               return;
>         ublk_end_request(req, res);
>  }
>
> @@ -2347,6 +2399,7 @@ static const struct blk_mq_ops ublk_mq_ops = {
>         .queue_rqs      = ublk_queue_rqs,
>         .init_hctx      = ublk_init_hctx,
>         .timeout        = ublk_timeout,
> +       .complete       = ublk_end_rq,
>  };
>
>  static const struct blk_mq_ops ublk_batch_mq_ops = {
> @@ -2355,6 +2408,7 @@ static const struct blk_mq_ops ublk_batch_mq_ops = {
>         .queue_rqs      = ublk_batch_queue_rqs,
>         .init_hctx      = ublk_init_hctx,
>         .timeout        = ublk_timeout,
> +       .complete       = ublk_end_rq,
>  };
>
>  static void ublk_queue_reinit(struct ublk_device *ub, struct ublk_queue *ubq)
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 33b25dd13965..47b5fba9519c 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -420,6 +420,14 @@ struct ublk_shmem_buf_reg {
>  /* ublksrv_io_desc size is specified by ublksrv_ctrl_dev_info's io_desc_size */
>  #define UBLK_F_IO_DESC_SIZE (1ULL << 20)
>
> +/*
> + * Request completion honors the block device's rq_affinity setting
> + * (/sys/block/ublkbN/queue/rq_affinity): the submitting CPU's completion
> + * work can run there instead of always on the ublk server's CPU. Without
> + * this feature, rq_affinity has no effect on ublk devices.
> + */
> +#define UBLK_F_SUPPORT_RQ_AFFINITY (1ULL << 21)
> +
>  /* device state */
>  #define UBLK_S_DEV_DEAD        0
>  #define UBLK_S_DEV_LIVE        1
> --
> 2.50.1 (Apple Git-155)
>

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

* Re: [PATCH v2 1/2] ublk: honor rq_affinity on request completion
  2026-10-06 21:53   ` Caleb Sander Mateos
@ 2026-10-09 13:43     ` Ming Lei
  2026-10-09 15:25       ` Caleb Sander Mateos
  0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2026-10-09 13:43 UTC (permalink / raw)
  To: Caleb Sander Mateos; +Cc: Yoav Cohen, Jens Axboe, linux-block, jholzman, omril

On Tue, Oct 06, 2026 at 02:53:47PM -0700, Caleb Sander Mateos wrote:
> On Tue, Oct 6, 2026 at 12:04 PM Yoav Cohen <yoav@nvidia.com> wrote:
> >
> > ublk ends successfully completed requests inline on the server thread
> > that issued UBLK_IO_COMMIT_AND_FETCH_REQ, so rq_affinity never
> > redirects completion back to the submitting CPU, unlike NVMe, SCSI,
> > virtio-blk, loop, nbd and rnbd.
> >
> > Route completion through blk_mq_complete_request_remote(), with a new
> > ->complete() callback, ublk_end_rq(). This covers every completion
> > outcome -- reads, writes, flush/discard, errors, and the zero-copy/
> > user-copy/auto-buf-reg/shmem-zc paths that skip the copy-back step --
> > not just the classic-copy-mode successful-read case, since
> > ublk_end_rq() re-derives which of those __ublk_complete_rq() took from
> > @req and @io state rather than assuming one.
> >
> > Gate it behind a new UBLK_F_SUPPORT_RQ_AFFINITY flag that a server
> > must request at ADD_DEV time; servers that don't ask for it keep
> > today's always-local completion unchanged. Add ublk_support_rq_affinity()
> > alongside the existing ublk_support_*() helpers, and check it before
> > calling blk_mq_complete_request_remote() at both call sites.
> >
> > Signed-off-by: Yoav Cohen <yoav@nvidia.com>
> > ---
> >  drivers/block/ublk_drv.c      | 98 +++++++++++++++++++++++++++--------
> >  include/uapi/linux/ublk_cmd.h |  8 +++
> >  2 files changed, 84 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 66eb55e7162e..b94293296b73 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -90,7 +90,8 @@
> >                 | UBLK_F_BATCH_IO \
> >                 | UBLK_F_NO_AUTO_PART_SCAN \
> >                 | UBLK_F_SHMEM_ZC \
> > -               | UBLK_F_IO_DESC_SIZE)
> > +               | UBLK_F_IO_DESC_SIZE \
> > +               | UBLK_F_SUPPORT_RQ_AFFINITY)
> >
> >  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> >                 | UBLK_F_USER_RECOVERY_REISSUE \
> > @@ -452,6 +453,11 @@ static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> >         return ubq->flags & UBLK_F_USER_COPY;
> >  }
> >
> > +static inline bool ublk_support_rq_affinity(const struct ublk_queue *ubq)
> > +{
> > +       return ubq->flags & UBLK_F_SUPPORT_RQ_AFFINITY;
> > +}
> > +
> >  static inline bool ublk_dev_support_user_copy(const struct ublk_device *ub)
> >  {
> >         return ub->dev_info.flags & UBLK_F_USER_COPY;
> > @@ -1468,6 +1474,14 @@ static inline bool ublk_need_unmap_req(const struct request *req)
> >                (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
> >  }
> >
> > +/* Whether __ublk_complete_rq() takes the copy-back/partial-completion path */
> > +static inline bool ublk_rq_need_unmap(const struct ublk_queue *ubq,
> > +                                     struct request *req)
> > +{
> > +       return ublk_need_map_io(ubq) && ublk_need_unmap_req(req) &&
> > +              !ublk_iod_is_shmem_zc(ubq, req->tag);
> > +}
> > +
> >  static unsigned int ublk_map_io(const struct request *req,
> >                                 const struct ublk_io *io)
> >  {
> > @@ -1549,13 +1563,62 @@ static void ublk_end_request(struct request *req, blk_status_t error)
> >         local_bh_enable();
> >  }
> >
> > +/*
> > + * Update @req with its result and requeue it if it was only partially
> > + * completed. Returns true if @req was requeued, in which case the caller
> > + * must not touch it any further.
> > + *
> > + * Run bio->bi_end_io() with softirqs disabled. If the final fput happens
> > + * off this path, then that will prevent ublk's blkdev_release() from
> > + * being called on current's task work, see fput() implementation.
> > + *
> > + * This matters for the caller completing @req locally on the ublk
> > + * server's own thread: it may already be holding disk->open_mutex, e.g.
> > + * reading the partition table from bdev_open(), and an fput() running
> > + * inline there could deadlock on it. Preferably we would not be doing
> > + * IO with a mutex held that is also used for release, but this
> > + * work-around will suffice for now. A caller reached instead via
> > + * blk_mq_complete_request_remote()'s softirq/IPI redirect never runs on
> > + * that thread, so it isn't exposed to this hazard, but disabling
> > + * softirqs here is harmless for it too.
> > + */
> > +static inline bool ublk_update_and_requeue(struct request *req,
> > +                                          struct ublk_io *io)
> > +{
> > +       bool requeue;
> > +
> > +       local_bh_disable();
> > +       requeue = blk_update_request(req, BLK_STS_OK, io->res);
> > +       local_bh_enable();
> > +       if (requeue)
> > +               blk_mq_requeue_request(req, true);
> > +       return requeue;
> > +}
> > +
> > +static void ublk_end_rq(struct request *req)
> > +{
> > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> > +       struct ublk_io *io = &ubq->ios[req->tag];
> > +
> > +       /* matches the two outcomes __ublk_complete_rq() may have redirected */
> > +       if (io->res >= 0 && ublk_rq_need_unmap(ubq, req)) {
> > +               if (!ublk_update_and_requeue(req, io) &&
> > +                   likely(!blk_should_fake_timeout(req->q)))
> > +                       __blk_mq_end_request(req, BLK_STS_OK);
> > +               return;
> > +       }
> > +
> > +       ublk_end_request(req, io->res < 0 ? errno_to_blk_status(io->res) :
> > +                                           BLK_STS_OK);
> > +}
> > +
> >  /* todo: handle partial completion */
> >  static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
> >                                       bool need_map, struct io_comp_batch *iob)
> >  {
> > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> 
> mq_hctx->driver_data and ubq->flags are likely to both be cache misses
> on a ublk server thread servicing many queues. req->mq_hctx is
> probably a hit as it's in the same cache line as req->cmd_flags
> accessed just before in ublk_ch_uring_cmd_local() for the req_op(req)
> == REQ_OP_ZONE_APPEND check. It would be nice to avoid these
> additional dereferences, can we prevent queue_rq_affinity_store() from
> setting rq_affinity to a value other than 0 if
> UBLK_F_SUPPORT_RQ_AFFINITY isn't enabled? Perhaps indicated via a new
> BLK_FEAT_SUPPORTS_RQ_AFFINITY flag in struct queue_limits's features
> field?

ubq is only for ublk_support_rq_affinity(ubq), `maybe rq_affinity` can be
passed by __ublk_complete_rq() parameter via
ublk_dev_support_rq_affinity(ub).

BTW, the one for ublk_iod_is_shmem_zc() can't be avoided.


Thanks,
Ming

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

* Re: [PATCH v2 1/2] ublk: honor rq_affinity on request completion
  2026-10-06 19:03 ` [PATCH v2 1/2] " Yoav Cohen
  2026-10-06 21:53   ` Caleb Sander Mateos
@ 2026-10-09 14:04   ` Ming Lei
  1 sibling, 0 replies; 7+ messages in thread
From: Ming Lei @ 2026-10-09 14:04 UTC (permalink / raw)
  To: Yoav Cohen; +Cc: Jens Axboe, linux-block, csander, jholzman, omril

On Tue, Oct 06, 2026 at 10:03:47PM +0300, Yoav Cohen wrote:
> ublk ends successfully completed requests inline on the server thread
> that issued UBLK_IO_COMMIT_AND_FETCH_REQ, so rq_affinity never
> redirects completion back to the submitting CPU, unlike NVMe, SCSI,
> virtio-blk, loop, nbd and rnbd.
> 
> Route completion through blk_mq_complete_request_remote(), with a new
> ->complete() callback, ublk_end_rq(). This covers every completion
> outcome -- reads, writes, flush/discard, errors, and the zero-copy/
> user-copy/auto-buf-reg/shmem-zc paths that skip the copy-back step --
> not just the classic-copy-mode successful-read case, since
> ublk_end_rq() re-derives which of those __ublk_complete_rq() took from
> @req and @io state rather than assuming one.
> 
> Gate it behind a new UBLK_F_SUPPORT_RQ_AFFINITY flag that a server
> must request at ADD_DEV time; servers that don't ask for it keep
> today's always-local completion unchanged. Add ublk_support_rq_affinity()
> alongside the existing ublk_support_*() helpers, and check it before
> calling blk_mq_complete_request_remote() at both call sites.
> 
> Signed-off-by: Yoav Cohen <yoav@nvidia.com>
> ---
>  drivers/block/ublk_drv.c      | 98 +++++++++++++++++++++++++++--------
>  include/uapi/linux/ublk_cmd.h |  8 +++
>  2 files changed, 84 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 66eb55e7162e..b94293296b73 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -90,7 +90,8 @@
>  		| UBLK_F_BATCH_IO \
>  		| UBLK_F_NO_AUTO_PART_SCAN \
>  		| UBLK_F_SHMEM_ZC \
> -		| UBLK_F_IO_DESC_SIZE)
> +		| UBLK_F_IO_DESC_SIZE \
> +		| UBLK_F_SUPPORT_RQ_AFFINITY)
>  
>  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
>  		| UBLK_F_USER_RECOVERY_REISSUE \
> @@ -452,6 +453,11 @@ static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
>  	return ubq->flags & UBLK_F_USER_COPY;
>  }
>  
> +static inline bool ublk_support_rq_affinity(const struct ublk_queue *ubq)
> +{
> +	return ubq->flags & UBLK_F_SUPPORT_RQ_AFFINITY;
> +}
> +
>  static inline bool ublk_dev_support_user_copy(const struct ublk_device *ub)
>  {
>  	return ub->dev_info.flags & UBLK_F_USER_COPY;
> @@ -1468,6 +1474,14 @@ static inline bool ublk_need_unmap_req(const struct request *req)
>  	       (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
>  }
>  
> +/* Whether __ublk_complete_rq() takes the copy-back/partial-completion path */
> +static inline bool ublk_rq_need_unmap(const struct ublk_queue *ubq,
> +				      struct request *req)
> +{
> +	return ublk_need_map_io(ubq) && ublk_need_unmap_req(req) &&
> +	       !ublk_iod_is_shmem_zc(ubq, req->tag);
> +}
> +
>  static unsigned int ublk_map_io(const struct request *req,
>  				const struct ublk_io *io)
>  {
> @@ -1549,13 +1563,62 @@ static void ublk_end_request(struct request *req, blk_status_t error)
>  	local_bh_enable();
>  }
>  
> +/*
> + * Update @req with its result and requeue it if it was only partially
> + * completed. Returns true if @req was requeued, in which case the caller
> + * must not touch it any further.
> + *
> + * Run bio->bi_end_io() with softirqs disabled. If the final fput happens
> + * off this path, then that will prevent ublk's blkdev_release() from
> + * being called on current's task work, see fput() implementation.
> + *
> + * This matters for the caller completing @req locally on the ublk
> + * server's own thread: it may already be holding disk->open_mutex, e.g.
> + * reading the partition table from bdev_open(), and an fput() running
> + * inline there could deadlock on it. Preferably we would not be doing
> + * IO with a mutex held that is also used for release, but this
> + * work-around will suffice for now. A caller reached instead via
> + * blk_mq_complete_request_remote()'s softirq/IPI redirect never runs on
> + * that thread, so it isn't exposed to this hazard, but disabling
> + * softirqs here is harmless for it too.
> + */
> +static inline bool ublk_update_and_requeue(struct request *req,
> +					   struct ublk_io *io)
> +{
> +	bool requeue;
> +
> +	local_bh_disable();
> +	requeue = blk_update_request(req, BLK_STS_OK, io->res);
> +	local_bh_enable();
> +	if (requeue)
> +		blk_mq_requeue_request(req, true);
> +	return requeue;
> +}
> +
> +static void ublk_end_rq(struct request *req)
> +{
> +	struct ublk_queue *ubq = req->mq_hctx->driver_data;
> +	struct ublk_io *io = &ubq->ios[req->tag];
> +
> +	/* matches the two outcomes __ublk_complete_rq() may have redirected */
> +	if (io->res >= 0 && ublk_rq_need_unmap(ubq, req)) {
> +		if (!ublk_update_and_requeue(req, io) &&
> +		    likely(!blk_should_fake_timeout(req->q)))
> +			__blk_mq_end_request(req, BLK_STS_OK);
> +		return;
> +	}

ublk_end_rq() may be run from remove cpu, then it becomes fragile to
touch `io` here, and not efficient to touch `ubq` as Caleb mentioned.

It could be simplified a lot to just deal with normal io->res == blk_rq_bytes(req)
here, meantime keep everything else(short write, failure, ...) in __ublk_complete_rq().


Thanks, 
Ming

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

* Re: [PATCH v2 1/2] ublk: honor rq_affinity on request completion
  2026-10-09 13:43     ` Ming Lei
@ 2026-10-09 15:25       ` Caleb Sander Mateos
  0 siblings, 0 replies; 7+ messages in thread
From: Caleb Sander Mateos @ 2026-10-09 15:25 UTC (permalink / raw)
  To: Ming Lei; +Cc: Yoav Cohen, Jens Axboe, linux-block, jholzman, omril

On Fri, Oct 9, 2026 at 6:43 AM Ming Lei <tom.leiming@gmail.com> wrote:
>
> On Tue, Oct 06, 2026 at 02:53:47PM -0700, Caleb Sander Mateos wrote:
> > On Tue, Oct 6, 2026 at 12:04 PM Yoav Cohen <yoav@nvidia.com> wrote:
> > >
> > > ublk ends successfully completed requests inline on the server thread
> > > that issued UBLK_IO_COMMIT_AND_FETCH_REQ, so rq_affinity never
> > > redirects completion back to the submitting CPU, unlike NVMe, SCSI,
> > > virtio-blk, loop, nbd and rnbd.
> > >
> > > Route completion through blk_mq_complete_request_remote(), with a new
> > > ->complete() callback, ublk_end_rq(). This covers every completion
> > > outcome -- reads, writes, flush/discard, errors, and the zero-copy/
> > > user-copy/auto-buf-reg/shmem-zc paths that skip the copy-back step --
> > > not just the classic-copy-mode successful-read case, since
> > > ublk_end_rq() re-derives which of those __ublk_complete_rq() took from
> > > @req and @io state rather than assuming one.
> > >
> > > Gate it behind a new UBLK_F_SUPPORT_RQ_AFFINITY flag that a server
> > > must request at ADD_DEV time; servers that don't ask for it keep
> > > today's always-local completion unchanged. Add ublk_support_rq_affinity()
> > > alongside the existing ublk_support_*() helpers, and check it before
> > > calling blk_mq_complete_request_remote() at both call sites.
> > >
> > > Signed-off-by: Yoav Cohen <yoav@nvidia.com>
> > > ---
> > >  drivers/block/ublk_drv.c      | 98 +++++++++++++++++++++++++++--------
> > >  include/uapi/linux/ublk_cmd.h |  8 +++
> > >  2 files changed, 84 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 66eb55e7162e..b94293296b73 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -90,7 +90,8 @@
> > >                 | UBLK_F_BATCH_IO \
> > >                 | UBLK_F_NO_AUTO_PART_SCAN \
> > >                 | UBLK_F_SHMEM_ZC \
> > > -               | UBLK_F_IO_DESC_SIZE)
> > > +               | UBLK_F_IO_DESC_SIZE \
> > > +               | UBLK_F_SUPPORT_RQ_AFFINITY)
> > >
> > >  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> > >                 | UBLK_F_USER_RECOVERY_REISSUE \
> > > @@ -452,6 +453,11 @@ static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> > >         return ubq->flags & UBLK_F_USER_COPY;
> > >  }
> > >
> > > +static inline bool ublk_support_rq_affinity(const struct ublk_queue *ubq)
> > > +{
> > > +       return ubq->flags & UBLK_F_SUPPORT_RQ_AFFINITY;
> > > +}
> > > +
> > >  static inline bool ublk_dev_support_user_copy(const struct ublk_device *ub)
> > >  {
> > >         return ub->dev_info.flags & UBLK_F_USER_COPY;
> > > @@ -1468,6 +1474,14 @@ static inline bool ublk_need_unmap_req(const struct request *req)
> > >                (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
> > >  }
> > >
> > > +/* Whether __ublk_complete_rq() takes the copy-back/partial-completion path */
> > > +static inline bool ublk_rq_need_unmap(const struct ublk_queue *ubq,
> > > +                                     struct request *req)
> > > +{
> > > +       return ublk_need_map_io(ubq) && ublk_need_unmap_req(req) &&
> > > +              !ublk_iod_is_shmem_zc(ubq, req->tag);
> > > +}
> > > +
> > >  static unsigned int ublk_map_io(const struct request *req,
> > >                                 const struct ublk_io *io)
> > >  {
> > > @@ -1549,13 +1563,62 @@ static void ublk_end_request(struct request *req, blk_status_t error)
> > >         local_bh_enable();
> > >  }
> > >
> > > +/*
> > > + * Update @req with its result and requeue it if it was only partially
> > > + * completed. Returns true if @req was requeued, in which case the caller
> > > + * must not touch it any further.
> > > + *
> > > + * Run bio->bi_end_io() with softirqs disabled. If the final fput happens
> > > + * off this path, then that will prevent ublk's blkdev_release() from
> > > + * being called on current's task work, see fput() implementation.
> > > + *
> > > + * This matters for the caller completing @req locally on the ublk
> > > + * server's own thread: it may already be holding disk->open_mutex, e.g.
> > > + * reading the partition table from bdev_open(), and an fput() running
> > > + * inline there could deadlock on it. Preferably we would not be doing
> > > + * IO with a mutex held that is also used for release, but this
> > > + * work-around will suffice for now. A caller reached instead via
> > > + * blk_mq_complete_request_remote()'s softirq/IPI redirect never runs on
> > > + * that thread, so it isn't exposed to this hazard, but disabling
> > > + * softirqs here is harmless for it too.
> > > + */
> > > +static inline bool ublk_update_and_requeue(struct request *req,
> > > +                                          struct ublk_io *io)
> > > +{
> > > +       bool requeue;
> > > +
> > > +       local_bh_disable();
> > > +       requeue = blk_update_request(req, BLK_STS_OK, io->res);
> > > +       local_bh_enable();
> > > +       if (requeue)
> > > +               blk_mq_requeue_request(req, true);
> > > +       return requeue;
> > > +}
> > > +
> > > +static void ublk_end_rq(struct request *req)
> > > +{
> > > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> > > +       struct ublk_io *io = &ubq->ios[req->tag];
> > > +
> > > +       /* matches the two outcomes __ublk_complete_rq() may have redirected */
> > > +       if (io->res >= 0 && ublk_rq_need_unmap(ubq, req)) {
> > > +               if (!ublk_update_and_requeue(req, io) &&
> > > +                   likely(!blk_should_fake_timeout(req->q)))
> > > +                       __blk_mq_end_request(req, BLK_STS_OK);
> > > +               return;
> > > +       }
> > > +
> > > +       ublk_end_request(req, io->res < 0 ? errno_to_blk_status(io->res) :
> > > +                                           BLK_STS_OK);
> > > +}
> > > +
> > >  /* todo: handle partial completion */
> > >  static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
> > >                                       bool need_map, struct io_comp_batch *iob)
> > >  {
> > > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> >
> > mq_hctx->driver_data and ubq->flags are likely to both be cache misses
> > on a ublk server thread servicing many queues. req->mq_hctx is
> > probably a hit as it's in the same cache line as req->cmd_flags
> > accessed just before in ublk_ch_uring_cmd_local() for the req_op(req)
> > == REQ_OP_ZONE_APPEND check. It would be nice to avoid these
> > additional dereferences, can we prevent queue_rq_affinity_store() from
> > setting rq_affinity to a value other than 0 if
> > UBLK_F_SUPPORT_RQ_AFFINITY isn't enabled? Perhaps indicated via a new
> > BLK_FEAT_SUPPORTS_RQ_AFFINITY flag in struct queue_limits's features
> > field?
>
> ubq is only for ublk_support_rq_affinity(ubq), `maybe rq_affinity` can be
> passed by __ublk_complete_rq() parameter via
> ublk_dev_support_rq_affinity(ub).
>
> BTW, the one for ublk_iod_is_shmem_zc() can't be avoided.

Yes, but ublk_iod_is_shmem_zc() is only called in the need_map case. I
would expect most modern ublk servers to enable user copy and/or zero
copy, so they wouldn't take that path.

Best,
Caleb

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

end of thread, other threads:[~2026-10-09 15:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-06 19:03 [PATCH v2 0/2] ublk: honor rq_affinity on request completion Yoav Cohen
2026-10-06 19:03 ` [PATCH v2 1/2] " Yoav Cohen
2026-10-06 21:53   ` Caleb Sander Mateos
2026-10-09 13:43     ` Ming Lei
2026-10-09 15:25       ` Caleb Sander Mateos
2026-10-09 14:04   ` Ming Lei
2026-10-06 19:03 ` [PATCH v2 2/2] selftests: ublk: add rq_affinity test Yoav Cohen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox