* Re: [PATCH v2 01/10] ublk: add UBLK_U_CMD_REG_BUF/UNREG_BUF control commands
From: Caleb Sander Mateos @ 2026-04-08 15:20 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block
In-Reply-To: <adW8I8RpSD471iFy@fedora>
On Tue, Apr 7, 2026 at 7:23 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Tue, Apr 07, 2026 at 12:35:49PM -0700, Caleb Sander Mateos wrote:
> > On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > Add control commands for registering and unregistering shared memory
> > > buffers for zero-copy I/O:
> > >
> > > - UBLK_U_CMD_REG_BUF (0x18): pins pages from userspace, inserts PFN
> > > ranges into a per-device maple tree for O(log n) lookup during I/O.
> > > Buffer pointers are tracked in a per-device xarray. Returns the
> > > assigned buffer index.
> > >
> > > - UBLK_U_CMD_UNREG_BUF (0x19): removes PFN entries and unpins pages.
> > >
> > > Queue freeze/unfreeze is handled internally so userspace need not
> > > quiesce the device during registration.
> > >
> > > Also adds:
> > > - UBLK_IO_F_SHMEM_ZC flag and addr encoding helpers in UAPI header
> > > (16-bit buffer index supporting up to 65536 buffers)
> > > - Data structures (ublk_buf, ublk_buf_range) and xarray/maple tree
> > > - __ublk_ctrl_reg_buf() helper for PFN insertion with error unwinding
> > > - __ublk_ctrl_unreg_buf() helper for cleanup reuse
> > > - ublk_support_shmem_zc() / ublk_dev_support_shmem_zc() stubs
> > > (returning false — feature not enabled yet)
> > >
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > > drivers/block/ublk_drv.c | 300 ++++++++++++++++++++++++++++++++++
> > > include/uapi/linux/ublk_cmd.h | 72 ++++++++
> > > 2 files changed, 372 insertions(+)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 71c7c56b38ca..ac6ccc174d44 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -46,6 +46,8 @@
> > > #include <linux/kref.h>
> > > #include <linux/kfifo.h>
> > > #include <linux/blk-integrity.h>
> > > +#include <linux/maple_tree.h>
> > > +#include <linux/xarray.h>
> > > #include <uapi/linux/fs.h>
> > > #include <uapi/linux/ublk_cmd.h>
> > >
> > > @@ -58,6 +60,8 @@
> > > #define UBLK_CMD_UPDATE_SIZE _IOC_NR(UBLK_U_CMD_UPDATE_SIZE)
> > > #define UBLK_CMD_QUIESCE_DEV _IOC_NR(UBLK_U_CMD_QUIESCE_DEV)
> > > #define UBLK_CMD_TRY_STOP_DEV _IOC_NR(UBLK_U_CMD_TRY_STOP_DEV)
> > > +#define UBLK_CMD_REG_BUF _IOC_NR(UBLK_U_CMD_REG_BUF)
> > > +#define UBLK_CMD_UNREG_BUF _IOC_NR(UBLK_U_CMD_UNREG_BUF)
> > >
> > > #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)
> > > @@ -289,6 +293,20 @@ struct ublk_queue {
> > > struct ublk_io ios[] __counted_by(q_depth);
> > > };
> > >
> > > +/* Per-registered shared memory buffer */
> > > +struct ublk_buf {
> > > + struct page **pages;
> > > + unsigned int nr_pages;
> > > +};
> > > +
> > > +/* Maple tree value: maps a PFN range to buffer location */
> > > +struct ublk_buf_range {
> > > + unsigned long base_pfn;
> > > + unsigned short buf_index;
> > > + unsigned short flags;
> > > + unsigned int base_offset; /* byte offset within buffer */
> > > +};
> > > +
> > > struct ublk_device {
> > > struct gendisk *ub_disk;
> > >
> > > @@ -323,6 +341,10 @@ struct ublk_device {
> > >
> > > bool block_open; /* protected by open_mutex */
> > >
> > > + /* shared memory zero copy */
> > > + struct maple_tree buf_tree;
> > > + struct xarray bufs_xa;
> > > +
> > > struct ublk_queue *queues[];
> > > };
> > >
> > > @@ -334,6 +356,7 @@ struct ublk_params_header {
> > >
> > > static void ublk_io_release(void *priv);
> > > static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> > > +static void ublk_buf_cleanup(struct ublk_device *ub);
> > > static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> > > static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > > u16 q_id, u16 tag, struct ublk_io *io);
> > > @@ -398,6 +421,16 @@ static inline bool ublk_dev_support_zero_copy(const struct ublk_device *ub)
> > > return ub->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY;
> > > }
> > >
> > > +static inline bool ublk_support_shmem_zc(const struct ublk_queue *ubq)
> > > +{
> > > + return false;
> > > +}
> > > +
> > > +static inline bool ublk_dev_support_shmem_zc(const struct ublk_device *ub)
> > > +{
> > > + return false;
> > > +}
> > > +
> > > static inline bool ublk_support_auto_buf_reg(const struct ublk_queue *ubq)
> > > {
> > > return ubq->flags & UBLK_F_AUTO_BUF_REG;
> > > @@ -1460,6 +1493,7 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> > > iod->op_flags = ublk_op | ublk_req_build_flags(req);
> > > iod->nr_sectors = blk_rq_sectors(req);
> > > iod->start_sector = blk_rq_pos(req);
> > > +
> >
> > nit: unrelated whitespace change?
> >
> > > iod->addr = io->buf.addr;
> > >
> > > return BLK_STS_OK;
> > > @@ -1665,6 +1699,7 @@ static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> > > {
> > > unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > >
> > > +
> >
> > nit: unrelated whitespace change?
> >
> > > /* partially mapped, update io descriptor */
> > > if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > > /*
> > > @@ -4206,6 +4241,7 @@ static void ublk_cdev_rel(struct device *dev)
> > > {
> > > struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
> > >
> > > + ublk_buf_cleanup(ub);
> > > blk_mq_free_tag_set(&ub->tag_set);
> > > ublk_deinit_queues(ub);
> > > ublk_free_dev_number(ub);
> > > @@ -4625,6 +4661,8 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
> > > mutex_init(&ub->mutex);
> > > spin_lock_init(&ub->lock);
> > > mutex_init(&ub->cancel_mutex);
> > > + mt_init(&ub->buf_tree);
> > > + xa_init_flags(&ub->bufs_xa, XA_FLAGS_ALLOC);
> > > INIT_WORK(&ub->partition_scan_work, ublk_partition_scan_work);
> > >
> > > ret = ublk_alloc_dev_number(ub, header->dev_id);
> > > @@ -5168,6 +5206,260 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
> > > return err;
> > > }
> > >
> > > +/*
> > > + * Drain inflight I/O and quiesce the queue. Freeze drains all inflight
> > > + * requests, quiesce_nowait marks the queue so no new requests dispatch,
> > > + * then unfreeze allows new submissions (which won't dispatch due to
> > > + * quiesce). This keeps freeze and ub->mutex non-nested.
> > > + */
> > > +static void ublk_quiesce_and_release(struct gendisk *disk)
> > > +{
> > > + unsigned int memflags;
> > > +
> > > + memflags = blk_mq_freeze_queue(disk->queue);
> > > + blk_mq_quiesce_queue_nowait(disk->queue);
> > > + blk_mq_unfreeze_queue(disk->queue, memflags);
> > > +}
> > > +
> > > +static void ublk_unquiesce_and_resume(struct gendisk *disk)
> > > +{
> > > + blk_mq_unquiesce_queue(disk->queue);
> > > +}
> > > +
> > > +/*
> > > + * Insert PFN ranges of a registered buffer into the maple tree,
> > > + * coalescing consecutive PFNs into single range entries.
> > > + * Returns 0 on success, negative error with partial insertions unwound.
> > > + */
> > > +/* Erase coalesced PFN ranges from the maple tree for pages [0, nr_pages) */
> > > +static void ublk_buf_erase_ranges(struct ublk_device *ub,
> > > + struct ublk_buf *ubuf,
> > > + unsigned long nr_pages)
> > > +{
> > > + unsigned long i;
> > > +
> > > + for (i = 0; i < nr_pages; ) {
> > > + unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > > + unsigned long start = i;
> > > +
> > > + while (i + 1 < nr_pages &&
> > > + page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > > + i++;
> > > + i++;
> > > + kfree(mtree_erase(&ub->buf_tree, pfn));
> > > + }
> > > +}
> > > +
> > > +static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> > > + struct ublk_buf *ubuf, int index,
> > > + unsigned short flags)
> > > +{
> > > + unsigned long nr_pages = ubuf->nr_pages;
> > > + unsigned long i;
> > > + int ret;
> > > +
> > > + for (i = 0; i < nr_pages; ) {
> > > + unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > > + unsigned long start = i;
> > > + struct ublk_buf_range *range;
> > > +
> > > + /* Find run of consecutive PFNs */
> > > + while (i + 1 < nr_pages &&
> > > + page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > > + i++;
> > > + i++; /* past the last page in this run */
> >
> > Move this increment to the for loop so you don't need the "- 1" in the
> > mtree_insert_range() call?
>
> Good catch!
>
> >
> > > +
> > > + range = kzalloc(sizeof(*range), GFP_KERNEL);
> >
> > Not sure kzalloc() is necessary; all the fields are initialized below
>
> Yeah, kmalloc() is fine, and we shouldn't add more fields to `range` in
> future.
>
> >
> > > + if (!range) {
> > > + ret = -ENOMEM;
> > > + goto unwind;
> > > + }
> > > + range->buf_index = index;
> > > + range->flags = flags;
> > > + range->base_pfn = pfn;
> > > + range->base_offset = start << PAGE_SHIFT;
> > > +
> > > + ret = mtree_insert_range(&ub->buf_tree, pfn,
> > > + pfn + (i - start) - 1,
> > > + range, GFP_KERNEL);
> > > + if (ret) {
> > > + kfree(range);
> > > + goto unwind;
> > > + }
> > > + }
> > > + return 0;
> > > +
> > > +unwind:
> > > + ublk_buf_erase_ranges(ub, ubuf, i);
> > > + return ret;
> > > +}
> > > +
> > > +/*
> > > + * Register a shared memory buffer for zero-copy I/O.
> > > + * Pins pages, builds PFN maple tree, freezes/unfreezes the queue
> > > + * internally. Returns buffer index (>= 0) on success.
> > > + */
> > > +static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > > + struct ublksrv_ctrl_cmd *header)
> > > +{
> > > + void __user *argp = (void __user *)(unsigned long)header->addr;
> > > + struct ublk_shmem_buf_reg buf_reg;
> > > + unsigned long addr, size, nr_pages;
> >
> > size and nr_pages could be u32
>
> Yeah, it was caused by internal change on `ublk_shmem_buf_reg`.
>
> >
> > > + unsigned int gup_flags;
> > > + struct gendisk *disk;
> > > + struct ublk_buf *ubuf;
> > > + long pinned;
> >
> > pinned could be int to match the return type of pin_user_pages_fast()
>
> OK.
>
> >
> > > + u32 index;
> > > + int ret;
> > > +
> > > + if (!ublk_dev_support_shmem_zc(ub))
> > > + return -EOPNOTSUPP;
> > > +
> > > + memset(&buf_reg, 0, sizeof(buf_reg));
> > > + if (copy_from_user(&buf_reg, argp,
> > > + min_t(size_t, header->len, sizeof(buf_reg))))
> > > + return -EFAULT;
> > > +
> > > + if (buf_reg.flags & ~UBLK_SHMEM_BUF_READ_ONLY)
> > > + return -EINVAL;
> > > +
> > > + addr = buf_reg.addr;
> > > + size = buf_reg.len;
> >
> > nit: don't see much value in these additional variables that are just
> > copies of buf_reg fields
> >
> > > + nr_pages = size >> PAGE_SHIFT;
> > > +
> > > + if (!size || !PAGE_ALIGNED(size) || !PAGE_ALIGNED(addr))
> > > + return -EINVAL;
> > > +
> > > + disk = ublk_get_disk(ub);
> > > + if (!disk)
> > > + return -ENODEV;
> >
> > So buffers can't be registered before the ublk device is started? Is
> > there a reason why that's not possible? Could we just make the
> > ublk_quiesce_and_release() and ublk_unquiesce_and_resume() conditional
> > on disk being non-NULL? I guess we'd have to hold the ublk_device
> > mutex before calling ublk_get_disk() to prevent it from being assigned
> > concurrently.
>
> Here `disk` is used for freeze & quiesce queue.
>
> But the implementation can be a bit more complicated given the dependency
> between ub->mutex and freeze queue should be avoided.
>
> Anyway, it is one nice requirement, I will try to relax the constraint.
>
> >
> > > +
> > > + /* Pin pages before quiescing (may sleep) */
> > > + ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
> > > + if (!ubuf) {
> > > + ret = -ENOMEM;
> > > + goto put_disk;
> > > + }
> > > +
> > > + ubuf->pages = kvmalloc_array(nr_pages, sizeof(*ubuf->pages),
> > > + GFP_KERNEL);
> > > + if (!ubuf->pages) {
> > > + ret = -ENOMEM;
> > > + goto err_free;
> > > + }
> > > +
> > > + gup_flags = FOLL_LONGTERM;
> > > + if (!(buf_reg.flags & UBLK_SHMEM_BUF_READ_ONLY))
> > > + gup_flags |= FOLL_WRITE;
> > > +
> > > + pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, ubuf->pages);
> > > + if (pinned < 0) {
> > > + ret = pinned;
> > > + goto err_free_pages;
> > > + }
> > > + if (pinned != nr_pages) {
> > > + ret = -EFAULT;
> > > + goto err_unpin;
> > > + }
> > > + ubuf->nr_pages = nr_pages;
> > > +
> > > + /*
> > > + * Drain inflight I/O and quiesce the queue so no new requests
> > > + * are dispatched while we modify the maple tree. Keep freeze
> > > + * and mutex non-nested to avoid lock dependency.
> > > + */
> > > + ublk_quiesce_and_release(disk);
> > > +
> > > + mutex_lock(&ub->mutex);
> >
> > Looks like the xarray and maple tree do their own spinlocking, is this needed?
>
> Right, looks it isn't needed now, and it was added from beginning with plain
> array.
>
> >
> > > +
> > > + ret = xa_alloc(&ub->bufs_xa, &index, ubuf, xa_limit_16b, GFP_KERNEL);
> > > + if (ret)
> > > + goto err_unlock;
> > > +
> > > + ret = __ublk_ctrl_reg_buf(ub, ubuf, index, buf_reg.flags);
> > > + if (ret) {
> > > + xa_erase(&ub->bufs_xa, index);
> > > + goto err_unlock;
> > > + }
> > > +
> > > + mutex_unlock(&ub->mutex);
> > > +
> > > + ublk_unquiesce_and_resume(disk);
> > > + ublk_put_disk(disk);
> > > + return index;
> > > +
> > > +err_unlock:
> > > + mutex_unlock(&ub->mutex);
> > > + ublk_unquiesce_and_resume(disk);
> > > +err_unpin:
> > > + unpin_user_pages(ubuf->pages, pinned);
> > > +err_free_pages:
> > > + kvfree(ubuf->pages);
> > > +err_free:
> > > + kfree(ubuf);
> > > +put_disk:
> > > + ublk_put_disk(disk);
> > > + return ret;
> > > +}
> > > +
> > > +static void __ublk_ctrl_unreg_buf(struct ublk_device *ub,
> > > + struct ublk_buf *ubuf)
> > > +{
> > > + ublk_buf_erase_ranges(ub, ubuf, ubuf->nr_pages);
> > > + unpin_user_pages(ubuf->pages, ubuf->nr_pages);
> > > + kvfree(ubuf->pages);
> > > + kfree(ubuf);
> > > +}
> > > +
> > > +static int ublk_ctrl_unreg_buf(struct ublk_device *ub,
> > > + struct ublksrv_ctrl_cmd *header)
> > > +{
> > > + int index = (int)header->data[0];
> > > + struct gendisk *disk;
> > > + struct ublk_buf *ubuf;
> > > +
> > > + if (!ublk_dev_support_shmem_zc(ub))
> > > + return -EOPNOTSUPP;
> > > +
> > > + disk = ublk_get_disk(ub);
> > > + if (!disk)
> > > + return -ENODEV;
> > > +
> > > + /* Drain inflight I/O before modifying the maple tree */
> > > + ublk_quiesce_and_release(disk);
> > > +
> > > + mutex_lock(&ub->mutex);
> > > +
> > > + ubuf = xa_erase(&ub->bufs_xa, index);
> > > + if (!ubuf) {
> > > + mutex_unlock(&ub->mutex);
> > > + ublk_unquiesce_and_resume(disk);
> > > + ublk_put_disk(disk);
> > > + return -ENOENT;
> > > + }
> > > +
> > > + __ublk_ctrl_unreg_buf(ub, ubuf);
> > > +
> > > + mutex_unlock(&ub->mutex);
> > > +
> > > + ublk_unquiesce_and_resume(disk);
> > > + ublk_put_disk(disk);
> > > + return 0;
> > > +}
> > > +
> > > +static void ublk_buf_cleanup(struct ublk_device *ub)
> > > +{
> > > + struct ublk_buf *ubuf;
> > > + unsigned long index;
> > > +
> > > + xa_for_each(&ub->bufs_xa, index, ubuf)
> > > + __ublk_ctrl_unreg_buf(ub, ubuf);
> >
> > This looks quadratic in the number of registered buffers. Can we do a
> > single pass over the xarray and the maple tree?
>
> It can be done two passes: first pass walks maple tree for unpinning
> pages, the 2nd pass is for freeing buffers in xarray, which looks more
> clean.
>
> But the current way can reuse __ublk_ctrl_unreg_buf(), also
> ublk_buf_cleanup() is only called one-shot in device release handler, so we can
> leave it for future optimization.
>
> >
> > > + xa_destroy(&ub->bufs_xa);
> > > + mtree_destroy(&ub->buf_tree);
> > > +}
> > > +
> > > +
> > > +
> > > static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> > > u32 cmd_op, struct ublksrv_ctrl_cmd *header)
> > > {
> > > @@ -5225,6 +5517,8 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> > > case UBLK_CMD_UPDATE_SIZE:
> > > case UBLK_CMD_QUIESCE_DEV:
> > > case UBLK_CMD_TRY_STOP_DEV:
> > > + case UBLK_CMD_REG_BUF:
> > > + case UBLK_CMD_UNREG_BUF:
> > > mask = MAY_READ | MAY_WRITE;
> > > break;
> > > default:
> > > @@ -5350,6 +5644,12 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
> > > case UBLK_CMD_TRY_STOP_DEV:
> > > ret = ublk_ctrl_try_stop_dev(ub);
> > > break;
> > > + case UBLK_CMD_REG_BUF:
> > > + ret = ublk_ctrl_reg_buf(ub, &header);
> > > + break;
> > > + case UBLK_CMD_UNREG_BUF:
> > > + ret = ublk_ctrl_unreg_buf(ub, &header);
> > > + break;
> > > default:
> > > ret = -EOPNOTSUPP;
> > > break;
> > > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > > index a88876756805..52bb9b843d73 100644
> > > --- a/include/uapi/linux/ublk_cmd.h
> > > +++ b/include/uapi/linux/ublk_cmd.h
> > > @@ -57,6 +57,44 @@
> > > _IOWR('u', 0x16, struct ublksrv_ctrl_cmd)
> > > #define UBLK_U_CMD_TRY_STOP_DEV \
> > > _IOWR('u', 0x17, struct ublksrv_ctrl_cmd)
> > > +/*
> > > + * Register a shared memory buffer for zero-copy I/O.
> > > + * Input: ctrl_cmd.addr points to struct ublk_buf_reg (buffer VA + size)
> > > + * ctrl_cmd.len = sizeof(struct ublk_buf_reg)
> > > + * Result: >= 0 is the assigned buffer index, < 0 is error
> > > + *
> > > + * The kernel pins pages from the calling process's address space
> > > + * and inserts PFN ranges into a per-device maple tree. When a block
> > > + * request's pages match registered pages, the driver sets
> > > + * UBLK_IO_F_SHMEM_ZC and encodes the buffer index + offset in addr,
> > > + * allowing the server to access the data via its own mapping of the
> > > + * same shared memory — true zero copy.
> > > + *
> > > + * The memory can be backed by memfd, hugetlbfs, or any GUP-compatible
> > > + * shared mapping. Queue freeze is handled internally.
> > > + *
> > > + * The buffer VA and size are passed via a user buffer (not inline in
> > > + * ctrl_cmd) so that unprivileged devices can prepend the device path
> > > + * to ctrl_cmd.addr without corrupting the VA.
> > > + */
> > > +#define UBLK_U_CMD_REG_BUF \
> > > + _IOWR('u', 0x18, struct ublksrv_ctrl_cmd)
> > > +/*
> > > + * Unregister a shared memory buffer.
> > > + * Input: ctrl_cmd.data[0] = buffer index
> > > + */
> > > +#define UBLK_U_CMD_UNREG_BUF \
> > > + _IOWR('u', 0x19, struct ublksrv_ctrl_cmd)
> > > +
> > > +/* Parameter buffer for UBLK_U_CMD_REG_BUF, pointed to by ctrl_cmd.addr */
> > > +struct ublk_shmem_buf_reg {
> > > + __u64 addr; /* userspace virtual address of shared memory */
> > > + __u32 len; /* buffer size in bytes (page-aligned, max 4GB) */
> > > + __u32 flags;
> > > +};
> > > +
> > > +/* Pin pages without FOLL_WRITE; usable with write-sealed memfd */
> > > +#define UBLK_SHMEM_BUF_READ_ONLY (1U << 0)
> > > /*
> > > * 64bits are enough now, and it should be easy to extend in case of
> > > * running out of feature flags
> > > @@ -370,6 +408,7 @@
> > > /* Disable automatic partition scanning when device is started */
> > > #define UBLK_F_NO_AUTO_PART_SCAN (1ULL << 18)
> > >
> > > +
> > > /* device state */
> > > #define UBLK_S_DEV_DEAD 0
> > > #define UBLK_S_DEV_LIVE 1
> > > @@ -469,6 +508,12 @@ struct ublksrv_ctrl_dev_info {
> > > #define UBLK_IO_F_NEED_REG_BUF (1U << 17)
> > > /* Request has an integrity data buffer */
> > > #define UBLK_IO_F_INTEGRITY (1UL << 18)
> > > +/*
> > > + * I/O buffer is in a registered shared memory buffer. When set, the addr
> > > + * field in ublksrv_io_desc encodes buffer index and byte offset instead
> > > + * of a userspace virtual address.
> > > + */
> > > +#define UBLK_IO_F_SHMEM_ZC (1U << 19)
> > >
> > > /*
> > > * io cmd is described by this structure, and stored in share memory, indexed
> > > @@ -743,4 +788,31 @@ struct ublk_params {
> > > struct ublk_param_integrity integrity;
> > > };
> > >
> > > +/*
> > > + * Shared memory zero-copy addr encoding for UBLK_IO_F_SHMEM_ZC.
> > > + *
> > > + * When UBLK_IO_F_SHMEM_ZC is set, ublksrv_io_desc.addr is encoded as:
> > > + * bits [0:31] = byte offset within the buffer (up to 4GB)
> > > + * bits [32:47] = buffer index (up to 65536)
> > > + * bits [48:63] = reserved (must be zero)
> >
> > I wonder whether the "buffer index" is necessary. Can iod->addr and
> > UBLK_U_CMD_UNREG_BUF refer to the buffer by the virtual address used
> > with UBLK_U_CMD_REG_BUF? Then struct ublksrv_io_desc's addr field
> > would retain its meaning. We would also avoid needing to compare the
> > range buf_index values in ublk_try_buf_match(). And the xarray
> > wouldn't be necessary to allocate buffer indices.
>
> There are several reasons for choosing "buffer index":
>
> - avoid to add extra storage in `struct ublk_buf_range`, extra
> `vaddr` field is required for returning virtual address; otherwise one extra
> lookup from buffer index is needed in fast path of ublk_try_buf_match()
Yes, struct ublk_buf_range would have to track the virtual address,
but that would replace both buf_index and base_offset. And you could
store flags in the lower bits of the virtual address (since it has to
be page-aligned) if you want to keep it as 16 bytes. I'm not sure what
you mean about "one extra lookup from buffer index"; I was thinking it
would be possible to get rid of buffer indices entirely.
>
> - I want ublk server to know the difference between shmem_zc buffer and the
> plain IO buffer clearly, both two shouldn't be mixed, otherwise it is easy to
> cause data corruption. For example, client is using buf A, but the
> ublk server fallback code path may be using it at the same time.
Yes, certainly the ublk server should only use a shared-memory buffer
when an incoming UBLK_IO_F_SHMEM_ZC request specifies it. I don't see
the connection to referring to buffers by virtual address instead of
buffer index, though. The kernel can't prevent the ublk server from
writing to a registered shared memory region it has mapped into its
address space.
It seems like a simpler interface if iod->addr indicates the virtual
address of the data buffer regardless of the UBLK_IO_F_SHMEM_ZC flag.
Then the ublk server doesn't have to care whether the data is in
shared memory or was copied automatically by the
ublk_map_io()/ublk_unmap_io() fallback path; it just accesses the
memory at iod->addr and lets the kernel take care of the copying (if
necessary).
>
> - total registered buffers can be limited naturally by `u16` buffer_index
I don't really see a reason to limit the number of SHMEM_ZC buffers if
they don't consume any per-buffer resources. I think it would make
more sense to limit it based on the _total size_ of registered
buffers, but the page pinning should already provide that.
>
> - it is proved that buffer index is one nice pattern wrt. buffer
> registration, such as io_uring fixed buffer; and it helps userspace
> to manage multiple buffers, given each one has unique ID.
If you really prefer the (buf_index, buf_offset) interface, I won't
stand in the way. It just seems to me like the only thing the ublk
server cares about a shared memory buffer is its virtual address, so
that's a more natural way to refer to it.
Best,
Caleb
^ permalink raw reply
* Re: [PATCH 00/20] DRBD 9 rework
From: Jens Axboe @ 2026-04-08 12:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Christoph Böhmwalder, drbd-dev, linux-kernel, Lars Ellenberg,
Philipp Reisner, linux-block
In-Reply-To: <adXk_m3SbRcWTOIC@infradead.org>
On 4/7/26 11:17 PM, Christoph Hellwig wrote:
> On Thu, Apr 02, 2026 at 07:30:35PM -0600, Jens Axboe wrote:
>> On 3/27/26 4:38 PM, Christoph Böhmwalder wrote:
>>> As discussed (context: [0]), here is the first version of our DRBD 9
>>> rework series, intended for for-next via for-7.1/drbd.
>>
>> Will you fixup the kerneldoc (nits) and the assigned-but-not-read
>> issues and send out a new version? Also looks this series doesn't
>> actually apply to for-7.1/block, patch 12 fails.
>
> Looks like all this went in without any review? Since when have we
> started these grand replacements anyway, as they are known to cause
> bugs and regressions?
See the previous discussion, the goal is to sync the two drbd code
bases. It's followed the "usual" pattern of the in-kernel driver being
neglected and development and users pushed to the out-of-tree one,
which is highly annoying.
As per the previous thread, the intent is not to push to to mainline
right now, but rather give it -next exposure without running into
conflicts between the trees. for-7.x/drbd will be rebased as needed,
and it's only in for-next for the stated reason.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] md-7.1-20260407
From: Jens Axboe @ 2026-04-08 12:55 UTC (permalink / raw)
To: Yu Kuai, linux-block, linux-raid
Cc: linux-kernel, song, linan122, abd.masalkhi, chiamingc, gourry,
moonafterrain, xni
In-Reply-To: <20260408050517.2109269-1-yukuai@fnnas.com>
On 4/7/26 11:05 PM, Yu Kuai wrote:
> Hi Jens,
>
> Please consider pulling the following changes into your for-7.1/block branch.
>
> This pull request contains:
>
> Bug Fixes:
> - avoid a sysfs deadlock when clearing array state (Yu Kuai)
> - validate raid5 journal payloads before reading metadata (Junrui Luo)
> - fall back to the correct bitmap operations after version mismatches (Yu Kuai)
> - serialize overlapping writes on writemostly raid1 disks (Xiao Ni)
> - wake raid456 reshape waiters before suspend (Yu Kuai)
> - prevent retry_aligned_read() from triggering soft lockups (Chia-Ming Chang)
>
> Improvements:
> - switch raid0 strip zone and devlist allocations to kvmalloc helpers (Gregory Price)
> - track clean unwritten stripes for proactive RAID5 parity building (Yu Kuai)
> - speed up initial llbitmap sync with write_zeroes_unmap support (Yu Kuai)
>
> Cleanups:
> - remove the unused static md workqueue definition (Abd-Alrhman Masalkhi)
Pulled, thanks.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH v2 00/10] ublk: add shared memory zero-copy support
From: Jens Axboe @ 2026-04-08 12:52 UTC (permalink / raw)
To: Ming Lei, Caleb Sander Mateos; +Cc: linux-block
In-Reply-To: <adXFm1dlDzcikarF@fedora>
On 4/7/26 9:03 PM, Ming Lei wrote:
> On Tue, Apr 07, 2026 at 12:29:39PM -0700, Caleb Sander Mateos wrote:
>> On Mon, Apr 6, 2026 at 7:39 PM Ming Lei <ming.lei@redhat.com> wrote:
>>>
>>> On Tue, Mar 31, 2026 at 11:31:51PM +0800, Ming Lei wrote:
>>>> Hello,
>>>>
>>>> Add shared memory based zero-copy (UBLK_F_SHMEM_ZC) support for ublk.
>>>>
>>>> The ublk server and its client share a memory region (e.g. memfd or
>>>> hugetlbfs file) via MAP_SHARED mmap. The server registers this region
>>>> with the kernel via UBLK_U_CMD_REG_BUF, which pins the pages and
>>>> builds a PFN maple tree. When I/O arrives, the driver looks up bio
>>>> pages in the maple tree — if they match registered buffer pages, the
>>>> data is used directly without copying.
>>>>
>>>> Please see details on document added in patch 3.
>>>>
>>>> Patches 1-4 implement the kernel side:
>>>> - buffer register/unregister control commands with PFN coalescing,
>>>> including read-only buffer support (UBLK_SHMEM_BUF_READ_ONLY)
>>>> - PFN-based matching in the I/O path, with enforcement that read-only
>>>> buffers reject non-WRITE requests
>>>> - UBLK_F_SHMEM_ZC feature flag
>>>> - eliminate permanent pages[] array from struct ublk_buf; the maple
>>>> tree already stores PFN ranges, so pages[] becomes temporary
>>>>
>>>> Patches 5-10 add kublk (selftest server) support and tests:
>>>> - hugetlbfs buffer sharing (both kublk and fio mmap the same file)
>>>> - null target and loop target tests with fio verify
>>>> - filesystem-level test (ext4 on ublk, fio verify on a file)
>>>> - read-only buffer registration test (--rdonly_shmem_buf)
>>>>
>>>> Changes since V1:
>>>> - rename struct ublk_buf_reg to struct ublk_shmem_buf_reg, add __u32
>>>> flags field for extensibility, narrow __u64 len to __u32 (max 4GB
>>>> per UBLK_SHMEM_ZC_OFF_MASK), remove __u32 reserved (patch 1)
>>>> - add UBLK_SHMEM_BUF_READ_ONLY flag: pin pages without FOLL_WRITE,
>>>> enabling registration of write-sealed memfd buffers (patch 1)
>>>> - use backward-compatible struct reading: memset zero + copy
>>>> min(header->len, sizeof(struct)) (patch 1)
>>>> - reorder struct ublk_buf_range fields for better packing (16 bytes
>>>> vs 24 bytes), change buf_index to unsigned short, add unsigned short
>>>> flags to store per-range read-only state (patch 1)
>>>> - enforce read-only buffer semantics in ublk_try_buf_match(): reject
>>>> non-WRITE requests on read-only buffers since READ I/O needs to
>>>> write data into the buffer (patch 2)
>>>> - narrow struct ublk_buf::nr_pages to unsigned int, narrow struct
>>>> ublk_buf_range::base_offset to unsigned int (patch 1)
>>>> - add new patch 4: eliminate permanent pages[] array from struct
>>>> ublk_buf — recover struct page pointers via pfn_to_page() from the
>>>> maple tree during unregistration, saving 2MB per 1GB buffer
>>>> - add UBLK_F_SHMEM_ZC to feat_map in kublk (patch 5)
>>>> - add new patch 10: read-only buffer registration selftest with
>>>> --rdonly_shmem_buf option on null target + hugetlbfs
>>>
>>> Hello,
>>>
>>> Ping...
>>
>> Sorry I didn't get a chance to look at this earlier. It looks really
>> nice, thank you for implementing it! I have just a few comments. One
>> is about the UAPI (can we just use virtual addresses instead of buffer
>> index + offset), so I might wait on landing this patchset until we've
>> finalized that.
>
> Hi Jens,
>
> Can you drop V2 from for-7.1/block so that we can polish up it in V3?
>
> Or I am fine to cook up a followup for misc clean & fix?
Please just do a fixup series. If I drop it now, it's not going into
7.1, and the issues aren't that big of a deal to fix up.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 06/20] drbd: add RDMA transport implementation
From: Christoph Böhmwalder @ 2026-04-08 12:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, drbd-dev, linux-kernel, Lars Ellenberg,
Philipp Reisner, linux-block, Joel Colledge, linux-rdma,
Jason Gunthorpe, Leon Romanovsky
In-Reply-To: <adXq36pbGLXMZc2r@infradead.org>
On Tue, Apr 07, 2026 at 10:42:55PM -0700, Christoph Hellwig wrote:
>You really need to add the RDMA mailing list before adding new RDMA
>code. I'll try to review the bits I still remember, but you also
>need a maintainer ACK.
Thanks for the hint and your detailed feedback. I'll address all that
in v2 (plus some other similar fixes).
Thanks,
Christoph
^ permalink raw reply
* Re: [PATCH 05/79] block: rust: change `queue_rq` request type to `Owned`
From: Andreas Hindborg @ 2026-04-08 11:54 UTC (permalink / raw)
To: Alice Ryhl
Cc: Boqun Feng, Jens Axboe, Miguel Ojeda, Gary Guo,
Björn Roy Baron, Benno Lossin, Trevor Gross,
Danilo Krummrich, FUJITA Tomonori, Frederic Weisbecker,
Lyude Paul, Thomas Gleixner, Anna-Maria Behnsen, John Stultz,
Stephen Boyd, Lorenzo Stoakes, Liam R. Howlett, linux-block,
rust-for-linux, linux-kernel, linux-mm
In-Reply-To: <CAH5fLggq-1pReNsRtgRfm0Y_PiQvUsXbMMkPkyO9kuhqzB3wrA@mail.gmail.com>
Alice Ryhl <aliceryhl@google.com> writes:
> On Mon, Mar 23, 2026 at 1:08 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Alice Ryhl <aliceryhl@google.com> writes:
>>
>> > On Mon, Feb 16, 2026 at 12:34:52AM +0100, Andreas Hindborg wrote:
>> >> Simplify the reference counting scheme for `Request` from 4 states to 3
>> >> states. This is achieved by coalescing the zero state between block layer
>> >> owned and uniquely owned by driver.
>> >>
>> >> Implement `Ownable` for `Request` and deliver `Request` to drivers as
>> >> `Owned<Request>`. In this process:
>> >>
>> >> - Move uniqueness assertions out of `rnull` as these are now guaranteed by
>> >> the `Owned` type.
>> >> - Move `start_unchecked`, `try_set_end` and `end_ok` from `Request` to
>> >> `Owned<Request>`, relying on type invariant for uniqueness.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >
>> > It would be a lot cleaner if we could implement HrTimerPointer for
>> > Owned<Request> and entirely get rid of the refcount in request so we
>> > don't need ARef<Request> at all.
>> >
>> > Is there a reason we *need* ARef here?
>>
>> There is. Real drivers will need to dma map the data buffers in
>> `Request` to a device. This requires taking a reference on the pages to
>> be mapped, which in turn requires taking a reference on the `Request`.
>>
>> We could split up the reference counts into multiple fields, but that
>> would be less efficient.
>
> So how exactly is the refcount used here? Can you elaborate?
I can try to be more clear.
`Request` objects are created when a driver initializes a device. A
driver initializes a number `Request` equal to the queue depth the
driver supports. That is, if a driver/device supports 16 in-flight
requests, the driver allocates and initializes 16 `Request` objects up
front.
When the kernel wants to issue IO to a block device, it finds an idle
`Request` object and sets it up for the IO operation. Then:
1. The block layer hands off the request to the driver. Ownership of the
request is transferred to the driver. At this point, the driver has a
unique reference to the request (`Owned<Request>`).
2. The driver DMA maps the pages of the request. We have to make sure
the request is not handed back to the block layer while the pages are
mapped. To this end, we take a refcount on the request. The reference
that the driver holds is no longer unique (`ARef<Request>`).
3. The driver instructs a device to carry out the request. The driver
releases its refcount on the request and the device takes a refcount.
4. When the device finishes processing the request, ownership is
transferred back to the driver. The device releases it's refcount and
the driver takes a refcount.
5. DMA mappings are torn down. The refcount associated with the DMA
mappings is released.
6. The driver transfers ownership of the request back to the block
layer. To do this, the request must be uniquely owned by the driver.
When the device is done processing a request and we have to transfer
ownership of the request back to the driver, we use an API function
called `tag_to_rq`. This function takes an integer tag and may return a
request reference. For this function to be safe, we have to be able to
assert that the integer tag passed to the function is naming a request
object that it is valid to obtain a reference to. It is not sound to
create references to requests that are not currently in flight. Thus, we
must be able to know this information. The current implementation relies
on the refcount to discover this information:
/// There are three states for a request that the Rust bindings care about:
///
/// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
/// - 1: The request is owned by Rust abstractions but is not referenced.
/// - 2+: There is one or more [`ARef`] instances referencing the request.
So, we are using 1 refcount field to encode all the information we need.
Further, in the current implementation, for step 3, the device does not
actually take a refcount on the request. If a driver drops all
references to a request, the refcount lands on 1. We use this to
indicate that the request has been leaked and to know that `tag_to_rq`
is safe. In a situation where the request is DMA mapped, the refcount
would be 2 while the device is processing the request.
Here is a sequence diagram of the flow:
Block Layer Driver Device
─────────── ────── ──────
| | |
| | |
(1) | hand off request | |
|-------------------->| |
| | Owned<Request> |
| | refcount = 0 |
| | |
| | |
(2) | DMA map pages |
| |---------. |
| | into_shared() |
| | ARef<Request> |
| | refcount = 2 |
| | map_pages() |
| | refcount = 3 |
| | |
| | |
(3) | submit to device |
| |--------------------->|
| | (drop driver ref) |
| | refcount = 2 |
| | (DMA ref remains) |
| | |
| | |
| | .----------------. |
| | | Device does | |
| | | DMA to/from | |
| | | mapped pages | |
| | '----------------' |
| | |
| | |
(4) | completion IRQ |
| |<---------------------|
| | refcount = 2 |
| | tag_to_rq(tag) |
| | ARef<Request> |
| | refcount = 3 |
| | |
| | |
(5) | tear down DMA mappings |
| |---------. |
| | (drop DMA ref) |
| | refcount = 2 |
| | |
| | |
(6) | hand back request | |
|<--------------------| |
| | try_from_shared() |
| | cmpxchg(2 -> 0) |
| | Owned<Request> |
| | refcount = 0 |
| | end_ok() |
| | |
We might be able to get by without shared references (`ARef<Request>`)
and only use an owned reference (`Owned<Request>`) if we add additional
fields to the `Reqeuest` structure. We need to track if the request is
in a state where we can return an `Owned<Requst>` from `tag_to_rq` , and
we need to track if any of the pages of the request are mapped for DMA,
so that we can end the request.
I am not convinced that having the additional fields is worth it for
simplifying the reference counting scheme.
> With regards to the Owned series, I still think we should split it up
> so that the patches making ARef+Owned work like Arc/UniqueArc is
> separate follow-up series.
I'll take that into consideration when sending the next spin of that series.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH 07/13] libmultipath: Add delayed removal support
From: John Garry @ 2026-04-08 11:28 UTC (permalink / raw)
To: Nilay Shroff, hch, kbusch, sagi, axboe, martin.petersen,
james.bottomley, hare
Cc: jmeneghi, linux-nvme, linux-scsi, michael.christie, snitzer,
bmarzins, dm-devel, linux-block, linux-kernel
In-Reply-To: <bc006d17-22b6-49d5-9e04-02eab7dab729@linux.ibm.com>
On 02/03/2026 12:41, Nilay Shroff wrote:
>> +
>> void mpath_add_sysfs_link(struct mpath_disk *mpath_disk)
>> {
>> struct mpath_head *mpath_head = mpath_disk->mpath_head;
>> @@ -793,6 +868,8 @@ struct mpath_head *mpath_alloc_head(void)
>> mutex_init(&mpath_head->lock);
>> kref_init(&mpath_head->ref);
>> + mpath_head->delayed_removal_secs = 0;
>> +
>> INIT_WORK(&mpath_head->requeue_work, mpath_requeue_work);
>> spin_lock_init(&mpath_head->requeue_lock);
>> bio_list_init(&mpath_head->requeue_list);
>
> I think we also need to initialize ->drv_module here.
Hi Nilay,
I am just coming back to this now. About NVMe multipath delayed disk
removal, did you consider a blktests testcase to cover it? I might look
at it if I have a chance (and it makes sense to do so).
Thanks!
^ permalink raw reply
* Re: [PATCH 5/5] xfs: use bio_await in xfs_zone_gc_reset_sync
From: Chaitanya Kulkarni @ 2026-04-08 7:57 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Carlos Maiolino, Bart Van Assche, Damien Le Moal,
linux-block@vger.kernel.org, linux-xfs@vger.kernel.org
In-Reply-To: <20260407140538.633364-6-hch@lst.de>
On 4/7/26 07:05, Christoph Hellwig wrote:
> Replace the open-coded bio wait logic with the new bio_await helper.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> Reviewed-by: Damien Le Moal<dlemoal@kernel.org>
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply
* Re: [PATCH 2/5] block: unify the synchronous bi_end_io callbacks
From: Chaitanya Kulkarni @ 2026-04-08 7:56 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Carlos Maiolino, Bart Van Assche, Damien Le Moal,
linux-block@vger.kernel.org, linux-xfs@vger.kernel.org
In-Reply-To: <20260407140538.633364-3-hch@lst.de>
On 4/7/26 07:05, Christoph Hellwig wrote:
> Put the bio in bio_await_chain after waiting for the completion, and
> share the now identical callbacks between submit_bio_wait and
> bio_await_chain.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> Reviewed-by: Bart Van Assche<bvanassche@acm.org>
> Reviewed-by: Damien Le Moal<dlemoal@kernel.org>
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply
* Re: [PATCH 2/2] block: allow different-pgmap pages as separate bvecs in bio_add_page
From: Christoph Hellwig @ 2026-04-08 6:08 UTC (permalink / raw)
To: Naman Jain
Cc: Christoph Hellwig, Jens Axboe, Chaitanya Kulkarni, John Hubbard,
Logan Gunthorpe, linux-kernel, linux-block, Saurabh Sengar,
Long Li, Michael Kelley
In-Reply-To: <6106a655-2d4b-4ebf-b269-459e3bd3efd4@linux.microsoft.com>
Hi Naman,
On Tue, Apr 07, 2026 at 12:38:30PM +0530, Naman Jain wrote:
>> So the zone_device_pages_have_same_pgmap check should go into
>> zone_device_pages_compatible and we need to stop building the bio
>> as well in that case.
>
> Ok, so rest all things same, from my last email, but my previous compatible
> function would look like this:
>
> static inline bool zone_device_pages_compatible(const struct page *a,
> const struct page *b)
> {
> if (is_pci_p2pdma_page(a) || is_pci_p2pdma_page(b))
> return zone_device_pages_have_same_pgmap(a, b);
> return true;
> }
>
> This would prevent two P2PDMA pages from different pgmaps (different PCI
> devices) passing the compatible check and both get added to the bio.
> Please correct me if that is not what you meant. I'll wait for a couple
> more days and send the next version with this, and we can review this
> again.
This looks good modulo the tabs vs spaces in the indentation.
^ permalink raw reply
* Re: [PATCH 08/20] drbd: add DAX/PMEM support for metadata access
From: Christoph Hellwig @ 2026-04-08 5:46 UTC (permalink / raw)
To: Christoph Böhmwalder
Cc: Jens Axboe, drbd-dev, linux-kernel, Lars Ellenberg,
Philipp Reisner, linux-block, Joel Colledge, an Williams,
Vishal Verma, Dave Jiang, nvdimm
In-Reply-To: <20260327223820.2244227-9-christoph.boehmwalder@linbit.com>
Adding the dax maintainers and they really need to review this.
> + long want = (drbd_md_last_sector(bdev) + 1 - first_sector) >> (PAGE_SHIFT - SECTOR_SHIFT);
> + pgoff_t pgoff = first_sector >> (PAGE_SHIFT - SECTOR_SHIFT);
> + long md_offset_byte = (bdev->md.md_offset - first_sector) << SECTOR_SHIFT;
> + long al_offset_byte = (al_sector - first_sector) << SECTOR_SHIFT;
You really want helpers to make all these unit conversions maintainable.
^ permalink raw reply
* Re: [PATCH 06/20] drbd: add RDMA transport implementation
From: Christoph Hellwig @ 2026-04-08 5:42 UTC (permalink / raw)
To: Christoph Böhmwalder
Cc: Jens Axboe, drbd-dev, linux-kernel, Lars Ellenberg,
Philipp Reisner, linux-block, Joel Colledge, linux-rdma,
Jason Gunthorpe, Leon Romanovsky
In-Reply-To: <20260327223820.2244227-7-christoph.boehmwalder@linbit.com>
You really need to add the RDMA mailing list before adding new RDMA
code. I'll try to review the bits I still remember, but you also
need a maintainer ACK.
> +#ifndef SENDER_COMPACTS_BVECS
> +/* My benchmarking shows a limit of 30 MB/s
> + * with the current implementation of this idea.
> + * cpu bound, perf top shows mainly get_page/put_page.
> + * Without this, using the plain send_page,
> + * I achieve > 400 MB/s on the same system.
> + * => disable for now, improve later.
> + */
> +#define SENDER_COMPACTS_BVECS 0
> +#endif
Nothing explains what "this idea" is. And we do not add dead code as
a rule of thumb anyway.
> +/* Nearly all data transfer uses the send/receive semantics. No need to
Please use the normal kernel command style:
/*
* Blah, blah, blah.
*/
> +int allocation_size;
> +/* module_param(allocation_size, int, 0664);
> + MODULE_PARM_DESC(allocation_size, "Allocation size for receive buffers (page size of peer)");
> +
> + That needs to be implemented in dtr_create_rx_desc() and in dtr_recv() and dtr_recv_pages() */
> +
> +/* If no recvbuf_size or sendbuf_size is configured use 1M plus two pages for the DATA_STREAM */
> +/* Actually it is not a buffer, but the number of tx_descs or rx_descs we allow,
> + very comparable to the socket sendbuf and recvbuf sizes */
Please don't add random unused code.
Also while the kernel coding style has an exception for overly long
lines for selected lines where the improve readability, that by
definition can't apply to block comments.
> +/* Assuming that a singe 4k write should be at the highest scatterd over 8
> + pages. I.e. has no parts smaller than 512 bytes.
> + Arbitrary assumption. It seems that Mellanox hardware can do up to 29
> + ppc64 page size might be 64k */
> +#if (PAGE_SIZE / 512) > 28
> +# define DTR_MAX_TX_SGES 28
> +#else
> +# define DTR_MAX_TX_SGES (PAGE_SIZE / 512)
> +#endif
All this looks complete bollocks. We had multi-page bvecs for years,
aso the page size should not apply for the I/O path.
> +union dtr_immediate {
> + struct {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> + unsigned int sequence:SEQUENCE_BITS;
> + unsigned int stream:2;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> + unsigned int stream:2;
> + unsigned int sequence:SEQUENCE_BITS;
> +#else
> +# error "this endianness is not supported"
> +#endif
> + };
> + unsigned int i;
> +};
Bitfields for on-the-write structures are an anti-pattern, Please
use proper masking and shifting like just about everyone else in modern
code.
> +static int dtr_init(struct drbd_transport *transport);
> +static void dtr_free(struct drbd_transport *transport, enum drbd_tr_free_op);
> +static int dtr_prepare_connect(struct drbd_transport *transport);
> +static int dtr_connect(struct drbd_transport *transport);
> +static void dtr_finish_connect(struct drbd_transport *transport);
The code structure here is totally messed up if you need all these
dozens of forward declarations. Please reshuffled it that you only
need those actually required,
> +static struct drbd_transport_class rdma_transport_class = {
> + .name = "rdma",
> + .instance_size = sizeof(struct dtr_transport),
> + .path_instance_size = sizeof(struct dtr_path),
> + .listener_instance_size = sizeof(struct dtr_listener),
> + .ops = (struct drbd_transport_ops) {
nested struct initializers don't need casts.
> +static bool atomic_inc_if_below(atomic_t *v, int limit)
> +{
> + int old, cur;
> +
> + cur = atomic_read(v);
> + do {
> + old = cur;
> + if (old >= limit)
> + return false;
> +
> + cur = atomic_cmpxchg(v, old, old + 1);
> + } while (cur != old);
> +
> + return true;
> +}
Don't add you own atomics, please talk to the atomics maintainers
instead of adding hacks like this.
> + err = -ENOMEM;
> + tx_desc = kzalloc(sizeof(*tx_desc) + sizeof(struct ib_sge), gfp_mask);
> + if (!tx_desc)
> + goto out_put;
This is supposed to use the new flex kmalloc family of functions
(as much as I hate them).
> + send_buffer = kmalloc(size, gfp_mask);
> + if (!send_buffer) {
> + kfree(tx_desc);
> + goto out_put;
> + }
> + memcpy(send_buffer, buf, size);
and this is a kmemdup.
> + if (err) {
> + kfree(tx_desc);
> + kfree(send_buffer);
> + goto out_put;
> + }
And please use proper gotos to unwind.
> +static int dtr_recv(struct drbd_transport *transport, enum drbd_stream stream, void **buf, size_t size, int flags)
> +{
> + struct dtr_transport *rdma_transport;
> + int err;
> +
> + if (!transport)
> + return -ECONNRESET;
How can this be NULL?
> +static int dtr_path_prepare(struct dtr_path *path, struct dtr_cm *cm, bool active)
> +{
> + struct dtr_cm *cm2;
> + int i, err;
> +
> + cm2 = cmpxchg(&path->cm, NULL, cm); // RCU xchg
Wht's that comment supposed to mean? If it is a rcu_replace_pointer,
use that. If not the comment looks really odd.
> + err = dtr_cm_alloc_rdma_res(cm);
> +
> + return err;
You can return the value directly here.
Giving up for now. I think this needs a sweep for basic sanity an
a review from the RDMA maintainers before we can go into more details.
^ permalink raw reply
* Re: [PATCH 00/20] DRBD 9 rework
From: Christoph Hellwig @ 2026-04-08 5:17 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Böhmwalder, drbd-dev, linux-kernel, Lars Ellenberg,
Philipp Reisner, linux-block
In-Reply-To: <ecf00b4b-c3f8-4161-a97d-3d23b423cabf@kernel.dk>
On Thu, Apr 02, 2026 at 07:30:35PM -0600, Jens Axboe wrote:
> On 3/27/26 4:38 PM, Christoph Böhmwalder wrote:
> > As discussed (context: [0]), here is the first version of our DRBD 9
> > rework series, intended for for-next via for-7.1/drbd.
>
> Will you fixup the kerneldoc (nits) and the assigned-but-not-read
> issues and send out a new version? Also looks this series doesn't
> actually apply to for-7.1/block, patch 12 fails.
Looks like all this went in without any review? Since when have we
started these grand replacements anyway, as they are known to cause
bugs and regressions?
^ permalink raw reply
* Re: [PATCH 4/5] block: add a bio_submit_or_kill helper
From: Christoph Hellwig @ 2026-04-08 5:16 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, Jens Axboe, Carlos Maiolino, Damien Le Moal,
linux-block, linux-xfs
In-Reply-To: <b2e8e413-39bf-4255-8aad-a30d7fd676ea@acm.org>
On Tue, Apr 07, 2026 at 11:37:50AM -0700, Bart Van Assche wrote:
> Without this change, if a fatal signal is pending before the loop starts,
> -EINTR is returned. With this patch applied, 0 is returned,
> isn't it?
Yes, I'll look into a fix.
^ permalink raw reply
* Re: [PATCH 3/5] block: factor out a bio_await helper
From: Christoph Hellwig @ 2026-04-08 5:11 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, Jens Axboe, Carlos Maiolino, Damien Le Moal,
linux-block, linux-xfs
In-Reply-To: <dbbd5553-1442-4a90-88cd-59348f81cbb7@acm.org>
On Tue, Apr 07, 2026 at 07:59:57AM -0700, Bart Van Assche wrote:
>> }
> The comment above bio_await() says that the callback function passed as
> third argument submits the bio. The above code passes bio_end_cb as the
> "submit" argument. bio_end_cb() does something else than submitting a
> bio. How about renaming the "submit" argument of bio_await() into
> something else, e.g. "cb" or "callback" to make its role more clear?
It was called kick and Damien objected politely. A direct completion
isn't really distinguishable from a submission anyway, submit_bio
could complete the bio with an error before doing much.
^ permalink raw reply
* Re: [PATCH 2/6] block: use integrity interval instead of sector as seed
From: Christoph Hellwig @ 2026-04-08 5:10 UTC (permalink / raw)
To: Caleb Sander Mateos
Cc: Christoph Hellwig, Jens Axboe, Sagi Grimberg, Chaitanya Kulkarni,
Martin K. Petersen, linux-block, linux-kernel, linux-nvme,
linux-scsi, target-devel
In-Reply-To: <CADUfDZqHyTDO6aYed+mtChU9m3iPgW=hghwfifuCV8Z_CzxoFQ@mail.gmail.com>
On Tue, Apr 07, 2026 at 09:48:52AM -0700, Caleb Sander Mateos wrote:
> On Sun, Apr 5, 2026 at 11:35 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Fri, Apr 03, 2026 at 01:41:05PM -0600, Caleb Sander Mateos wrote:
> > > void bio_integrity_setup_default(struct bio *bio)
> > > {
> > > struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
> > > struct bio_integrity_payload *bip = bio_integrity(bio);
> > >
> > > - bip_set_seed(bip, bio->bi_iter.bi_sector);
> > > + bip_set_seed(bip, bio_integrity_intervals(bi, bio->bi_iter.bi_sector));
> >
> > Should we simply switch bip_set_seed to take a bio bvec_iter argument and
> > lift all this logic into it? That feels a lot less fragile.
>
> Perhaps I'm misunderstanding the suggestion, but how would that work
> for initializing the seed from struct uio_meta in
> bio_integrity_map_iter()?
>
> bip_set_seed(bio_integrity(bio), meta->seed);
Just open code the assignment there as the code must be build with
blk-integrityu support?
^ permalink raw reply
* [GIT PULL] md-7.1-20260407
From: Yu Kuai @ 2026-04-08 5:05 UTC (permalink / raw)
To: axboe, linux-block, linux-raid
Cc: linux-kernel, song, linan122, abd.masalkhi, chiamingc, gourry,
moonafterrain, xni, yukuai
Hi Jens,
Please consider pulling the following changes into your for-7.1/block branch.
This pull request contains:
Bug Fixes:
- avoid a sysfs deadlock when clearing array state (Yu Kuai)
- validate raid5 journal payloads before reading metadata (Junrui Luo)
- fall back to the correct bitmap operations after version mismatches (Yu Kuai)
- serialize overlapping writes on writemostly raid1 disks (Xiao Ni)
- wake raid456 reshape waiters before suspend (Yu Kuai)
- prevent retry_aligned_read() from triggering soft lockups (Chia-Ming Chang)
Improvements:
- switch raid0 strip zone and devlist allocations to kvmalloc helpers (Gregory Price)
- track clean unwritten stripes for proactive RAID5 parity building (Yu Kuai)
- speed up initial llbitmap sync with write_zeroes_unmap support (Yu Kuai)
Cleanups:
- remove the unused static md workqueue definition (Abd-Alrhman Masalkhi)
Thanks,
Kuai
---
The following changes since commit 8b155f2e4a91f3507951e6ace4b413688ac28b96:
block: remove unused BVEC_ITER_ALL_INIT (2026-04-04 08:10:37 -0600)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mdraid/linux.git tags/md-7.1-20260407
for you to fetch changes up to 7f9f7c697474268d9ef9479df3ddfe7cdcfbbffc:
md/raid5: fix soft lockup in retry_aligned_read() (2026-04-07 15:13:52 +0800)
----------------------------------------------------------------
Abd-Alrhman Masalkhi (1):
md: remove unused static md_wq workqueue
Chia-Ming Chang (1):
md/raid5: fix soft lockup in retry_aligned_read()
Gregory Price (1):
md/raid0: use kvzalloc/kvfree for strip_zone and devlist allocations
Junrui Luo (1):
md/raid5: validate payload size before accessing journal metadata
Xiao Ni (1):
md/raid1: serialize overlap io for writemostly disk
Yu Kuai (5):
md: fix array_state=clear sysfs deadlock
md: add fallback to correct bitmap_ops on version mismatch
md/md-llbitmap: add CleanUnwritten state for RAID-5 proactive parity building
md/md-llbitmap: optimize initial sync with write_zeroes_unmap support
md: wake raid456 reshape waiters before suspend
drivers/md/md-llbitmap.c | 202 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/md/md.c | 139 +++++++++++++++++++++++++++++---
drivers/md/md.h | 5 +-
drivers/md/raid0.c | 18 ++---
drivers/md/raid1.c | 47 ++++++++---
drivers/md/raid5-cache.c | 48 +++++++----
drivers/md/raid5.c | 8 +-
7 files changed, 405 insertions(+), 62 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 00/10] ublk: add shared memory zero-copy support
From: Ming Lei @ 2026-04-08 3:03 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: Jens Axboe, linux-block
In-Reply-To: <CADUfDZoGGUcZ1_ifsXrh_8uy-7DVH30xrbaA+sH8ftwYxB-0Hw@mail.gmail.com>
On Tue, Apr 07, 2026 at 12:29:39PM -0700, Caleb Sander Mateos wrote:
> On Mon, Apr 6, 2026 at 7:39 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Tue, Mar 31, 2026 at 11:31:51PM +0800, Ming Lei wrote:
> > > Hello,
> > >
> > > Add shared memory based zero-copy (UBLK_F_SHMEM_ZC) support for ublk.
> > >
> > > The ublk server and its client share a memory region (e.g. memfd or
> > > hugetlbfs file) via MAP_SHARED mmap. The server registers this region
> > > with the kernel via UBLK_U_CMD_REG_BUF, which pins the pages and
> > > builds a PFN maple tree. When I/O arrives, the driver looks up bio
> > > pages in the maple tree — if they match registered buffer pages, the
> > > data is used directly without copying.
> > >
> > > Please see details on document added in patch 3.
> > >
> > > Patches 1-4 implement the kernel side:
> > > - buffer register/unregister control commands with PFN coalescing,
> > > including read-only buffer support (UBLK_SHMEM_BUF_READ_ONLY)
> > > - PFN-based matching in the I/O path, with enforcement that read-only
> > > buffers reject non-WRITE requests
> > > - UBLK_F_SHMEM_ZC feature flag
> > > - eliminate permanent pages[] array from struct ublk_buf; the maple
> > > tree already stores PFN ranges, so pages[] becomes temporary
> > >
> > > Patches 5-10 add kublk (selftest server) support and tests:
> > > - hugetlbfs buffer sharing (both kublk and fio mmap the same file)
> > > - null target and loop target tests with fio verify
> > > - filesystem-level test (ext4 on ublk, fio verify on a file)
> > > - read-only buffer registration test (--rdonly_shmem_buf)
> > >
> > > Changes since V1:
> > > - rename struct ublk_buf_reg to struct ublk_shmem_buf_reg, add __u32
> > > flags field for extensibility, narrow __u64 len to __u32 (max 4GB
> > > per UBLK_SHMEM_ZC_OFF_MASK), remove __u32 reserved (patch 1)
> > > - add UBLK_SHMEM_BUF_READ_ONLY flag: pin pages without FOLL_WRITE,
> > > enabling registration of write-sealed memfd buffers (patch 1)
> > > - use backward-compatible struct reading: memset zero + copy
> > > min(header->len, sizeof(struct)) (patch 1)
> > > - reorder struct ublk_buf_range fields for better packing (16 bytes
> > > vs 24 bytes), change buf_index to unsigned short, add unsigned short
> > > flags to store per-range read-only state (patch 1)
> > > - enforce read-only buffer semantics in ublk_try_buf_match(): reject
> > > non-WRITE requests on read-only buffers since READ I/O needs to
> > > write data into the buffer (patch 2)
> > > - narrow struct ublk_buf::nr_pages to unsigned int, narrow struct
> > > ublk_buf_range::base_offset to unsigned int (patch 1)
> > > - add new patch 4: eliminate permanent pages[] array from struct
> > > ublk_buf — recover struct page pointers via pfn_to_page() from the
> > > maple tree during unregistration, saving 2MB per 1GB buffer
> > > - add UBLK_F_SHMEM_ZC to feat_map in kublk (patch 5)
> > > - add new patch 10: read-only buffer registration selftest with
> > > --rdonly_shmem_buf option on null target + hugetlbfs
> >
> > Hello,
> >
> > Ping...
>
> Sorry I didn't get a chance to look at this earlier. It looks really
> nice, thank you for implementing it! I have just a few comments. One
> is about the UAPI (can we just use virtual addresses instead of buffer
> index + offset), so I might wait on landing this patchset until we've
> finalized that.
Hi Jens,
Can you drop V2 from for-7.1/block so that we can polish up it in V3?
Or I am fine to cook up a followup for misc clean & fix?
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH v2 04/10] ublk: eliminate permanent pages[] array from struct ublk_buf
From: Ming Lei @ 2026-04-08 2:58 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: Jens Axboe, linux-block
In-Reply-To: <CADUfDZow_gmwzQk41kn=Hw4w4bhsF9ogm2ZPGNgRuz9d5aF_kQ@mail.gmail.com>
On Tue, Apr 07, 2026 at 12:50:15PM -0700, Caleb Sander Mateos wrote:
> On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > The pages[] array (kvmalloc'd, 8 bytes per page = 2MB for a 1GB buffer)
> > was stored permanently in struct ublk_buf but only needed during
> > pin_user_pages_fast() and maple tree construction. Since the maple tree
> > already stores PFN ranges via ublk_buf_range, struct page pointers can
> > be recovered via pfn_to_page() during unregistration.
> >
> > Make pages[] a temporary allocation in ublk_ctrl_reg_buf(), freed
> > immediately after the maple tree is built. Rewrite __ublk_ctrl_unreg_buf()
> > to iterate the maple tree for matching buf_index entries, recovering
> > struct page pointers via pfn_to_page() and unpinning in batches of 32.
> > Simplify ublk_buf_erase_ranges() to iterate the maple tree by buf_index
> > instead of walking the now-removed pages[] array.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > drivers/block/ublk_drv.c | 87 +++++++++++++++++++++++++---------------
> > 1 file changed, 55 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index c2b9992503a4..2e475bdc54dd 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -296,7 +296,6 @@ struct ublk_queue {
> >
> > /* Per-registered shared memory buffer */
> > struct ublk_buf {
> > - struct page **pages;
> > unsigned int nr_pages;
> > };
>
> It looks like nr_pages doesn't need to be stored either, it could just
> be passed to __ublk_ctrl_reg_buf(). Then I think we could get rid of
> struct ublk_buf and the xarray entirely. We really just need a bitmap
> for allocating buffer indices.
Maybe idr_alloc().
>
> >
> > @@ -5261,27 +5260,25 @@ static void ublk_unquiesce_and_resume(struct gendisk *disk)
> > * coalescing consecutive PFNs into single range entries.
> > * Returns 0 on success, negative error with partial insertions unwound.
> > */
> > -/* Erase coalesced PFN ranges from the maple tree for pages [0, nr_pages) */
> > -static void ublk_buf_erase_ranges(struct ublk_device *ub,
> > - struct ublk_buf *ubuf,
> > - unsigned long nr_pages)
> > +/* Erase coalesced PFN ranges from the maple tree matching buf_index */
> > +static void ublk_buf_erase_ranges(struct ublk_device *ub, int buf_index)
> > {
> > - unsigned long i;
> > -
> > - for (i = 0; i < nr_pages; ) {
> > - unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > - unsigned long start = i;
> > + MA_STATE(mas, &ub->buf_tree, 0, ULONG_MAX);
> > + struct ublk_buf_range *range;
> >
> > - while (i + 1 < nr_pages &&
> > - page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > - i++;
> > - i++;
> > - kfree(mtree_erase(&ub->buf_tree, pfn));
> > + mas_lock(&mas);
> > + mas_for_each(&mas, range, ULONG_MAX) {
> > + if (range->buf_index == buf_index) {
> > + mas_erase(&mas);
> > + kfree(range);
> > + }
> > }
> > + mas_unlock(&mas);
> > }
> >
> > static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> > - struct ublk_buf *ubuf, int index,
> > + struct ublk_buf *ubuf,
> > + struct page **pages, int index,
> > unsigned short flags)
> > {
> > unsigned long nr_pages = ubuf->nr_pages;
> > @@ -5289,13 +5286,13 @@ static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> > int ret;
> >
> > for (i = 0; i < nr_pages; ) {
> > - unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > + unsigned long pfn = page_to_pfn(pages[i]);
> > unsigned long start = i;
> > struct ublk_buf_range *range;
> >
> > /* Find run of consecutive PFNs */
> > while (i + 1 < nr_pages &&
> > - page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > + page_to_pfn(pages[i + 1]) == pfn + (i - start) + 1)
> > i++;
> > i++; /* past the last page in this run */
> >
> > @@ -5320,7 +5317,7 @@ static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> > return 0;
> >
> > unwind:
> > - ublk_buf_erase_ranges(ub, ubuf, i);
> > + ublk_buf_erase_ranges(ub, index);
> > return ret;
> > }
> >
> > @@ -5335,6 +5332,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > void __user *argp = (void __user *)(unsigned long)header->addr;
> > struct ublk_shmem_buf_reg buf_reg;
> > unsigned long addr, size, nr_pages;
> > + struct page **pages = NULL;
> > unsigned int gup_flags;
> > struct gendisk *disk;
> > struct ublk_buf *ubuf;
> > @@ -5371,9 +5369,8 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > goto put_disk;
> > }
> >
> > - ubuf->pages = kvmalloc_array(nr_pages, sizeof(*ubuf->pages),
> > - GFP_KERNEL);
> > - if (!ubuf->pages) {
> > + pages = kvmalloc_array(nr_pages, sizeof(*pages), GFP_KERNEL);
> > + if (!pages) {
> > ret = -ENOMEM;
> > goto err_free;
> > }
> > @@ -5382,7 +5379,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > if (!(buf_reg.flags & UBLK_SHMEM_BUF_READ_ONLY))
> > gup_flags |= FOLL_WRITE;
> >
> > - pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, ubuf->pages);
> > + pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, pages);
> > if (pinned < 0) {
> > ret = pinned;
> > goto err_free_pages;
> > @@ -5406,7 +5403,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > if (ret)
> > goto err_unlock;
> >
> > - ret = __ublk_ctrl_reg_buf(ub, ubuf, index, buf_reg.flags);
> > + ret = __ublk_ctrl_reg_buf(ub, ubuf, pages, index, buf_reg.flags);
> > if (ret) {
> > xa_erase(&ub->bufs_xa, index);
> > goto err_unlock;
> > @@ -5414,6 +5411,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> >
> > mutex_unlock(&ub->mutex);
> >
> > + kvfree(pages);
> > ublk_unquiesce_and_resume(disk);
> > ublk_put_disk(disk);
> > return index;
> > @@ -5422,9 +5420,9 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > mutex_unlock(&ub->mutex);
> > ublk_unquiesce_and_resume(disk);
> > err_unpin:
> > - unpin_user_pages(ubuf->pages, pinned);
> > + unpin_user_pages(pages, pinned);
> > err_free_pages:
> > - kvfree(ubuf->pages);
> > + kvfree(pages);
> > err_free:
> > kfree(ubuf);
> > put_disk:
> > @@ -5433,11 +5431,36 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > }
> >
> > static void __ublk_ctrl_unreg_buf(struct ublk_device *ub,
> > - struct ublk_buf *ubuf)
> > + struct ublk_buf *ubuf, int buf_index)
>
> ubuf is only passed to kfree() now, maybe it would make sense to move
> that to the caller so the argument can be dropped?
Yeah, looks fine.
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH v2 03/10] ublk: enable UBLK_F_SHMEM_ZC feature flag
From: Ming Lei @ 2026-04-08 2:50 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: Jens Axboe, linux-block
In-Reply-To: <CADUfDZrFe2jWV1SpA6opQXB_Zx7COGXEeJAFNRWdMAaaDHomWQ@mail.gmail.com>
On Tue, Apr 07, 2026 at 12:47:58PM -0700, Caleb Sander Mateos wrote:
> On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Add UBLK_F_SHMEM_ZC (1ULL << 19) to the UAPI header and UBLK_F_ALL.
> > Switch ublk_support_shmem_zc() and ublk_dev_support_shmem_zc() from
> > returning false to checking the actual flag, enabling the shared
> > memory zero-copy feature for devices that request it.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > Documentation/block/ublk.rst | 117 ++++++++++++++++++++++++++++++++++
> > drivers/block/ublk_drv.c | 7 +-
> > include/uapi/linux/ublk_cmd.h | 7 ++
> > 3 files changed, 128 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
> > index 6ad28039663d..a818e09a4b66 100644
> > --- a/Documentation/block/ublk.rst
> > +++ b/Documentation/block/ublk.rst
> > @@ -485,6 +485,123 @@ Limitations
> > in case that too many ublk devices are handled by this single io_ring_ctx
> > and each one has very large queue depth
> >
> > +Shared Memory Zero Copy (UBLK_F_SHMEM_ZC)
> > +------------------------------------------
> > +
> > +The ``UBLK_F_SHMEM_ZC`` feature provides an alternative zero-copy path
> > +that works by sharing physical memory pages between the client application
> > +and the ublk server. Unlike the io_uring fixed buffer approach above,
> > +shared memory zero copy does not require io_uring buffer registration
> > +per I/O — instead, it relies on the kernel matching page frame numbers
> > +(PFNs) at I/O time. This allows the ublk server to access the shared
>
> Maybe "physical pages" would be clearer than the kernel-internal
> concept of "page frame numbers"?
OK, but it is one kernel doc, PFN shouldn't be bad.
>
> > +buffer directly, which is unlikely for the io_uring fixed buffer
> > +approach.
> > +
> > +Motivation
> > +~~~~~~~~~~
> > +
> > +Shared memory zero copy takes a different approach: if the client
> > +application and the ublk server both map the same physical memory, there is
> > +nothing to copy. The kernel detects the shared pages automatically and
> > +tells the server where the data already lives.
> > +
> > +``UBLK_F_SHMEM_ZC`` can be thought of as a supplement for optimized client
> > +applications — when the client is willing to allocate I/O buffers from
> > +shared memory, the entire data path becomes zero-copy without any per-I/O
> > +overhead.
>
> nit: The shmem buffer lookup still has some overhead. I think just
> "becomes zero-copy" would be fine.
Fine, mapple tree has very small depth, the lookup cost is pretty small.
>
> > +
> > +Use Cases
> > +~~~~~~~~~
> > +
> > +This feature is useful when the client application can be configured to
> > +use a specific shared memory region for its I/O buffers:
> > +
> > +- **Custom storage clients** that allocate I/O buffers from shared memory
> > + (memfd, hugetlbfs) and issue direct I/O to the ublk device
> > +- **Database engines** that use pre-allocated buffer pools with O_DIRECT
> > +
> > +How It Works
> > +~~~~~~~~~~~~
> > +
> > +1. The ublk server and client both ``mmap()`` the same file (memfd or
> > + hugetlbfs) with ``MAP_SHARED``. This gives both processes access to the
> > + same physical pages.
> > +
> > +2. The ublk server registers its mapping with the kernel::
> > +
> > + struct ublk_buf_reg buf = { .addr = mmap_va, .len = size };
> > + ublk_ctrl_cmd(UBLK_U_CMD_REG_BUF, .addr = &buf);
>
> This doesn't look like valid C syntax. Maybe it could say something like:
> struct ublksrv_ctrl_cmd cmd = {.dev_id = dev_id, .addr = &buf, .len =
> sizeof(buf)};
> io_uring_prep_uring_cmd(sqe, UBLK_U_CMD_REG_BUF, ublk_control_fd);
> memcpy(sqe->cmd, &cmd, sizeof(cmd));
It is pseudocode, looks not a big deal.
>
> > +
> > + The kernel pins the pages and builds a PFN lookup tree.
> > +
> > +3. When the client issues direct I/O (``O_DIRECT``) to ``/dev/ublkb*``,
> > + the kernel checks whether the I/O buffer pages match any registered
> > + pages by comparing PFNs.
> > +
> > +4. On a match, the kernel sets ``UBLK_IO_F_SHMEM_ZC`` in the I/O
> > + descriptor and encodes the buffer index and offset in ``addr``::
> > +
> > + if (iod->op_flags & UBLK_IO_F_SHMEM_ZC) {
> > + /* Data is already in our shared mapping — zero copy */
> > + index = ublk_shmem_zc_index(iod->addr);
> > + offset = ublk_shmem_zc_offset(iod->addr);
> > + buf = shmem_table[index].mmap_base + offset;
> > + }
> > +
> > +5. If pages do not match (e.g., the client used a non-shared buffer),
> > + the I/O falls back to the normal copy path silently.
> > +
> > +The shared memory can be set up via two methods:
> > +
> > +- **Socket-based**: the client sends a memfd to the ublk server via
> > + ``SCM_RIGHTS`` on a unix socket. The server mmaps and registers it.
> > +- **Hugetlbfs-based**: both processes ``mmap(MAP_SHARED)`` the same
> > + hugetlbfs file. No IPC needed — same file gives same physical pages.
> > +
> > +Advantages
> > +~~~~~~~~~~
> > +
> > +- **Simple**: no per-I/O buffer registration or unregistration commands.
> > + Once the shared buffer is registered, all matching I/O is zero-copy
> > + automatically.
> > +- **Direct buffer access**: the ublk server can read and write the shared
> > + buffer directly via its own mmap, without going through io_uring fixed
> > + buffer operations. This is more friendly for server implementations.
> > +- **Fast**: PFN matching is a single maple tree lookup per bvec. No
> > + io_uring command round-trips for buffer management.
> > +- **Compatible**: non-matching I/O silently falls back to the copy path.
> > + The device works normally for any client, with zero-copy as an
> > + optimization when shared memory is available.
> > +
> > +Limitations
> > +~~~~~~~~~~~
> > +
> > +- **Requires client cooperation**: the client must allocate its I/O
> > + buffers from the shared memory region. This requires a custom or
> > + configured client — standard applications using their own buffers
> > + will not benefit.
> > +- **Direct I/O only**: buffered I/O (without ``O_DIRECT``) goes through
> > + the page cache, which allocates its own pages. These kernel-allocated
> > + pages will never match the registered shared buffer. Only ``O_DIRECT``
> > + puts the client's buffer pages directly into the block I/O.
>
> One other limitation that might be worth mentioning is that
> scatter/gather I/O can't use the SHMEM_ZC optimization, as the
> request's data must be contiguous in the registered virtual address
> range.
Good catch, will document this limit.
It could be supported in future by introducing bpf, and bpf prog can use its
map(such as arena) to build iov like data to userspace.
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH v2 02/10] ublk: add PFN-based buffer matching in I/O path
From: Ming Lei @ 2026-04-08 2:36 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: Jens Axboe, linux-block
In-Reply-To: <CADUfDZq3rTwcnxAekdan3-fi3qdLwFp-T5LsJJ1viqoUv8RqOw@mail.gmail.com>
On Tue, Apr 07, 2026 at 12:47:29PM -0700, Caleb Sander Mateos wrote:
> On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Add ublk_try_buf_match() which walks a request's bio_vecs, looks up
> > each page's PFN in the per-device maple tree, and verifies all pages
> > belong to the same registered buffer at contiguous offsets.
> >
> > Add ublk_iod_is_shmem_zc() inline helper for checking whether a
> > request uses the shmem zero-copy path.
> >
> > Integrate into the I/O path:
> > - ublk_setup_iod(): if pages match a registered buffer, set
> > UBLK_IO_F_SHMEM_ZC and encode buffer index + offset in addr
> > - ublk_start_io(): skip ublk_map_io() for zero-copy requests
> > - __ublk_complete_rq(): skip ublk_unmap_io() for zero-copy requests
> >
> > The feature remains disabled (ublk_support_shmem_zc() returns false)
> > until the UBLK_F_SHMEM_ZC flag is enabled in the next patch.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > drivers/block/ublk_drv.c | 77 +++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index ac6ccc174d44..d53865437600 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -356,6 +356,8 @@ struct ublk_params_header {
> >
> > static void ublk_io_release(void *priv);
> > static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> > +static bool ublk_try_buf_match(struct ublk_device *ub, struct request *rq,
> > + u32 *buf_idx, u32 *buf_off);
>
> buf_idx could be a u16 * for consistency?
Yeah.
>
> > static void ublk_buf_cleanup(struct ublk_device *ub);
> > static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> > static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > @@ -426,6 +428,12 @@ static inline bool ublk_support_shmem_zc(const struct ublk_queue *ubq)
> > return false;
> > }
> >
> > +static inline bool ublk_iod_is_shmem_zc(const struct ublk_queue *ubq,
> > + unsigned int tag)
> > +{
> > + return ublk_get_iod(ubq, tag)->op_flags & UBLK_IO_F_SHMEM_ZC;
> > +}
> > +
> > static inline bool ublk_dev_support_shmem_zc(const struct ublk_device *ub)
> > {
> > return false;
> > @@ -1494,6 +1502,18 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> > iod->nr_sectors = blk_rq_sectors(req);
> > iod->start_sector = blk_rq_pos(req);
> >
> > + /* Try shmem zero-copy match before setting addr */
> > + if (ublk_support_shmem_zc(ubq) && ublk_rq_has_data(req)) {
> > + u32 buf_idx, buf_off;
> > +
> > + if (ublk_try_buf_match(ubq->dev, req,
> > + &buf_idx, &buf_off)) {
> > + iod->op_flags |= UBLK_IO_F_SHMEM_ZC;
> > + iod->addr = ublk_shmem_zc_addr(buf_idx, buf_off);
> > + return BLK_STS_OK;
> > + }
> > + }
> > +
> > iod->addr = io->buf.addr;
> >
> > return BLK_STS_OK;
> > @@ -1539,6 +1559,10 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
> > req_op(req) != REQ_OP_DRV_IN)
> > goto exit;
> >
> > + /* shmem zero copy: no data to unmap, pages already shared */
> > + if (ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))
>
> This is a lot of pointer chasing. Could we track this with a flag on
> struct ublk_io instead?
Yeah, it can be done by adding and setting the new flag in ublk_start_io(), or
we can just pass `ubq` to __ublk_complete_rq() from its call sites.
I prefer to the latter.
>
> > + goto exit;
> > +
> > /* for READ request, writing data in iod->addr to rq buffers */
> > unmapped_bytes = ublk_unmap_io(need_map, req, io);
> >
> > @@ -1697,8 +1721,13 @@ static void ublk_auto_buf_dispatch(const struct ublk_queue *ubq,
> > static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> > struct ublk_io *io)
> > {
> > - unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> > + unsigned mapped_bytes;
> >
> > + /* shmem zero copy: skip data copy, pages already shared */
> > + if (ublk_iod_is_shmem_zc(ubq, req->tag))
> > + return true;
> > +
> > + mapped_bytes = ublk_map_io(ubq, req, io);
> >
> > /* partially mapped, update io descriptor */
> > if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > @@ -5458,7 +5487,53 @@ static void ublk_buf_cleanup(struct ublk_device *ub)
> > mtree_destroy(&ub->buf_tree);
> > }
> >
> > +/* Check if request pages match a registered shared memory buffer */
> > +static bool ublk_try_buf_match(struct ublk_device *ub,
> > + struct request *rq,
> > + u32 *buf_idx, u32 *buf_off)
> > +{
> > + struct req_iterator iter;
> > + struct bio_vec bv;
> > + int index = -1;
> > + unsigned long expected_offset = 0;
> > + bool first = true;
>
> Could check index < 0 in place of first?
>
> > +
> > + rq_for_each_bvec(bv, rq, iter) {
> > + unsigned long pfn = page_to_pfn(bv.bv_page);
> > + struct ublk_buf_range *range;
> > + unsigned long off;
> >
> > + range = mtree_load(&ub->buf_tree, pfn);
> > + if (!range)
> > + return false;
> > +
> > + off = range->base_offset +
> > + (pfn - range->base_pfn) * PAGE_SIZE + bv.bv_offset;
>
> Doesn't this need to check that the end of the bvec is less than the
> end of the range? Otherwise, the bvec could extend into physically
> contiguous pages that aren't part of the registered range.
It is actually fixed in my local tree:
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2e475bdc54dd..69db3b3b9071 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -5524,13 +5524,20 @@ static bool ublk_try_buf_match(struct ublk_device *ub,
rq_for_each_bvec(bv, rq, iter) {
unsigned long pfn = page_to_pfn(bv.bv_page);
+ unsigned long end_pfn = pfn +
+ ((bv.bv_offset + bv.bv_len - 1) >> PAGE_SHIFT);
struct ublk_buf_range *range;
unsigned long off;
+ MA_STATE(mas, &ub->buf_tree, pfn, pfn);
- range = mtree_load(&ub->buf_tree, pfn);
+ range = mas_walk(&mas);
if (!range)
return false;
+ /* verify all pages in this bvec fall within the range */
+ if (end_pfn > mas.last)
+ return false;
+
off = range->base_offset +
(pfn - range->base_pfn) * PAGE_SIZE + bv.bv_offset;
>
> Also, the range could precompute base_pfn - base_offset / PAGE_SIZE
> instead of base_offset to make this a bit cheaper.
>
> > +
> > + if (first) {
> > + /* Read-only buffer can't serve READ (kernel writes) */
> > + if ((range->flags & UBLK_SHMEM_BUF_READ_ONLY) &&
> > + req_op(rq) != REQ_OP_WRITE)
> > + return false;
> > + index = range->buf_index;
> > + expected_offset = off;
> > + *buf_off = off;
> > + first = false;
> > + } else {
> > + if (range->buf_index != index)
> > + return false;
> > + if (off != expected_offset)
> > + return false;
> > + }
> > + expected_offset += bv.bv_len;
> > + }
> > +
> > + if (first)
> > + return false;
>
> How is this case possible? That would mean the request has no bvecs,
> but ublk_try_buf_match() is only called for requests with data, right?
Your are right, will clean it in next version.
Thanks,
Ming
^ permalink raw reply related
* Re: [PATCH v2 01/10] ublk: add UBLK_U_CMD_REG_BUF/UNREG_BUF control commands
From: Ming Lei @ 2026-04-08 2:23 UTC (permalink / raw)
To: Caleb Sander Mateos; +Cc: Jens Axboe, linux-block
In-Reply-To: <CADUfDZpUbses5NNR19M+81ZwDxSAb6ktz0+2f9GfkOL8yzEh+w@mail.gmail.com>
On Tue, Apr 07, 2026 at 12:35:49PM -0700, Caleb Sander Mateos wrote:
> On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Add control commands for registering and unregistering shared memory
> > buffers for zero-copy I/O:
> >
> > - UBLK_U_CMD_REG_BUF (0x18): pins pages from userspace, inserts PFN
> > ranges into a per-device maple tree for O(log n) lookup during I/O.
> > Buffer pointers are tracked in a per-device xarray. Returns the
> > assigned buffer index.
> >
> > - UBLK_U_CMD_UNREG_BUF (0x19): removes PFN entries and unpins pages.
> >
> > Queue freeze/unfreeze is handled internally so userspace need not
> > quiesce the device during registration.
> >
> > Also adds:
> > - UBLK_IO_F_SHMEM_ZC flag and addr encoding helpers in UAPI header
> > (16-bit buffer index supporting up to 65536 buffers)
> > - Data structures (ublk_buf, ublk_buf_range) and xarray/maple tree
> > - __ublk_ctrl_reg_buf() helper for PFN insertion with error unwinding
> > - __ublk_ctrl_unreg_buf() helper for cleanup reuse
> > - ublk_support_shmem_zc() / ublk_dev_support_shmem_zc() stubs
> > (returning false — feature not enabled yet)
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > drivers/block/ublk_drv.c | 300 ++++++++++++++++++++++++++++++++++
> > include/uapi/linux/ublk_cmd.h | 72 ++++++++
> > 2 files changed, 372 insertions(+)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 71c7c56b38ca..ac6ccc174d44 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -46,6 +46,8 @@
> > #include <linux/kref.h>
> > #include <linux/kfifo.h>
> > #include <linux/blk-integrity.h>
> > +#include <linux/maple_tree.h>
> > +#include <linux/xarray.h>
> > #include <uapi/linux/fs.h>
> > #include <uapi/linux/ublk_cmd.h>
> >
> > @@ -58,6 +60,8 @@
> > #define UBLK_CMD_UPDATE_SIZE _IOC_NR(UBLK_U_CMD_UPDATE_SIZE)
> > #define UBLK_CMD_QUIESCE_DEV _IOC_NR(UBLK_U_CMD_QUIESCE_DEV)
> > #define UBLK_CMD_TRY_STOP_DEV _IOC_NR(UBLK_U_CMD_TRY_STOP_DEV)
> > +#define UBLK_CMD_REG_BUF _IOC_NR(UBLK_U_CMD_REG_BUF)
> > +#define UBLK_CMD_UNREG_BUF _IOC_NR(UBLK_U_CMD_UNREG_BUF)
> >
> > #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)
> > @@ -289,6 +293,20 @@ struct ublk_queue {
> > struct ublk_io ios[] __counted_by(q_depth);
> > };
> >
> > +/* Per-registered shared memory buffer */
> > +struct ublk_buf {
> > + struct page **pages;
> > + unsigned int nr_pages;
> > +};
> > +
> > +/* Maple tree value: maps a PFN range to buffer location */
> > +struct ublk_buf_range {
> > + unsigned long base_pfn;
> > + unsigned short buf_index;
> > + unsigned short flags;
> > + unsigned int base_offset; /* byte offset within buffer */
> > +};
> > +
> > struct ublk_device {
> > struct gendisk *ub_disk;
> >
> > @@ -323,6 +341,10 @@ struct ublk_device {
> >
> > bool block_open; /* protected by open_mutex */
> >
> > + /* shared memory zero copy */
> > + struct maple_tree buf_tree;
> > + struct xarray bufs_xa;
> > +
> > struct ublk_queue *queues[];
> > };
> >
> > @@ -334,6 +356,7 @@ struct ublk_params_header {
> >
> > static void ublk_io_release(void *priv);
> > static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> > +static void ublk_buf_cleanup(struct ublk_device *ub);
> > static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> > static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > u16 q_id, u16 tag, struct ublk_io *io);
> > @@ -398,6 +421,16 @@ static inline bool ublk_dev_support_zero_copy(const struct ublk_device *ub)
> > return ub->dev_info.flags & UBLK_F_SUPPORT_ZERO_COPY;
> > }
> >
> > +static inline bool ublk_support_shmem_zc(const struct ublk_queue *ubq)
> > +{
> > + return false;
> > +}
> > +
> > +static inline bool ublk_dev_support_shmem_zc(const struct ublk_device *ub)
> > +{
> > + return false;
> > +}
> > +
> > static inline bool ublk_support_auto_buf_reg(const struct ublk_queue *ubq)
> > {
> > return ubq->flags & UBLK_F_AUTO_BUF_REG;
> > @@ -1460,6 +1493,7 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> > iod->op_flags = ublk_op | ublk_req_build_flags(req);
> > iod->nr_sectors = blk_rq_sectors(req);
> > iod->start_sector = blk_rq_pos(req);
> > +
>
> nit: unrelated whitespace change?
>
> > iod->addr = io->buf.addr;
> >
> > return BLK_STS_OK;
> > @@ -1665,6 +1699,7 @@ static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> > {
> > unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> >
> > +
>
> nit: unrelated whitespace change?
>
> > /* partially mapped, update io descriptor */
> > if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> > /*
> > @@ -4206,6 +4241,7 @@ static void ublk_cdev_rel(struct device *dev)
> > {
> > struct ublk_device *ub = container_of(dev, struct ublk_device, cdev_dev);
> >
> > + ublk_buf_cleanup(ub);
> > blk_mq_free_tag_set(&ub->tag_set);
> > ublk_deinit_queues(ub);
> > ublk_free_dev_number(ub);
> > @@ -4625,6 +4661,8 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
> > mutex_init(&ub->mutex);
> > spin_lock_init(&ub->lock);
> > mutex_init(&ub->cancel_mutex);
> > + mt_init(&ub->buf_tree);
> > + xa_init_flags(&ub->bufs_xa, XA_FLAGS_ALLOC);
> > INIT_WORK(&ub->partition_scan_work, ublk_partition_scan_work);
> >
> > ret = ublk_alloc_dev_number(ub, header->dev_id);
> > @@ -5168,6 +5206,260 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
> > return err;
> > }
> >
> > +/*
> > + * Drain inflight I/O and quiesce the queue. Freeze drains all inflight
> > + * requests, quiesce_nowait marks the queue so no new requests dispatch,
> > + * then unfreeze allows new submissions (which won't dispatch due to
> > + * quiesce). This keeps freeze and ub->mutex non-nested.
> > + */
> > +static void ublk_quiesce_and_release(struct gendisk *disk)
> > +{
> > + unsigned int memflags;
> > +
> > + memflags = blk_mq_freeze_queue(disk->queue);
> > + blk_mq_quiesce_queue_nowait(disk->queue);
> > + blk_mq_unfreeze_queue(disk->queue, memflags);
> > +}
> > +
> > +static void ublk_unquiesce_and_resume(struct gendisk *disk)
> > +{
> > + blk_mq_unquiesce_queue(disk->queue);
> > +}
> > +
> > +/*
> > + * Insert PFN ranges of a registered buffer into the maple tree,
> > + * coalescing consecutive PFNs into single range entries.
> > + * Returns 0 on success, negative error with partial insertions unwound.
> > + */
> > +/* Erase coalesced PFN ranges from the maple tree for pages [0, nr_pages) */
> > +static void ublk_buf_erase_ranges(struct ublk_device *ub,
> > + struct ublk_buf *ubuf,
> > + unsigned long nr_pages)
> > +{
> > + unsigned long i;
> > +
> > + for (i = 0; i < nr_pages; ) {
> > + unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > + unsigned long start = i;
> > +
> > + while (i + 1 < nr_pages &&
> > + page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > + i++;
> > + i++;
> > + kfree(mtree_erase(&ub->buf_tree, pfn));
> > + }
> > +}
> > +
> > +static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> > + struct ublk_buf *ubuf, int index,
> > + unsigned short flags)
> > +{
> > + unsigned long nr_pages = ubuf->nr_pages;
> > + unsigned long i;
> > + int ret;
> > +
> > + for (i = 0; i < nr_pages; ) {
> > + unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> > + unsigned long start = i;
> > + struct ublk_buf_range *range;
> > +
> > + /* Find run of consecutive PFNs */
> > + while (i + 1 < nr_pages &&
> > + page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> > + i++;
> > + i++; /* past the last page in this run */
>
> Move this increment to the for loop so you don't need the "- 1" in the
> mtree_insert_range() call?
Good catch!
>
> > +
> > + range = kzalloc(sizeof(*range), GFP_KERNEL);
>
> Not sure kzalloc() is necessary; all the fields are initialized below
Yeah, kmalloc() is fine, and we shouldn't add more fields to `range` in
future.
>
> > + if (!range) {
> > + ret = -ENOMEM;
> > + goto unwind;
> > + }
> > + range->buf_index = index;
> > + range->flags = flags;
> > + range->base_pfn = pfn;
> > + range->base_offset = start << PAGE_SHIFT;
> > +
> > + ret = mtree_insert_range(&ub->buf_tree, pfn,
> > + pfn + (i - start) - 1,
> > + range, GFP_KERNEL);
> > + if (ret) {
> > + kfree(range);
> > + goto unwind;
> > + }
> > + }
> > + return 0;
> > +
> > +unwind:
> > + ublk_buf_erase_ranges(ub, ubuf, i);
> > + return ret;
> > +}
> > +
> > +/*
> > + * Register a shared memory buffer for zero-copy I/O.
> > + * Pins pages, builds PFN maple tree, freezes/unfreezes the queue
> > + * internally. Returns buffer index (>= 0) on success.
> > + */
> > +static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> > + struct ublksrv_ctrl_cmd *header)
> > +{
> > + void __user *argp = (void __user *)(unsigned long)header->addr;
> > + struct ublk_shmem_buf_reg buf_reg;
> > + unsigned long addr, size, nr_pages;
>
> size and nr_pages could be u32
Yeah, it was caused by internal change on `ublk_shmem_buf_reg`.
>
> > + unsigned int gup_flags;
> > + struct gendisk *disk;
> > + struct ublk_buf *ubuf;
> > + long pinned;
>
> pinned could be int to match the return type of pin_user_pages_fast()
OK.
>
> > + u32 index;
> > + int ret;
> > +
> > + if (!ublk_dev_support_shmem_zc(ub))
> > + return -EOPNOTSUPP;
> > +
> > + memset(&buf_reg, 0, sizeof(buf_reg));
> > + if (copy_from_user(&buf_reg, argp,
> > + min_t(size_t, header->len, sizeof(buf_reg))))
> > + return -EFAULT;
> > +
> > + if (buf_reg.flags & ~UBLK_SHMEM_BUF_READ_ONLY)
> > + return -EINVAL;
> > +
> > + addr = buf_reg.addr;
> > + size = buf_reg.len;
>
> nit: don't see much value in these additional variables that are just
> copies of buf_reg fields
>
> > + nr_pages = size >> PAGE_SHIFT;
> > +
> > + if (!size || !PAGE_ALIGNED(size) || !PAGE_ALIGNED(addr))
> > + return -EINVAL;
> > +
> > + disk = ublk_get_disk(ub);
> > + if (!disk)
> > + return -ENODEV;
>
> So buffers can't be registered before the ublk device is started? Is
> there a reason why that's not possible? Could we just make the
> ublk_quiesce_and_release() and ublk_unquiesce_and_resume() conditional
> on disk being non-NULL? I guess we'd have to hold the ublk_device
> mutex before calling ublk_get_disk() to prevent it from being assigned
> concurrently.
Here `disk` is used for freeze & quiesce queue.
But the implementation can be a bit more complicated given the dependency
between ub->mutex and freeze queue should be avoided.
Anyway, it is one nice requirement, I will try to relax the constraint.
>
> > +
> > + /* Pin pages before quiescing (may sleep) */
> > + ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
> > + if (!ubuf) {
> > + ret = -ENOMEM;
> > + goto put_disk;
> > + }
> > +
> > + ubuf->pages = kvmalloc_array(nr_pages, sizeof(*ubuf->pages),
> > + GFP_KERNEL);
> > + if (!ubuf->pages) {
> > + ret = -ENOMEM;
> > + goto err_free;
> > + }
> > +
> > + gup_flags = FOLL_LONGTERM;
> > + if (!(buf_reg.flags & UBLK_SHMEM_BUF_READ_ONLY))
> > + gup_flags |= FOLL_WRITE;
> > +
> > + pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, ubuf->pages);
> > + if (pinned < 0) {
> > + ret = pinned;
> > + goto err_free_pages;
> > + }
> > + if (pinned != nr_pages) {
> > + ret = -EFAULT;
> > + goto err_unpin;
> > + }
> > + ubuf->nr_pages = nr_pages;
> > +
> > + /*
> > + * Drain inflight I/O and quiesce the queue so no new requests
> > + * are dispatched while we modify the maple tree. Keep freeze
> > + * and mutex non-nested to avoid lock dependency.
> > + */
> > + ublk_quiesce_and_release(disk);
> > +
> > + mutex_lock(&ub->mutex);
>
> Looks like the xarray and maple tree do their own spinlocking, is this needed?
Right, looks it isn't needed now, and it was added from beginning with plain
array.
>
> > +
> > + ret = xa_alloc(&ub->bufs_xa, &index, ubuf, xa_limit_16b, GFP_KERNEL);
> > + if (ret)
> > + goto err_unlock;
> > +
> > + ret = __ublk_ctrl_reg_buf(ub, ubuf, index, buf_reg.flags);
> > + if (ret) {
> > + xa_erase(&ub->bufs_xa, index);
> > + goto err_unlock;
> > + }
> > +
> > + mutex_unlock(&ub->mutex);
> > +
> > + ublk_unquiesce_and_resume(disk);
> > + ublk_put_disk(disk);
> > + return index;
> > +
> > +err_unlock:
> > + mutex_unlock(&ub->mutex);
> > + ublk_unquiesce_and_resume(disk);
> > +err_unpin:
> > + unpin_user_pages(ubuf->pages, pinned);
> > +err_free_pages:
> > + kvfree(ubuf->pages);
> > +err_free:
> > + kfree(ubuf);
> > +put_disk:
> > + ublk_put_disk(disk);
> > + return ret;
> > +}
> > +
> > +static void __ublk_ctrl_unreg_buf(struct ublk_device *ub,
> > + struct ublk_buf *ubuf)
> > +{
> > + ublk_buf_erase_ranges(ub, ubuf, ubuf->nr_pages);
> > + unpin_user_pages(ubuf->pages, ubuf->nr_pages);
> > + kvfree(ubuf->pages);
> > + kfree(ubuf);
> > +}
> > +
> > +static int ublk_ctrl_unreg_buf(struct ublk_device *ub,
> > + struct ublksrv_ctrl_cmd *header)
> > +{
> > + int index = (int)header->data[0];
> > + struct gendisk *disk;
> > + struct ublk_buf *ubuf;
> > +
> > + if (!ublk_dev_support_shmem_zc(ub))
> > + return -EOPNOTSUPP;
> > +
> > + disk = ublk_get_disk(ub);
> > + if (!disk)
> > + return -ENODEV;
> > +
> > + /* Drain inflight I/O before modifying the maple tree */
> > + ublk_quiesce_and_release(disk);
> > +
> > + mutex_lock(&ub->mutex);
> > +
> > + ubuf = xa_erase(&ub->bufs_xa, index);
> > + if (!ubuf) {
> > + mutex_unlock(&ub->mutex);
> > + ublk_unquiesce_and_resume(disk);
> > + ublk_put_disk(disk);
> > + return -ENOENT;
> > + }
> > +
> > + __ublk_ctrl_unreg_buf(ub, ubuf);
> > +
> > + mutex_unlock(&ub->mutex);
> > +
> > + ublk_unquiesce_and_resume(disk);
> > + ublk_put_disk(disk);
> > + return 0;
> > +}
> > +
> > +static void ublk_buf_cleanup(struct ublk_device *ub)
> > +{
> > + struct ublk_buf *ubuf;
> > + unsigned long index;
> > +
> > + xa_for_each(&ub->bufs_xa, index, ubuf)
> > + __ublk_ctrl_unreg_buf(ub, ubuf);
>
> This looks quadratic in the number of registered buffers. Can we do a
> single pass over the xarray and the maple tree?
It can be done two passes: first pass walks maple tree for unpinning
pages, the 2nd pass is for freeing buffers in xarray, which looks more
clean.
But the current way can reuse __ublk_ctrl_unreg_buf(), also
ublk_buf_cleanup() is only called one-shot in device release handler, so we can
leave it for future optimization.
>
> > + xa_destroy(&ub->bufs_xa);
> > + mtree_destroy(&ub->buf_tree);
> > +}
> > +
> > +
> > +
> > static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> > u32 cmd_op, struct ublksrv_ctrl_cmd *header)
> > {
> > @@ -5225,6 +5517,8 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> > case UBLK_CMD_UPDATE_SIZE:
> > case UBLK_CMD_QUIESCE_DEV:
> > case UBLK_CMD_TRY_STOP_DEV:
> > + case UBLK_CMD_REG_BUF:
> > + case UBLK_CMD_UNREG_BUF:
> > mask = MAY_READ | MAY_WRITE;
> > break;
> > default:
> > @@ -5350,6 +5644,12 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
> > case UBLK_CMD_TRY_STOP_DEV:
> > ret = ublk_ctrl_try_stop_dev(ub);
> > break;
> > + case UBLK_CMD_REG_BUF:
> > + ret = ublk_ctrl_reg_buf(ub, &header);
> > + break;
> > + case UBLK_CMD_UNREG_BUF:
> > + ret = ublk_ctrl_unreg_buf(ub, &header);
> > + break;
> > default:
> > ret = -EOPNOTSUPP;
> > break;
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index a88876756805..52bb9b843d73 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -57,6 +57,44 @@
> > _IOWR('u', 0x16, struct ublksrv_ctrl_cmd)
> > #define UBLK_U_CMD_TRY_STOP_DEV \
> > _IOWR('u', 0x17, struct ublksrv_ctrl_cmd)
> > +/*
> > + * Register a shared memory buffer for zero-copy I/O.
> > + * Input: ctrl_cmd.addr points to struct ublk_buf_reg (buffer VA + size)
> > + * ctrl_cmd.len = sizeof(struct ublk_buf_reg)
> > + * Result: >= 0 is the assigned buffer index, < 0 is error
> > + *
> > + * The kernel pins pages from the calling process's address space
> > + * and inserts PFN ranges into a per-device maple tree. When a block
> > + * request's pages match registered pages, the driver sets
> > + * UBLK_IO_F_SHMEM_ZC and encodes the buffer index + offset in addr,
> > + * allowing the server to access the data via its own mapping of the
> > + * same shared memory — true zero copy.
> > + *
> > + * The memory can be backed by memfd, hugetlbfs, or any GUP-compatible
> > + * shared mapping. Queue freeze is handled internally.
> > + *
> > + * The buffer VA and size are passed via a user buffer (not inline in
> > + * ctrl_cmd) so that unprivileged devices can prepend the device path
> > + * to ctrl_cmd.addr without corrupting the VA.
> > + */
> > +#define UBLK_U_CMD_REG_BUF \
> > + _IOWR('u', 0x18, struct ublksrv_ctrl_cmd)
> > +/*
> > + * Unregister a shared memory buffer.
> > + * Input: ctrl_cmd.data[0] = buffer index
> > + */
> > +#define UBLK_U_CMD_UNREG_BUF \
> > + _IOWR('u', 0x19, struct ublksrv_ctrl_cmd)
> > +
> > +/* Parameter buffer for UBLK_U_CMD_REG_BUF, pointed to by ctrl_cmd.addr */
> > +struct ublk_shmem_buf_reg {
> > + __u64 addr; /* userspace virtual address of shared memory */
> > + __u32 len; /* buffer size in bytes (page-aligned, max 4GB) */
> > + __u32 flags;
> > +};
> > +
> > +/* Pin pages without FOLL_WRITE; usable with write-sealed memfd */
> > +#define UBLK_SHMEM_BUF_READ_ONLY (1U << 0)
> > /*
> > * 64bits are enough now, and it should be easy to extend in case of
> > * running out of feature flags
> > @@ -370,6 +408,7 @@
> > /* Disable automatic partition scanning when device is started */
> > #define UBLK_F_NO_AUTO_PART_SCAN (1ULL << 18)
> >
> > +
> > /* device state */
> > #define UBLK_S_DEV_DEAD 0
> > #define UBLK_S_DEV_LIVE 1
> > @@ -469,6 +508,12 @@ struct ublksrv_ctrl_dev_info {
> > #define UBLK_IO_F_NEED_REG_BUF (1U << 17)
> > /* Request has an integrity data buffer */
> > #define UBLK_IO_F_INTEGRITY (1UL << 18)
> > +/*
> > + * I/O buffer is in a registered shared memory buffer. When set, the addr
> > + * field in ublksrv_io_desc encodes buffer index and byte offset instead
> > + * of a userspace virtual address.
> > + */
> > +#define UBLK_IO_F_SHMEM_ZC (1U << 19)
> >
> > /*
> > * io cmd is described by this structure, and stored in share memory, indexed
> > @@ -743,4 +788,31 @@ struct ublk_params {
> > struct ublk_param_integrity integrity;
> > };
> >
> > +/*
> > + * Shared memory zero-copy addr encoding for UBLK_IO_F_SHMEM_ZC.
> > + *
> > + * When UBLK_IO_F_SHMEM_ZC is set, ublksrv_io_desc.addr is encoded as:
> > + * bits [0:31] = byte offset within the buffer (up to 4GB)
> > + * bits [32:47] = buffer index (up to 65536)
> > + * bits [48:63] = reserved (must be zero)
>
> I wonder whether the "buffer index" is necessary. Can iod->addr and
> UBLK_U_CMD_UNREG_BUF refer to the buffer by the virtual address used
> with UBLK_U_CMD_REG_BUF? Then struct ublksrv_io_desc's addr field
> would retain its meaning. We would also avoid needing to compare the
> range buf_index values in ublk_try_buf_match(). And the xarray
> wouldn't be necessary to allocate buffer indices.
There are several reasons for choosing "buffer index":
- avoid to add extra storage in `struct ublk_buf_range`, extra
`vaddr` field is required for returning virtual address; otherwise one extra
lookup from buffer index is needed in fast path of ublk_try_buf_match()
- I want ublk server to know the difference between shmem_zc buffer and the
plain IO buffer clearly, both two shouldn't be mixed, otherwise it is easy to
cause data corruption. For example, client is using buf A, but the
ublk server fallback code path may be using it at the same time.
- total registered buffers can be limited naturally by `u16` buffer_index
- it is proved that buffer index is one nice pattern wrt. buffer
registration, such as io_uring fixed buffer; and it helps userspace
to manage multiple buffers, given each one has unique ID.
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH v2 04/10] ublk: eliminate permanent pages[] array from struct ublk_buf
From: Caleb Sander Mateos @ 2026-04-07 19:50 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block
In-Reply-To: <20260331153207.3635125-5-ming.lei@redhat.com>
On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> The pages[] array (kvmalloc'd, 8 bytes per page = 2MB for a 1GB buffer)
> was stored permanently in struct ublk_buf but only needed during
> pin_user_pages_fast() and maple tree construction. Since the maple tree
> already stores PFN ranges via ublk_buf_range, struct page pointers can
> be recovered via pfn_to_page() during unregistration.
>
> Make pages[] a temporary allocation in ublk_ctrl_reg_buf(), freed
> immediately after the maple tree is built. Rewrite __ublk_ctrl_unreg_buf()
> to iterate the maple tree for matching buf_index entries, recovering
> struct page pointers via pfn_to_page() and unpinning in batches of 32.
> Simplify ublk_buf_erase_ranges() to iterate the maple tree by buf_index
> instead of walking the now-removed pages[] array.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/block/ublk_drv.c | 87 +++++++++++++++++++++++++---------------
> 1 file changed, 55 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index c2b9992503a4..2e475bdc54dd 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -296,7 +296,6 @@ struct ublk_queue {
>
> /* Per-registered shared memory buffer */
> struct ublk_buf {
> - struct page **pages;
> unsigned int nr_pages;
> };
It looks like nr_pages doesn't need to be stored either, it could just
be passed to __ublk_ctrl_reg_buf(). Then I think we could get rid of
struct ublk_buf and the xarray entirely. We really just need a bitmap
for allocating buffer indices.
>
> @@ -5261,27 +5260,25 @@ static void ublk_unquiesce_and_resume(struct gendisk *disk)
> * coalescing consecutive PFNs into single range entries.
> * Returns 0 on success, negative error with partial insertions unwound.
> */
> -/* Erase coalesced PFN ranges from the maple tree for pages [0, nr_pages) */
> -static void ublk_buf_erase_ranges(struct ublk_device *ub,
> - struct ublk_buf *ubuf,
> - unsigned long nr_pages)
> +/* Erase coalesced PFN ranges from the maple tree matching buf_index */
> +static void ublk_buf_erase_ranges(struct ublk_device *ub, int buf_index)
> {
> - unsigned long i;
> -
> - for (i = 0; i < nr_pages; ) {
> - unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> - unsigned long start = i;
> + MA_STATE(mas, &ub->buf_tree, 0, ULONG_MAX);
> + struct ublk_buf_range *range;
>
> - while (i + 1 < nr_pages &&
> - page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> - i++;
> - i++;
> - kfree(mtree_erase(&ub->buf_tree, pfn));
> + mas_lock(&mas);
> + mas_for_each(&mas, range, ULONG_MAX) {
> + if (range->buf_index == buf_index) {
> + mas_erase(&mas);
> + kfree(range);
> + }
> }
> + mas_unlock(&mas);
> }
>
> static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> - struct ublk_buf *ubuf, int index,
> + struct ublk_buf *ubuf,
> + struct page **pages, int index,
> unsigned short flags)
> {
> unsigned long nr_pages = ubuf->nr_pages;
> @@ -5289,13 +5286,13 @@ static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> int ret;
>
> for (i = 0; i < nr_pages; ) {
> - unsigned long pfn = page_to_pfn(ubuf->pages[i]);
> + unsigned long pfn = page_to_pfn(pages[i]);
> unsigned long start = i;
> struct ublk_buf_range *range;
>
> /* Find run of consecutive PFNs */
> while (i + 1 < nr_pages &&
> - page_to_pfn(ubuf->pages[i + 1]) == pfn + (i - start) + 1)
> + page_to_pfn(pages[i + 1]) == pfn + (i - start) + 1)
> i++;
> i++; /* past the last page in this run */
>
> @@ -5320,7 +5317,7 @@ static int __ublk_ctrl_reg_buf(struct ublk_device *ub,
> return 0;
>
> unwind:
> - ublk_buf_erase_ranges(ub, ubuf, i);
> + ublk_buf_erase_ranges(ub, index);
> return ret;
> }
>
> @@ -5335,6 +5332,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> void __user *argp = (void __user *)(unsigned long)header->addr;
> struct ublk_shmem_buf_reg buf_reg;
> unsigned long addr, size, nr_pages;
> + struct page **pages = NULL;
> unsigned int gup_flags;
> struct gendisk *disk;
> struct ublk_buf *ubuf;
> @@ -5371,9 +5369,8 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> goto put_disk;
> }
>
> - ubuf->pages = kvmalloc_array(nr_pages, sizeof(*ubuf->pages),
> - GFP_KERNEL);
> - if (!ubuf->pages) {
> + pages = kvmalloc_array(nr_pages, sizeof(*pages), GFP_KERNEL);
> + if (!pages) {
> ret = -ENOMEM;
> goto err_free;
> }
> @@ -5382,7 +5379,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> if (!(buf_reg.flags & UBLK_SHMEM_BUF_READ_ONLY))
> gup_flags |= FOLL_WRITE;
>
> - pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, ubuf->pages);
> + pinned = pin_user_pages_fast(addr, nr_pages, gup_flags, pages);
> if (pinned < 0) {
> ret = pinned;
> goto err_free_pages;
> @@ -5406,7 +5403,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> if (ret)
> goto err_unlock;
>
> - ret = __ublk_ctrl_reg_buf(ub, ubuf, index, buf_reg.flags);
> + ret = __ublk_ctrl_reg_buf(ub, ubuf, pages, index, buf_reg.flags);
> if (ret) {
> xa_erase(&ub->bufs_xa, index);
> goto err_unlock;
> @@ -5414,6 +5411,7 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
>
> mutex_unlock(&ub->mutex);
>
> + kvfree(pages);
> ublk_unquiesce_and_resume(disk);
> ublk_put_disk(disk);
> return index;
> @@ -5422,9 +5420,9 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> mutex_unlock(&ub->mutex);
> ublk_unquiesce_and_resume(disk);
> err_unpin:
> - unpin_user_pages(ubuf->pages, pinned);
> + unpin_user_pages(pages, pinned);
> err_free_pages:
> - kvfree(ubuf->pages);
> + kvfree(pages);
> err_free:
> kfree(ubuf);
> put_disk:
> @@ -5433,11 +5431,36 @@ static int ublk_ctrl_reg_buf(struct ublk_device *ub,
> }
>
> static void __ublk_ctrl_unreg_buf(struct ublk_device *ub,
> - struct ublk_buf *ubuf)
> + struct ublk_buf *ubuf, int buf_index)
ubuf is only passed to kfree() now, maybe it would make sense to move
that to the caller so the argument can be dropped?
Best,
Caleb
> {
> - ublk_buf_erase_ranges(ub, ubuf, ubuf->nr_pages);
> - unpin_user_pages(ubuf->pages, ubuf->nr_pages);
> - kvfree(ubuf->pages);
> + MA_STATE(mas, &ub->buf_tree, 0, ULONG_MAX);
> + struct ublk_buf_range *range;
> + struct page *pages[32];
> +
> + mas_lock(&mas);
> + mas_for_each(&mas, range, ULONG_MAX) {
> + unsigned long base, nr, off;
> +
> + if (range->buf_index != buf_index)
> + continue;
> +
> + base = range->base_pfn;
> + nr = mas.last - mas.index + 1;
> + mas_erase(&mas);
> +
> + for (off = 0; off < nr; ) {
> + unsigned int batch = min_t(unsigned long,
> + nr - off, 32);
> + unsigned int j;
> +
> + for (j = 0; j < batch; j++)
> + pages[j] = pfn_to_page(base + off + j);
> + unpin_user_pages(pages, batch);
> + off += batch;
> + }
> + kfree(range);
> + }
> + mas_unlock(&mas);
> kfree(ubuf);
> }
>
> @@ -5468,7 +5491,7 @@ static int ublk_ctrl_unreg_buf(struct ublk_device *ub,
> return -ENOENT;
> }
>
> - __ublk_ctrl_unreg_buf(ub, ubuf);
> + __ublk_ctrl_unreg_buf(ub, ubuf, index);
>
> mutex_unlock(&ub->mutex);
>
> @@ -5483,7 +5506,7 @@ static void ublk_buf_cleanup(struct ublk_device *ub)
> unsigned long index;
>
> xa_for_each(&ub->bufs_xa, index, ubuf)
> - __ublk_ctrl_unreg_buf(ub, ubuf);
> + __ublk_ctrl_unreg_buf(ub, ubuf, index);
> xa_destroy(&ub->bufs_xa);
> mtree_destroy(&ub->buf_tree);
> }
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH v2 03/10] ublk: enable UBLK_F_SHMEM_ZC feature flag
From: Caleb Sander Mateos @ 2026-04-07 19:47 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block
In-Reply-To: <20260331153207.3635125-4-ming.lei@redhat.com>
On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Add UBLK_F_SHMEM_ZC (1ULL << 19) to the UAPI header and UBLK_F_ALL.
> Switch ublk_support_shmem_zc() and ublk_dev_support_shmem_zc() from
> returning false to checking the actual flag, enabling the shared
> memory zero-copy feature for devices that request it.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> Documentation/block/ublk.rst | 117 ++++++++++++++++++++++++++++++++++
> drivers/block/ublk_drv.c | 7 +-
> include/uapi/linux/ublk_cmd.h | 7 ++
> 3 files changed, 128 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/block/ublk.rst b/Documentation/block/ublk.rst
> index 6ad28039663d..a818e09a4b66 100644
> --- a/Documentation/block/ublk.rst
> +++ b/Documentation/block/ublk.rst
> @@ -485,6 +485,123 @@ Limitations
> in case that too many ublk devices are handled by this single io_ring_ctx
> and each one has very large queue depth
>
> +Shared Memory Zero Copy (UBLK_F_SHMEM_ZC)
> +------------------------------------------
> +
> +The ``UBLK_F_SHMEM_ZC`` feature provides an alternative zero-copy path
> +that works by sharing physical memory pages between the client application
> +and the ublk server. Unlike the io_uring fixed buffer approach above,
> +shared memory zero copy does not require io_uring buffer registration
> +per I/O — instead, it relies on the kernel matching page frame numbers
> +(PFNs) at I/O time. This allows the ublk server to access the shared
Maybe "physical pages" would be clearer than the kernel-internal
concept of "page frame numbers"?
> +buffer directly, which is unlikely for the io_uring fixed buffer
> +approach.
> +
> +Motivation
> +~~~~~~~~~~
> +
> +Shared memory zero copy takes a different approach: if the client
> +application and the ublk server both map the same physical memory, there is
> +nothing to copy. The kernel detects the shared pages automatically and
> +tells the server where the data already lives.
> +
> +``UBLK_F_SHMEM_ZC`` can be thought of as a supplement for optimized client
> +applications — when the client is willing to allocate I/O buffers from
> +shared memory, the entire data path becomes zero-copy without any per-I/O
> +overhead.
nit: The shmem buffer lookup still has some overhead. I think just
"becomes zero-copy" would be fine.
> +
> +Use Cases
> +~~~~~~~~~
> +
> +This feature is useful when the client application can be configured to
> +use a specific shared memory region for its I/O buffers:
> +
> +- **Custom storage clients** that allocate I/O buffers from shared memory
> + (memfd, hugetlbfs) and issue direct I/O to the ublk device
> +- **Database engines** that use pre-allocated buffer pools with O_DIRECT
> +
> +How It Works
> +~~~~~~~~~~~~
> +
> +1. The ublk server and client both ``mmap()`` the same file (memfd or
> + hugetlbfs) with ``MAP_SHARED``. This gives both processes access to the
> + same physical pages.
> +
> +2. The ublk server registers its mapping with the kernel::
> +
> + struct ublk_buf_reg buf = { .addr = mmap_va, .len = size };
> + ublk_ctrl_cmd(UBLK_U_CMD_REG_BUF, .addr = &buf);
This doesn't look like valid C syntax. Maybe it could say something like:
struct ublksrv_ctrl_cmd cmd = {.dev_id = dev_id, .addr = &buf, .len =
sizeof(buf)};
io_uring_prep_uring_cmd(sqe, UBLK_U_CMD_REG_BUF, ublk_control_fd);
memcpy(sqe->cmd, &cmd, sizeof(cmd));
> +
> + The kernel pins the pages and builds a PFN lookup tree.
> +
> +3. When the client issues direct I/O (``O_DIRECT``) to ``/dev/ublkb*``,
> + the kernel checks whether the I/O buffer pages match any registered
> + pages by comparing PFNs.
> +
> +4. On a match, the kernel sets ``UBLK_IO_F_SHMEM_ZC`` in the I/O
> + descriptor and encodes the buffer index and offset in ``addr``::
> +
> + if (iod->op_flags & UBLK_IO_F_SHMEM_ZC) {
> + /* Data is already in our shared mapping — zero copy */
> + index = ublk_shmem_zc_index(iod->addr);
> + offset = ublk_shmem_zc_offset(iod->addr);
> + buf = shmem_table[index].mmap_base + offset;
> + }
> +
> +5. If pages do not match (e.g., the client used a non-shared buffer),
> + the I/O falls back to the normal copy path silently.
> +
> +The shared memory can be set up via two methods:
> +
> +- **Socket-based**: the client sends a memfd to the ublk server via
> + ``SCM_RIGHTS`` on a unix socket. The server mmaps and registers it.
> +- **Hugetlbfs-based**: both processes ``mmap(MAP_SHARED)`` the same
> + hugetlbfs file. No IPC needed — same file gives same physical pages.
> +
> +Advantages
> +~~~~~~~~~~
> +
> +- **Simple**: no per-I/O buffer registration or unregistration commands.
> + Once the shared buffer is registered, all matching I/O is zero-copy
> + automatically.
> +- **Direct buffer access**: the ublk server can read and write the shared
> + buffer directly via its own mmap, without going through io_uring fixed
> + buffer operations. This is more friendly for server implementations.
> +- **Fast**: PFN matching is a single maple tree lookup per bvec. No
> + io_uring command round-trips for buffer management.
> +- **Compatible**: non-matching I/O silently falls back to the copy path.
> + The device works normally for any client, with zero-copy as an
> + optimization when shared memory is available.
> +
> +Limitations
> +~~~~~~~~~~~
> +
> +- **Requires client cooperation**: the client must allocate its I/O
> + buffers from the shared memory region. This requires a custom or
> + configured client — standard applications using their own buffers
> + will not benefit.
> +- **Direct I/O only**: buffered I/O (without ``O_DIRECT``) goes through
> + the page cache, which allocates its own pages. These kernel-allocated
> + pages will never match the registered shared buffer. Only ``O_DIRECT``
> + puts the client's buffer pages directly into the block I/O.
One other limitation that might be worth mentioning is that
scatter/gather I/O can't use the SHMEM_ZC optimization, as the
request's data must be contiguous in the registered virtual address
range.
Best,
Caleb
> +
> +Control Commands
> +~~~~~~~~~~~~~~~~
> +
> +- ``UBLK_U_CMD_REG_BUF``
> +
> + Register a shared memory buffer. ``ctrl_cmd.addr`` points to a
> + ``struct ublk_buf_reg`` containing the buffer virtual address and size.
> + Returns the assigned buffer index (>= 0) on success. The kernel pins
> + pages and builds the PFN lookup tree. Queue freeze is handled
> + internally.
> +
> +- ``UBLK_U_CMD_UNREG_BUF``
> +
> + Unregister a previously registered buffer. ``ctrl_cmd.data[0]`` is the
> + buffer index. Unpins pages and removes PFN entries from the lookup
> + tree.
> +
> References
> ==========
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index d53865437600..c2b9992503a4 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -85,7 +85,8 @@
> | (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) ? UBLK_F_INTEGRITY : 0) \
> | UBLK_F_SAFE_STOP_DEV \
> | UBLK_F_BATCH_IO \
> - | UBLK_F_NO_AUTO_PART_SCAN)
> + | UBLK_F_NO_AUTO_PART_SCAN \
> + | UBLK_F_SHMEM_ZC)
>
> #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> | UBLK_F_USER_RECOVERY_REISSUE \
> @@ -425,7 +426,7 @@ static inline bool ublk_dev_support_zero_copy(const struct ublk_device *ub)
>
> static inline bool ublk_support_shmem_zc(const struct ublk_queue *ubq)
> {
> - return false;
> + return ubq->flags & UBLK_F_SHMEM_ZC;
> }
>
> static inline bool ublk_iod_is_shmem_zc(const struct ublk_queue *ubq,
> @@ -436,7 +437,7 @@ static inline bool ublk_iod_is_shmem_zc(const struct ublk_queue *ubq,
>
> static inline bool ublk_dev_support_shmem_zc(const struct ublk_device *ub)
> {
> - return false;
> + return ub->dev_info.flags & UBLK_F_SHMEM_ZC;
> }
>
> static inline bool ublk_support_auto_buf_reg(const struct ublk_queue *ubq)
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 52bb9b843d73..ecd258847d3d 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -408,6 +408,13 @@ struct ublk_shmem_buf_reg {
> /* Disable automatic partition scanning when device is started */
> #define UBLK_F_NO_AUTO_PART_SCAN (1ULL << 18)
>
> +/*
> + * Enable shared memory zero copy. When enabled, the server can register
> + * shared memory buffers via UBLK_U_CMD_REG_BUF. If a block request's
> + * pages match a registered buffer, UBLK_IO_F_SHMEM_ZC is set and addr
> + * encodes the buffer index + offset instead of a userspace buffer address.
> + */
> +#define UBLK_F_SHMEM_ZC (1ULL << 19)
>
> /* device state */
> #define UBLK_S_DEV_DEAD 0
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH v2 02/10] ublk: add PFN-based buffer matching in I/O path
From: Caleb Sander Mateos @ 2026-04-07 19:47 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block
In-Reply-To: <20260331153207.3635125-3-ming.lei@redhat.com>
On Tue, Mar 31, 2026 at 8:32 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Add ublk_try_buf_match() which walks a request's bio_vecs, looks up
> each page's PFN in the per-device maple tree, and verifies all pages
> belong to the same registered buffer at contiguous offsets.
>
> Add ublk_iod_is_shmem_zc() inline helper for checking whether a
> request uses the shmem zero-copy path.
>
> Integrate into the I/O path:
> - ublk_setup_iod(): if pages match a registered buffer, set
> UBLK_IO_F_SHMEM_ZC and encode buffer index + offset in addr
> - ublk_start_io(): skip ublk_map_io() for zero-copy requests
> - __ublk_complete_rq(): skip ublk_unmap_io() for zero-copy requests
>
> The feature remains disabled (ublk_support_shmem_zc() returns false)
> until the UBLK_F_SHMEM_ZC flag is enabled in the next patch.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/block/ublk_drv.c | 77 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index ac6ccc174d44..d53865437600 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -356,6 +356,8 @@ struct ublk_params_header {
>
> static void ublk_io_release(void *priv);
> static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> +static bool ublk_try_buf_match(struct ublk_device *ub, struct request *rq,
> + u32 *buf_idx, u32 *buf_off);
buf_idx could be a u16 * for consistency?
> static void ublk_buf_cleanup(struct ublk_device *ub);
> static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> @@ -426,6 +428,12 @@ static inline bool ublk_support_shmem_zc(const struct ublk_queue *ubq)
> return false;
> }
>
> +static inline bool ublk_iod_is_shmem_zc(const struct ublk_queue *ubq,
> + unsigned int tag)
> +{
> + return ublk_get_iod(ubq, tag)->op_flags & UBLK_IO_F_SHMEM_ZC;
> +}
> +
> static inline bool ublk_dev_support_shmem_zc(const struct ublk_device *ub)
> {
> return false;
> @@ -1494,6 +1502,18 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> iod->nr_sectors = blk_rq_sectors(req);
> iod->start_sector = blk_rq_pos(req);
>
> + /* Try shmem zero-copy match before setting addr */
> + if (ublk_support_shmem_zc(ubq) && ublk_rq_has_data(req)) {
> + u32 buf_idx, buf_off;
> +
> + if (ublk_try_buf_match(ubq->dev, req,
> + &buf_idx, &buf_off)) {
> + iod->op_flags |= UBLK_IO_F_SHMEM_ZC;
> + iod->addr = ublk_shmem_zc_addr(buf_idx, buf_off);
> + return BLK_STS_OK;
> + }
> + }
> +
> iod->addr = io->buf.addr;
>
> return BLK_STS_OK;
> @@ -1539,6 +1559,10 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
> req_op(req) != REQ_OP_DRV_IN)
> goto exit;
>
> + /* shmem zero copy: no data to unmap, pages already shared */
> + if (ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))
This is a lot of pointer chasing. Could we track this with a flag on
struct ublk_io instead?
> + goto exit;
> +
> /* for READ request, writing data in iod->addr to rq buffers */
> unmapped_bytes = ublk_unmap_io(need_map, req, io);
>
> @@ -1697,8 +1721,13 @@ static void ublk_auto_buf_dispatch(const struct ublk_queue *ubq,
> static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> struct ublk_io *io)
> {
> - unsigned mapped_bytes = ublk_map_io(ubq, req, io);
> + unsigned mapped_bytes;
>
> + /* shmem zero copy: skip data copy, pages already shared */
> + if (ublk_iod_is_shmem_zc(ubq, req->tag))
> + return true;
> +
> + mapped_bytes = ublk_map_io(ubq, req, io);
>
> /* partially mapped, update io descriptor */
> if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
> @@ -5458,7 +5487,53 @@ static void ublk_buf_cleanup(struct ublk_device *ub)
> mtree_destroy(&ub->buf_tree);
> }
>
> +/* Check if request pages match a registered shared memory buffer */
> +static bool ublk_try_buf_match(struct ublk_device *ub,
> + struct request *rq,
> + u32 *buf_idx, u32 *buf_off)
> +{
> + struct req_iterator iter;
> + struct bio_vec bv;
> + int index = -1;
> + unsigned long expected_offset = 0;
> + bool first = true;
Could check index < 0 in place of first?
> +
> + rq_for_each_bvec(bv, rq, iter) {
> + unsigned long pfn = page_to_pfn(bv.bv_page);
> + struct ublk_buf_range *range;
> + unsigned long off;
>
> + range = mtree_load(&ub->buf_tree, pfn);
> + if (!range)
> + return false;
> +
> + off = range->base_offset +
> + (pfn - range->base_pfn) * PAGE_SIZE + bv.bv_offset;
Doesn't this need to check that the end of the bvec is less than the
end of the range? Otherwise, the bvec could extend into physically
contiguous pages that aren't part of the registered range.
Also, the range could precompute base_pfn - base_offset / PAGE_SIZE
instead of base_offset to make this a bit cheaper.
> +
> + if (first) {
> + /* Read-only buffer can't serve READ (kernel writes) */
> + if ((range->flags & UBLK_SHMEM_BUF_READ_ONLY) &&
> + req_op(rq) != REQ_OP_WRITE)
> + return false;
> + index = range->buf_index;
> + expected_offset = off;
> + *buf_off = off;
> + first = false;
> + } else {
> + if (range->buf_index != index)
> + return false;
> + if (off != expected_offset)
> + return false;
> + }
> + expected_offset += bv.bv_len;
> + }
> +
> + if (first)
> + return false;
How is this case possible? That would mean the request has no bvecs,
but ublk_try_buf_match() is only called for requests with data, right?
Best,
Caleb
> +
> + *buf_idx = index;
> + return true;
> +}
>
> static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> u32 cmd_op, struct ublksrv_ctrl_cmd *header)
> --
> 2.53.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox