ublk zero-copy support

From: Keith Busch <[email protected]>

When a registered resource is about to be freed, check if it has registered a release function, and call it if so. This is preparing for resources that are related to an external component.

Signed-off-by Keith Busch <[email protected]>

---
  io_uring/rsrc.c | 2 ++
  io_uring/rsrc.h | 3 +++
  2 files changed, 5 insertions(+)

io_uring/rsrc.c +2 -0

index 4d0e1c06c8bc6..30f08cf13ef60 100644
@@ -455,6 +455,8 @@ void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
  	case IORING_RSRC_BUFFER:
  		if (node->buf)
  			io_buffer_unmap(ctx, node);
+		if (node->release)
+			node->release(node->priv);
  		break;
  	default:
  		WARN_ON_ONCE(1);

io_uring/rsrc.h +0 -0

index abd0d5d42c3e1..a3826ab84e666 100644

io_uring/rsrc.h +0 -0

  };
 
  struct io_mapped_ubuf {

io_uring/rsrc.h struct io_rsrc_node { · line 29

Quoted 7 lines · Keith Busch

io_uring/rsrc.h +3 -0

@@ -24,6 +24,9 @@ struct io_rsrc_node {
  		unsigned long file_ptr;
  		struct io_mapped_ubuf *buf;
  	};
+
+	void (*release)(void *);
+	void *priv;

Nodes are more generic than buffers. Unless we want to reuse it for files, and I very much doubt that, it should move into io_mapped_ubuf.

Quoted 12 lines · Pavel Begunkov

io_uring/rsrc.h struct io_rsrc_node { · line 29

Quoted 7 lines · Keith Busch

io_uring/rsrc.h +3 -0

@@ -24,6 +24,9 @@ struct io_rsrc_node {
   		unsigned long file_ptr;
   		struct io_mapped_ubuf *buf;
   	};
+
+	void (*release)(void *);
+	void *priv;

Nodes are more generic than buffers. Unless we want to reuse it for files, and I very much doubt that, it should move into io_mapped_ubuf.

Good point. Cloning is another reason it should be in the imu.

If we want to save space, the "KBUFFER" type doesn't need ubuf or folio_shift, so we could even union the new fields to prevent imu from getting any bigger. The downside would be we can't use "release" to distinguish kernel vs user buffers, which you suggested in that patch's thread. Which option do you prefer?

From: Keith Busch <[email protected]>

Provide an interface for the kernel to leverage the existing pre-registered buffers that io_uring provides. User space can reference these later to achieve zero-copy IO.

User space must register an empty fixed buffer table with io_uring in order for the kernel to make use of it.

Signed-off-by Keith Busch <[email protected]>

---
  include/linux/io_uring.h       |   1 +
  include/linux/io_uring_types.h |   4 ++
  io_uring/rsrc.c                | 100 +++++++++++++++++++++++++++++++--
  io_uring/rsrc.h                |   1 +
  4 files changed, 100 insertions(+), 6 deletions(-)

include/linux/io_uring.h +1 -0

index 85fe4e6b275c7..b5637a2aae340 100644
@@ -5,6 +5,7 @@
  #include <linux/sched.h>
  #include <linux/xarray.h>
  #include <uapi/linux/io_uring.h>
+#include <linux/blk-mq.h>
 
  #if defined(CONFIG_IO_URING)
  void __io_uring_cancel(bool cancel_all);

include/linux/io_uring_types.h +4 -0

index e2fef264ff8b8..99aac2d52fbae 100644
@@ -693,4 +693,8 @@ static inline bool io_ctx_cqe32(struct io_ring_ctx *ctx)
  	return ctx->flags & IORING_SETUP_CQE32;
  }
 
+int io_buffer_register_bvec(struct io_ring_ctx *ctx, struct request *rq,
+			    void (*release)(void *), unsigned int index);
+void io_buffer_unregister_bvec(struct io_ring_ctx *ctx, unsigned int tag);
+
  #endif

io_uring/rsrc.c +3 -2

index 30f08cf13ef60..14efec8587888 100644
@@ -110,8 +110,9 @@ static void io_buffer_unmap(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 
  		if (!refcount_dec_and_test(&imu->refs))
  			return;
-		for (i = 0; i < imu->nr_bvecs; i++)
-			unpin_user_page(imu->bvec[i].bv_page);
+		if (node->type == IORING_RSRC_BUFFER)
+			for (i = 0; i < imu->nr_bvecs; i++)
+				unpin_user_page(imu->bvec[i].bv_page);
  		if (imu->acct_pages)
  			io_unaccount_mem(ctx, imu->acct_pages);
  		kvfree(imu);

io_uring/rsrc.c +4 -0

+			err = -EBUSY;
+			break;
+		}
+

io_uring/rsrc.c +7 -0

+int io_buffer_register_bvec(struct io_ring_ctx *ctx, struct request *rq,
+			    void (*release)(void *), unsigned int index)
+{
+	struct io_rsrc_data *data = &ctx->buf_table;
+	struct req_iterator rq_iter;
+	struct io_mapped_ubuf *imu;
+	struct io_rsrc_node *node;

From: Keith Busch <[email protected]>

Provide an interface for the kernel to leverage the existing pre-registered buffers that io_uring provides. User space can reference these later to achieve zero-copy IO.

User space must register an empty fixed buffer table with io_uring in order for the kernel to make use of it.

Signed-off-by Keith Busch <[email protected]>

---

io_uring/rsrc.c +32 -0

+int io_buffer_register_bvec(struct io_ring_ctx *ctx, struct request *rq,
+			    void (*release)(void *), unsigned int index)
+{
+	struct io_rsrc_data *data = &ctx->buf_table;
+	struct req_iterator rq_iter;
+	struct io_mapped_ubuf *imu;
+	struct io_rsrc_node *node;
+	struct bio_vec bv;
+	u16 nr_bvecs;
+	int i = 0;
+
+	lockdep_assert_held(&ctx->uring_lock);
+
+	if (!data->nr)
+		return -EINVAL;
+	if (index >= data->nr)
+		return -EINVAL;
+
+	node = data->nodes[index];
+	if (node)
+		return -EBUSY;
+
+	node = io_rsrc_node_alloc(IORING_RSRC_KBUFFER);
+	if (!node)
+		return -ENOMEM;
+
+	node->release = release;
+	node->priv = rq;
+
+	nr_bvecs = blk_rq_nr_phys_segments(rq);
+	imu = kvmalloc(struct_size(imu, bvec, nr_bvecs), GFP_KERNEL);
+	if (!imu) {

Quoted 7 lines · Keith Busch

io_uring/rsrc.c +7 -0

+
+	node->release = release;
+	node->priv = rq;
+
+	nr_bvecs = blk_rq_nr_phys_segments(rq);
+	imu = kvmalloc(struct_size(imu, bvec, nr_bvecs), GFP_KERNEL);
+	if (!imu) {

io_uring/rsrc.c int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, · line 914

Quoted 10 lines

io_uring/rsrc.c +10 -0

+		kfree(node);
+		return -ENOMEM;
+	}
+
+	imu->ubuf = 0;
+	imu->len = blk_rq_bytes(rq);
+	imu->acct_pages = 0;
+	imu->nr_bvecs = nr_bvecs;
+	refcount_set(&imu->refs, 1);
+	node->buf = imu;

request buffer direction needs to be stored in imu, for READ, the buffer is write-only, and for WRITE, the buffer is read-only, which isn't different with user mapped buffer.

Meantime in read_fixed/write_fixed side or buffer lookup abstraction helper, the buffer direction needs to be validated.

Quoted 23 lines · Ming Lei

Quoted 7 lines · Keith Busch

io_uring/rsrc.c +7 -0

+
+	node->release = release;
+	node->priv = rq;
+
+	nr_bvecs = blk_rq_nr_phys_segments(rq);
+	imu = kvmalloc(struct_size(imu, bvec, nr_bvecs), GFP_KERNEL);
+	if (!imu) {

io_uring/rsrc.c int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, · line 914

Quoted 10 lines

io_uring/rsrc.c +10 -0

+		kfree(node);
+		return -ENOMEM;
+	}
+
+	imu->ubuf = 0;
+	imu->len = blk_rq_bytes(rq);
+	imu->acct_pages = 0;
+	imu->nr_bvecs = nr_bvecs;
+	refcount_set(&imu->refs, 1);
+	node->buf = imu;

request buffer direction needs to be stored in imu, for READ, the buffer is write-only, and for WRITE, the buffer is read-only, which isn't different with user mapped buffer.

Quoted 2 lines

Meantime in read_fixed/write_fixed side or buffer lookup abstraction helper, the buffer direction needs to be validated.

I suppose we could add that check, but the primary use case doesn't even use those operations. They're using uring_cmd with the FIXED flag, and io_uring can't readily validate the data direction from that interface.

Quoted 3 lines

I suppose we could add that check, but the primary use case doesn't even use those operations. They're using uring_cmd with the FIXED flag, and io_uring can't readily validate the data direction from that interface.

The check can be added to io_import_fixed().

From: Keith Busch <[email protected]>

Provide new operations for the user to request mapping an active request to an io uring instance's buf_table. The user has to provide the index it wants to install the buffer.

A reference count is taken on the request to ensure it can't be completed while it is active in a ring's buf_table.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/block/ublk_drv.c      | 145 +++++++++++++++++++++++++---------
 include/uapi/linux/ublk_cmd.h |   4 +
 2 files changed, 113 insertions(+), 36 deletions(-)

drivers/block/ublk_drv.c +0 -0

index 529085181f355..ccfda7b2c24da 100644

drivers/block/ublk_drv.c +33 -2

+
 /* All UBLK_F_* have to be included into UBLK_F_ALL */
 #define UBLK_F_ALL (UBLK_F_SUPPORT_ZERO_COPY \
 		| UBLK_F_URING_CMD_COMP_IN_TASK \

@@ -76,6 +79,9 @@ struct ublk_rq_data {
 	struct llist_node node;
 
 	struct kref ref;
+
+#define UBLK_ZC_REGISTERED 0
+	unsigned long flags;
 };
 
 struct ublk_uring_cmd_pdu {

@@ -201,7 +207,7 @@ static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
 						   int tag);
 static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
 {
-	return ub->dev_info.flags & UBLK_F_USER_COPY;
+	return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
 }
 
 static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)

@@ -581,7 +587,7 @@ static void ublk_apply_params(struct ublk_device *ub)
 
 static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
 {
-	return ubq->flags & UBLK_F_USER_COPY;
+	return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
 }
 
 static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)

@@ -1747,6 +1753,102 @@ static inline void ublk_prep_cancel(struct io_uring_cmd *cmd,
 	io_uring_cmd_mark_cancelable(cmd, issue_flags);
 }
 
+static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
+		struct ublk_queue *ubq, int tag, size_t offset)
+{
+	struct request *req;
+
+	if (!ublk_need_req_ref(ubq))
+		return NULL;
+
+	req = blk_mq_tag_to_rq(ub->tag_set.tags[ubq->q_id], tag);
+	if (!req)
+		return NULL;
+
+	if (!ublk_get_req_ref(ubq, req))
+		return NULL;
+
+	if (unlikely(!blk_mq_request_started(req) || req->tag != tag))
+		goto fail_put;
+
+	if (!ublk_rq_has_data(req))
+		goto fail_put;
+
+	if (offset > blk_rq_bytes(req))
+		goto fail_put;
+
+	return req;
+fail_put:
+	ublk_put_req_ref(ubq, req);

Quoted 20 lines · Keith Busch

From: Keith Busch <[email protected]>

Provide new operations for the user to request mapping an active request to an io uring instance's buf_table. The user has to provide the index it wants to install the buffer.

A reference count is taken on the request to ensure it can't be completed while it is active in a ring's buf_table.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/block/ublk_drv.c      | 145 +++++++++++++++++++++++++---------
 include/uapi/linux/ublk_cmd.h |   4 +
 2 files changed, 113 insertions(+), 36 deletions(-)

drivers/block/ublk_drv.c +0 -0

index 529085181f355..ccfda7b2c24da 100644

drivers/block/ublk_drv.c line 55

Quoted 5 lines

drivers/block/ublk_drv.c +2 -0

@@ -51,6 +51,9 @@
 /* private ioctl command mirror */
 #define UBLK_CMD_DEL_DEV_ASYNC	_IOC_NR(UBLK_U_CMD_DEL_DEV_ASYNC)
 
+#define UBLK_IO_REGISTER_IO_BUF		_IOC_NR(UBLK_U_IO_REGISTER_IO_BUF)
+#define UBLK_IO_UNREGISTER_IO_BUF	_IOC_NR(UBLK_U_IO_UNREGISTER_IO_BUF)

UBLK_IO_REGISTER_IO_BUF command may be completed, and buffer isn't used by RW_FIXED yet in the following cases:

- application doesn't submit any RW_FIXED consumer OP

- io_uring_enter() only issued UBLK_IO_REGISTER_IO_BUF, and the other OPs can't be issued because of out of resource

...

Then io_uring_enter() returns, and the application is panic or killed, how to avoid buffer leak?

It need to deal with in io_uring cancel code for calling ->release() if the kbuffer node isn't released.

UBLK_IO_UNREGISTER_IO_BUF still need to call ->release() if the node buffer isn't used.

drivers/block/ublk_drv.c static inline void ublk_prep_cancel(struct io_uring_cmd *cmd, · line 1792

Quoted 7 lines

drivers/block/ublk_drv.c +7 -0

+static void ublk_io_release(void *priv)
+{
+	struct request *rq = priv;
+	struct ublk_queue *ubq = rq->mq_hctx->driver_data;
+
+	ublk_put_req_ref(ubq, rq);
+}

It isn't enough to just get & put request reference here between registering buffer and freeing the registered node buf, because the same reference can be dropped from ublk_commit_completion() which is from queueing UBLK_IO_COMMIT_AND_FETCH_REQ, and buggy app may queue this command multiple times for freeing the request.

One solution is to not allow request completion until the ->release() is returned.

Quoted 35 lines · Ming Lei

Quoted 20 lines · Keith Busch

From: Keith Busch <[email protected]>

Provide new operations for the user to request mapping an active request to an io uring instance's buf_table. The user has to provide the index it wants to install the buffer.

A reference count is taken on the request to ensure it can't be completed while it is active in a ring's buf_table.

Signed-off-by Keith Busch <[email protected]>

---
 drivers/block/ublk_drv.c      | 145 +++++++++++++++++++++++++---------
 include/uapi/linux/ublk_cmd.h |   4 +
 2 files changed, 113 insertions(+), 36 deletions(-)

drivers/block/ublk_drv.c +0 -0

index 529085181f355..ccfda7b2c24da 100644

drivers/block/ublk_drv.c line 55

Quoted 5 lines

drivers/block/ublk_drv.c +2 -0

@@ -51,6 +51,9 @@
 /* private ioctl command mirror */
 #define UBLK_CMD_DEL_DEV_ASYNC	_IOC_NR(UBLK_U_CMD_DEL_DEV_ASYNC)
 
+#define UBLK_IO_REGISTER_IO_BUF		_IOC_NR(UBLK_U_IO_REGISTER_IO_BUF)
+#define UBLK_IO_UNREGISTER_IO_BUF	_IOC_NR(UBLK_U_IO_UNREGISTER_IO_BUF)

UBLK_IO_REGISTER_IO_BUF command may be completed, and buffer isn't used by RW_FIXED yet in the following cases:

- application doesn't submit any RW_FIXED consumer OP

- io_uring_enter() only issued UBLK_IO_REGISTER_IO_BUF, and the other OPs can't be issued because of out of resource

Quoted 4 lines

...

Then io_uring_enter() returns, and the application is panic or killed, how to avoid buffer leak?

The death of the uring that registered the node tears down the table that it's registered with, which releases its reference. All good.

Quoted 3 lines

It need to deal with in io_uring cancel code for calling ->release() if the kbuffer node isn't released.

There should be no situation here where it isn't released after its use is completed. Either the resource was gracefully unregistered or the ring close while it was still active, but either one drops its reference.

Quoted 2 lines

UBLK_IO_UNREGISTER_IO_BUF still need to call ->release() if the node buffer isn't used.

Only once the last reference is dropped. Which should happen no matter which way the node is freed.

Quoted 14 lines

drivers/block/ublk_drv.c static inline void ublk_prep_cancel(struct io_uring_cmd *cmd, · line 1792

Quoted 7 lines

drivers/block/ublk_drv.c +7 -0

+static void ublk_io_release(void *priv)
+{
+	struct request *rq = priv;
+	struct ublk_queue *ubq = rq->mq_hctx->driver_data;
+
+	ublk_put_req_ref(ubq, rq);
+}

It isn't enough to just get & put request reference here between registering buffer and freeing the registered node buf, because the same reference can be dropped from ublk_commit_completion() which is from queueing UBLK_IO_COMMIT_AND_FETCH_REQ, and buggy app may queue this command multiple times for freeing the request.

Quoted 2 lines

One solution is to not allow request completion until the ->release() is returned.

Double completions are tricky because the same request id can be reused pretty quickly and there's no immediate way to tell if the 2nd completion is a double or a genuine completion of the reused request.

We have rotating sequence numbers in the nvme driver to try to detect a similar situation. So far it hasn't revealed any real bugs as far as I know. This feels like the other side screwed up and that's their fault.

On Tue, Feb 11, 2025 at 09:11:10PM -0700, Keith Busch wrote:

However, it still may cause use-after-free on this request which has been failed from io_uring_try_cancel_uring_cmd(), and please see the following code path:

io_uring_try_cancel_requests

	io_uring_try_cancel_uring_cmd
		ublk_uring_cmd_cancel_fn
			ublk_abort_requests
				ublk_abort_queue
					__ublk_fail_req
						ublk_put_req_ref

The above race needs to be covered.

From: Keith Busch <[email protected]>

Provide new operations for the user to request mapping an active request to an io uring instance's buf_table. The user has to provide the index it wants to install the buffer.

A reference count is taken on the request to ensure it can't be completed while it is active in a ring's buf_table.

Signed-off-by Keith Busch <[email protected]>

---
  drivers/block/ublk_drv.c      | 145 +++++++++++++++++++++++++---------
  include/uapi/linux/ublk_cmd.h |   4 +
  2 files changed, 113 insertions(+), 36 deletions(-)

drivers/block/ublk_drv.c +0 -0

index 529085181f355..ccfda7b2c24da 100644
@@ -51,6 +51,9 @@

drivers/block/ublk_drv.c +10 -0

+static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
+				  struct ublk_queue *ubq, int tag,
+				  const struct ublksrv_io_cmd *ub_cmd)
+{
+	struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
+	struct ublk_device *ub = cmd->file->private_data;
+	int index = (int)ub_cmd->addr;
+	struct ublk_rq_data *data;
+	struct request *req;
+

From: Keith Busch <[email protected]>

Previous version was discussed here:

https://lore.kernel.org/linux-block/[email protected]/

The same ublksrv reference code in that link was used to test the kernel side changes.

Before listing what has changed, I want to mention what is the same: the reliance on the ring ctx lock to serialize the register ahead of any use. I'm not ignoring the feedback; I just don't have a solid answer right now, and want to progress on the other fronts in the meantime.

It is explained in the following links:

https://lore.kernel.org/linux-block/[email protected]/

- node kbuffer is registered in ublk uring_cmd's ->issue(), but lookup in RW_FIXED OP's ->prep(), and ->prep() is always called before calling ->issue() when the two are submitted in same io_uring_enter(), so you need to move io_rsrc_node_lookup() & buffer importing from RW_FIXED's ->prep() to ->issue() first.

- secondly, ->issue() order is only respected by IO_LINK, and io_uring can't provide such guarantee without using IO_LINK:

Pavel explained it in the following link:

https://lore.kernel.org/linux-block/[email protected]/

There are also other examples, such as, register buffer stays in one link chain, and the consumer OP isn't in this chain, the consumer OP can still be issued before issuing register_buffer.

Quoted 9 lines · Ming Lei

It is explained in the following links:

https://lore.kernel.org/linux-block/[email protected]/

- node kbuffer is registered in ublk uring_cmd's ->issue(), but lookup in RW_FIXED OP's ->prep(), and ->prep() is always called before calling ->issue() when the two are submitted in same io_uring_enter(), so you need to move io_rsrc_node_lookup() & buffer importing from RW_FIXED's ->prep() to ->issue() first.

I don't think that's accurate, at least in practice. In a normal flow, we'll have this sequence:

io_submit_sqes

    io_submit_sqe (uring_cmd ublk register)
      io_init_req
        ->prep()
      io_queue_sqe
        ->issue()
    io_submit_sqe (read/write_fixed)
      io_init_req
        ->prep()
      io_queue_sqe
       ->issue()

The first SQE is handled in its entirety before even looking at the subsequent SQE. Since the register is first, then the read/write_fixed's prep will have a valid index. Testing this patch series appears to show this reliably works.

Quoted 10 lines

- secondly, ->issue() order is only respected by IO_LINK, and io_uring can't provide such guarantee without using IO_LINK:

Pavel explained it in the following link:

https://lore.kernel.org/linux-block/[email protected]/

There are also other examples, such as, register buffer stays in one link chain, and the consumer OP isn't in this chain, the consumer OP can still be issued before issuing register_buffer.

Yep, I got that. Linking is just something I was hoping to avoid. I understand there are conditions that can break the normal flow I'm relying on regarding the ordering. This hasn't appeared to be a problem in practice, but I agree this needs to be handled.

Quoted 26 lines · Keith Busch

Quoted 9 lines · Ming Lei

It is explained in the following links:

https://lore.kernel.org/linux-block/[email protected]/

- node kbuffer is registered in ublk uring_cmd's ->issue(), but lookup in RW_FIXED OP's ->prep(), and ->prep() is always called before calling ->issue() when the two are submitted in same io_uring_enter(), so you need to move io_rsrc_node_lookup() & buffer importing from RW_FIXED's ->prep() to ->issue() first.

I don't think that's accurate, at least in practice. In a normal flow, we'll have this sequence:

io_submit_sqes

    io_submit_sqe (uring_cmd ublk register)
      io_init_req
        ->prep()
      io_queue_sqe
        ->issue()
    io_submit_sqe (read/write_fixed)
      io_init_req
        ->prep()
      io_queue_sqe
       ->issue()

Quoted 4 lines

The first SQE is handled in its entirety before even looking at the subsequent SQE. Since the register is first, then the read/write_fixed's prep will have a valid index. Testing this patch series appears to show this reliably works.

Ming describes how it works for links. This one is indeed how non links are normally executed. Though I'd repeat it's an implementation detail and not a part of the uapi. Interestingly, Keith, you sent some patches changing the ordering here quite a while ago, just as an example of how it can change.

Quoted 9 lines

Quoted 9 lines

- secondly, ->issue() order is only respected by IO_LINK, and io_uring can't provide such guarantee without using IO_LINK:

Pavel explained it in the following link:

   https://lore.kernel.org/linux-block/[email protected]/

Quoted 8 lines

Quoted 3 lines

There are also other examples, such as, register buffer stays in one link chain, and the consumer OP isn't in this chain, the consumer OP can still be issued before issuing register_buffer.

Yep, I got that. Linking is just something I was hoping to avoid. I understand there are conditions that can break the normal flow I'm relying on regarding the ordering. This hasn't appeared to be a problem in practice, but I agree this needs to be handled.

-----Original Message-----
From: Keith Busch <[email protected]>
Sent: Tuesday, February 11, 2025 8:57 AM
To: [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Cc: [email protected]; Keith Busch <[email protected]>
Subject: [PATCHv2 0/6] ublk zero-copy support

From: Keith Busch <[email protected]>

Previous version was discussed here:

https://lore.kernel.org/linux-block/20250203154517.937623-1-
[email protected]/

The same ublksrv reference code in that link was used to test the kernel side changes.

Before listing what has changed, I want to mention what is the same: the reliance on the ring ctx lock to serialize the register ahead of any use. I'm not ignoring the feedback; I just don't have a solid answer right now, and want to progress on the other fronts in the meantime.

Here's what's different from the previous:

- Introduced an optional 'release' callback when the resource node is no longer referenced. The callback addresses any buggy applications that may complete their request and unregister their index while IO is in flight. This obviates any need to take extra page references since it prevents the request from completing.

- Removed peeking into the io_cache element size and instead use a more intuitive bvec segment count limit to decide if we're caching the imu (suggested by Pavel).

- Dropped the const request changes; it's not needed.

I tested this patch set. When I use null as the device, the test results are like your v1.
When the bs is 4k, there is a slight improvement; when the bs is 64k, there is a significant improvement. However, when I used loop as the device, I found that there was no improvement, whether using 4k or 64k. As follow:

ublk add -t loop -f ./ublk-loop.img
ublk add -t loop -f ./ublk-loop-zerocopy.img

fio -filename=/dev/ublkb0 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring -bs=128k -size=5G

    read: IOPS=2015, BW=126MiB/s (132MB/s)(1260MiB/10005msec)

fio -filename=/dev/ublkb1 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring -bs=128k -size=5G

    read: IOPS=1998, BW=125MiB/s (131MB/s)(1250MiB/10005msec)

So, this patch set is optimized for null type devices? Or if I've missed any key information, please let me know.

-----Original Message-----
From: Keith Busch <[email protected]>
Sent: Friday, February 14, 2025 12:07 AM
To: lizetao <[email protected]>
Cc: Keith Busch <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCHv2 0/6] ublk zero-copy support

Quoted 1 line · lizetao

I tested this patch set. When I use null as the device, the test results are like

your v1.

Quoted 1 line

When the bs is 4k, there is a slight improvement; when the bs is 64k, there is

a significant improvement.

Quoted 1 line

However, when I used loop as the device, I found that there was no

improvement, whether using 4k or 64k. As follow:

Quoted 5 lines

ublk add -t loop -f ./ublk-loop.img
ublk add -t loop -f ./ublk-loop-zerocopy.img

fio -filename=/dev/ublkb0 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring

-bs=128k -size=5G

Quoted 3 lines

    read: IOPS=2015, BW=126MiB/s (132MB/s)(1260MiB/10005msec)

fio -filename=/dev/ublkb1 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring

-bs=128k -size=5G

Quoted 4 lines

    read: IOPS=1998, BW=125MiB/s (131MB/s)(1250MiB/10005msec)

So, this patch set is optimized for null type devices? Or if I've missed any key

information, please let me know.

What do you get if if you run your fio job directly on your ublk-loop.img file?

Throughput should improve until you've saturated the backend device. Once you hit that point, the primary benefit of zero-copy come from decreased memory and CPU utilizations.

Hi,

Quoted 31 lines

-----Original Message-----
From: Keith Busch <[email protected]>
Sent: Tuesday, February 11, 2025 8:57 AM
To: [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Cc: [email protected]; Keith Busch <[email protected]>
Subject: [PATCHv2 0/6] ublk zero-copy support

From: Keith Busch <[email protected]>

Previous version was discussed here:

https://lore.kernel.org/linux-block/20250203154517.937623-1-
[email protected]/

The same ublksrv reference code in that link was used to test the kernel side changes.

Before listing what has changed, I want to mention what is the same: the reliance on the ring ctx lock to serialize the register ahead of any use. I'm not ignoring the feedback; I just don't have a solid answer right now, and want to progress on the other fronts in the meantime.

Here's what's different from the previous:

- Introduced an optional 'release' callback when the resource node is no longer referenced. The callback addresses any buggy applications that may complete their request and unregister their index while IO is in flight. This obviates any need to take extra page references since it prevents the request from completing.

Quoted 5 lines

- Removed peeking into the io_cache element size and instead use a more intuitive bvec segment count limit to decide if we're caching the imu (suggested by Pavel).

- Dropped the const request changes; it's not needed.

I tested this patch set. When I use null as the device, the test results are like your v1. When the bs is 4k, there is a slight improvement; when the bs is 64k, there is a significant improvement.

However, when I used loop as the device, I found that there was no improvement, whether using 4k or 64k. As follow:

ublk add -t loop -f ./ublk-loop.img
ublk add -t loop -f ./ublk-loop-zerocopy.img

fio -filename=/dev/ublkb0 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring -bs=128k -size=5G

    read: IOPS=2015, BW=126MiB/s (132MB/s)(1260MiB/10005msec)

fio -filename=/dev/ublkb1 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring -bs=128k -size=5G

    read: IOPS=1998, BW=125MiB/s (131MB/s)(1250MiB/10005msec)

So, this patch set is optimized for null type devices? Or if I've missed any key information, please let me know.

-----Original Message-----
From: Ming Lei <[email protected]>
Sent: Friday, February 14, 2025 10:41 AM
To: lizetao <[email protected]>
Cc: Keith Busch <[email protected]>; [email protected]; [email protected]; Ming Lei <[email protected]>
Subject: Re: [PATCHv2 0/6] ublk zero-copy support

Quoted 25 lines · lizetao

Hi,

Quoted 22 lines

-----Original Message-----
From: Keith Busch <[email protected]>
Sent: Tuesday, February 11, 2025 8:57 AM
To: [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]
Cc: [email protected]; Keith Busch <[email protected]>
Subject: [PATCHv2 0/6] ublk zero-copy support

From: Keith Busch <[email protected]>

Previous version was discussed here:

https://lore.kernel.org/linux-block/20250203154517.937623-1-
[email protected]/

The same ublksrv reference code in that link was used to test the kernel side changes.

Before listing what has changed, I want to mention what is the same:
the reliance on the ring ctx lock to serialize the register ahead of any use. I'm not ignoring the feedback; I just don't have a solid answer right now, and want to progress on the other fronts in the

meantime.

Quoted 9 lines

Quoted 9 lines

Here's what's different from the previous:

- Introduced an optional 'release' callback when the resource node is no longer referenced. The callback addresses any buggy applications that may complete their request and unregister their index while IO is in flight. This obviates any need to take extra page references since it prevents the request from completing.

Quoted 7 lines

Quoted 5 lines

- Removed peeking into the io_cache element size and instead use a more intuitive bvec segment count limit to decide if we're caching the imu (suggested by Pavel).

- Dropped the const request changes; it's not needed.

I tested this patch set. When I use null as the device, the test results are like

your v1.

Quoted 1 line

When the bs is 4k, there is a slight improvement; when the bs is 64k, there is

a significant improvement.

Yes, the improvement is usually more obvious with a big IO size(>= 64K).

Quoted 1 line

However, when I used loop as the device, I found that there was no

improvement, whether using 4k or 64k. As follow:

Quoted 5 lines

ublk add -t loop -f ./ublk-loop.img
ublk add -t loop -f ./ublk-loop-zerocopy.img

fio -filename=/dev/ublkb0 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring

-bs=128k -size=5G

Quoted 3 lines

    read: IOPS=2015, BW=126MiB/s (132MB/s)(1260MiB/10005msec)

fio -filename=/dev/ublkb1 -direct=1 -rw=read -iodepth=1 -ioengine=io_uring

-bs=128k -size=5G

Quoted 4 lines

    read: IOPS=1998, BW=125MiB/s (131MB/s)(1250MiB/10005msec)

So, this patch set is optimized for null type devices? Or if I've missed any key

information, please let me know.

Latency may have decreased a bit.

System sources can't be saturated in single queue depth, please run the same test with high queue depth per Keith's suggestion:

        --iodepth=128 --iodepth_batch_submit=16 --

iodepth_batch_complete_min=16

Also if you set up the backing file as ramfs image, the improvement should be pretty obvious, I observed IOPS doubled in this way.