* Re: [PATCH v3 02/12] block/bdev: Annotate the blk_holder_ops callback invocations
From: Christoph Hellwig @ 2026-04-09 6:42 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Damien Le Moal,
Marco Elver, Nathan Chancellor
In-Reply-To: <20260402183950.3626956-3-bvanassche@acm.org>
On Thu, Apr 02, 2026 at 11:39:34AM -0700, Bart Van Assche wrote:
> The four callback functions in blk_holder_ops all release the
> bd_holder_lock. Add __release() annotations where appropriate to prepare
> for enabling thread-safety analysis. Explicit __release() annotations have
> been added since Clang does not support adding a __releases() annotation
> to a function pointer.
I have to say I still hate this with passion. If we can't propagate
the lock context over function pointers it isn't ready for prime time
unfortunately, as much as I'm looking forward to it.
^ permalink raw reply
* Re: [PATCH 00/20] DRBD 9 rework
From: Christoph Hellwig @ 2026-04-09 6:40 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Christoph Böhmwalder, drbd-dev,
linux-kernel, Lars Ellenberg, Philipp Reisner, linux-block
In-Reply-To: <c92d5318-8c8e-434f-835e-61cf1fae4edf@kernel.dk>
First an apology, I thought it was in your tree, but it looks like
the drbd branch just has minor fixes. So a lot less urgency.
On Wed, Apr 08, 2026 at 06:58:58AM -0600, Jens Axboe wrote:
> See the previous discussion,
Do you have a pointer to that discussion? I can't remember one.
> 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.
I don't think that's a a usual pattern. Also the new version looks
like a complete rewrite and not something incremental:
45 files changed, 45891 insertions(+), 16264 deletions(-)
For a code base that is "29482 total".
I think reviewing it would be easier by just adding an new drbd9 driver
and then steering people toward it carefully, as that is actually
reviewable compared to non-bisectable patches changing large chunks
of code in a non-atomic way.
^ permalink raw reply
* Re: [PATCH 07/13] libmultipath: Add delayed removal support
From: Nilay Shroff @ 2026-04-09 6:37 UTC (permalink / raw)
To: John Garry, Hannes Reinecke, 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: <c5334a6b-8089-4ee5-abd3-8340133db29a@oracle.com>
On 4/8/26 9:58 PM, John Garry wrote:
> On 08/04/2026 16:41, Hannes Reinecke wrote:
>> On 4/8/26 13:28, John Garry wrote:
>>> 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).
>>>
>>
>> That look patently like the 'queue_if_no_path' feature from dm- multipath. Any chance of reconciling these two?
>
> You mean a common blktests testcase, right?
>
> For NVMe, that test would:
> a. try to remove NVMe ko when we have the delayed removal active
> b. ensure that we can queue for no path
>
> I suppose that a common testcase could be possible (with dm mpath), but doesn't dm have its own testsuite?
>
Yes, I'd add a blktest for 'queue_if_no_path' feature. But as we know we have
separate test suite for dm under blktests, I'd first target nvme testcase and
then later add another testcase for dm-multipath.
Thanks,
--Nilay
^ permalink raw reply
* [PATCH 5.15.y] block: fix resource leak in blk_register_queue() error path
From: Robert Garcia @ 2026-04-09 6:36 UTC (permalink / raw)
To: stable, Zheng Qixing
Cc: Jens Axboe, Robert Garcia, Christoph Hellwig, Yu Kuai,
linux-block, linux-kernel
From: Zheng Qixing <zhengqixing@huawei.com>
[ Upstream commit 40f2eb9b531475dd01b683fdaf61ca3cfd03a51e ]
When registering a queue fails after blk_mq_sysfs_register() is
successful but the function later encounters an error, we need
to clean up the blk_mq_sysfs resources.
Add the missing blk_mq_sysfs_unregister() call in the error path
to properly clean up these resources and prevent a memory leak.
Fixes: 320ae51feed5 ("blk-mq: new multi-queue block IO queueing mechanism")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Link: https://lore.kernel.org/r/20250412092554.475218-1-zhengqixing@huaweicloud.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
[ Change disk to q in blk_mq_sysfs_unregister(). ]
Signed-off-by: Robert Garcia <rob_garcia@163.com>
---
block/blk-sysfs.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 4ea84e46a665..c1917992e619 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -900,6 +900,8 @@ int blk_register_queue(struct gendisk *disk)
if (q->elevator)
kobject_uevent(&q->elevator->kobj, KOBJ_ADD);
mutex_unlock(&q->sysfs_lock);
+ if (queue_is_mq(q))
+ blk_mq_sysfs_unregister(q);
ret = 0;
unlock:
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 3/3] scsi: align scsi_device iodone_cnt to avoid cache line contention
From: Sumit Saxena @ 2026-04-09 6:17 UTC (permalink / raw)
To: Bart Van Assche
Cc: martin.petersen, axboe, linux-scsi, linux-block,
mpi3mr-linuxdrv.pdl, James Rizzo
In-Reply-To: <fd582bae-a090-48d4-a4eb-b2db6c10c26c@acm.org>
[-- Attachment #1: Type: text/plain, Size: 233 bytes --]
> Has it been considered to change both iorequest_cnt and iodone_cnt into
> per-cpu counters?
We're testing with per-cpu counters, initial results look good. Once
the testing is complete,
I will post the next version.
Thanks,
Sumit
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] block: align nr_active_requests_shared_tags to avoid cache line contention
From: Sumit Saxena @ 2026-04-09 6:13 UTC (permalink / raw)
To: Bart Van Assche
Cc: martin.petersen, axboe, linux-scsi, linux-block,
mpi3mr-linuxdrv.pdl, James Rizzo
In-Reply-To: <1d0d35fe-5e9d-44b7-9dcd-48289f9d9f53@acm.org>
[-- Attachment #1: Type: text/plain, Size: 241 bytes --]
> A possible alternative is this patch that removes
> nr_active_requests_shared_tags:
>
> https://lore.kernel.org/linux-block/20240529213921.3166462-1-bvanassche@acm.org/
Sorry for the late reply. Let me test with your patch.
Thanks,
Sumit
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Jens Axboe @ 2026-04-08 23:36 UTC (permalink / raw)
To: Tal Zussman
Cc: Matthew Wilcox, Christian Brauner, Darrick J. Wong,
Carlos Maiolino, Alexander Viro, Jan Kara, Christoph Hellwig,
linux-block, linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <97b81868-6410-4c79-a242-679a9f04f073@columbia.edu>
On Apr 8, 2026, at 4:51 PM, Tal Zussman <tz2294@columbia.edu> wrote:
>
>
>
>> On 4/8/26 3:51 PM, Jens Axboe wrote:
>>> On 4/8/26 12: 48 PM, Tal Zussman wrote: > On 3/25/26 4: 14 PM, Jens Axboe wrote:
>>>
>>> Thanks! I'm going to give Dave's llist suggestion a shot on top of
>>> this as it seems like it'll simplify this nicely. Looks like that'll
>>> involve turning bio::bi_next into a union with a struct llist_node.
>>
>> Since these lists can get long, I'd keep an eye on llist reversal
>> overhead there...
>>
>
> Going to send v5 shortly -- tested with and without the llist reversal and
> it didn't seem to make much of a difference. This was on a single-disk VM
> though, so any stress testing you could do would be very helpful.
>
With all due respect, a single test like that isn’t going to be that useful. I’d be wary of making that change willy nilly and just thinking “it’s fine, worked fine on the one case I tested”.
—
Jens Axboe
^ permalink raw reply
* [PATCH RFC v5 3/3] block: enable RWF_DONTCACHE for block devices
From: Tal Zussman @ 2026-04-08 23:08 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, Dave Chinner, Bart Van Assche, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm, Tal Zussman
In-Reply-To: <20260408-blk-dontcache-v5-0-0f080c20a96f@columbia.edu>
Block device buffered reads and writes already pass through
filemap_read() and iomap_file_buffered_write() respectively, both of
which handle IOCB_DONTCACHE. Enable RWF_DONTCACHE for block device files
by setting FOP_DONTCACHE in def_blk_fops.
For CONFIG_BUFFER_HEAD=y paths, add block_write_begin_iocb() which
threads the kiocb through so that buffer_head-based I/O can use
DONTCACHE behavior. The existing block_write_begin() is preserved as a
wrapper that passes a NULL iocb. Set BIO_COMPLETE_IN_TASK in
submit_bh_wbc() when the folio has dropbehind so that buffer_head
writeback completions get deferred to task context.
CONFIG_BUFFER_HEAD=n paths are handled by the previously added iomap
BIO_COMPLETE_IN_TASK support.
This support is useful for databases that operate on raw block devices,
among other userspace applications.
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
block/fops.c | 5 +++--
fs/buffer.c | 22 +++++++++++++++++++---
include/linux/buffer_head.h | 3 +++
3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/block/fops.c b/block/fops.c
index 4d32785b31d9..d8165f6ba71c 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -505,7 +505,8 @@ static int blkdev_write_begin(const struct kiocb *iocb,
unsigned len, struct folio **foliop,
void **fsdata)
{
- return block_write_begin(mapping, pos, len, foliop, blkdev_get_block);
+ return block_write_begin_iocb(iocb, mapping, pos, len, foliop,
+ blkdev_get_block);
}
static int blkdev_write_end(const struct kiocb *iocb,
@@ -967,7 +968,7 @@ const struct file_operations def_blk_fops = {
.splice_write = iter_file_splice_write,
.fallocate = blkdev_fallocate,
.uring_cmd = blkdev_uring_cmd,
- .fop_flags = FOP_BUFFER_RASYNC,
+ .fop_flags = FOP_BUFFER_RASYNC | FOP_DONTCACHE,
};
static __init int blkdev_init(void)
diff --git a/fs/buffer.c b/fs/buffer.c
index ed724a902657..c60c0ad6cc35 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2239,14 +2239,19 @@ EXPORT_SYMBOL(block_commit_write);
*
* The filesystem needs to handle block truncation upon failure.
*/
-int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len,
+int block_write_begin_iocb(const struct kiocb *iocb,
+ struct address_space *mapping, loff_t pos, unsigned len,
struct folio **foliop, get_block_t *get_block)
{
pgoff_t index = pos >> PAGE_SHIFT;
+ fgf_t fgp_flags = FGP_WRITEBEGIN;
struct folio *folio;
int status;
- folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
+ if (iocb && iocb->ki_flags & IOCB_DONTCACHE)
+ fgp_flags |= FGP_DONTCACHE;
+
+ folio = __filemap_get_folio(mapping, index, fgp_flags,
mapping_gfp_mask(mapping));
if (IS_ERR(folio))
return PTR_ERR(folio);
@@ -2261,6 +2266,13 @@ int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len,
*foliop = folio;
return status;
}
+
+int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len,
+ struct folio **foliop, get_block_t *get_block)
+{
+ return block_write_begin_iocb(NULL, mapping, pos, len, foliop,
+ get_block);
+}
EXPORT_SYMBOL(block_write_begin);
int block_write_end(loff_t pos, unsigned len, unsigned copied,
@@ -2589,7 +2601,8 @@ int cont_write_begin(const struct kiocb *iocb, struct address_space *mapping,
(*bytes)++;
}
- return block_write_begin(mapping, pos, len, foliop, get_block);
+ return block_write_begin_iocb(iocb, mapping, pos, len, foliop,
+ get_block);
}
EXPORT_SYMBOL(cont_write_begin);
@@ -2801,6 +2814,9 @@ static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh,
bio = bio_alloc(bh->b_bdev, 1, opf, GFP_NOIO);
+ if (folio_test_dropbehind(bh->b_folio))
+ bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
+
fscrypt_set_bio_crypt_ctx_bh(bio, bh, GFP_NOIO);
bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index b16b88bfbc3e..ddf88ce290f2 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -260,6 +260,9 @@ int block_read_full_folio(struct folio *, get_block_t *);
bool block_is_partially_uptodate(struct folio *, size_t from, size_t count);
int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len,
struct folio **foliop, get_block_t *get_block);
+int block_write_begin_iocb(const struct kiocb *iocb,
+ struct address_space *mapping, loff_t pos, unsigned len,
+ struct folio **foliop, get_block_t *get_block);
int __block_write_begin(struct folio *folio, loff_t pos, unsigned len,
get_block_t *get_block);
int block_write_end(loff_t pos, unsigned len, unsigned copied, struct folio *);
--
2.39.5
^ permalink raw reply related
* [PATCH RFC v5 2/3] iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
From: Tal Zussman @ 2026-04-08 23:08 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, Dave Chinner, Bart Van Assche, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm, Tal Zussman
In-Reply-To: <20260408-blk-dontcache-v5-0-0f080c20a96f@columbia.edu>
Set BIO_COMPLETE_IN_TASK on iomap writeback bios when a dropbehind folio
is added. This ensures that bi_end_io runs in task context, where
folio_end_dropbehind() can safely invalidate folios.
With the bio layer now handling task-context deferral generically,
IOMAP_IOEND_DONTCACHE is no longer needed, as XFS no longer needs to
route DONTCACHE ioends through its completion workqueue. Remove the flag
and its NOMERGE entry.
Without the NOMERGE, regular I/Os that get merged with a dropbehind
folio will also have their completion deferred to task context.
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
fs/iomap/ioend.c | 5 +++--
fs/xfs/xfs_aops.c | 4 ----
include/linux/iomap.h | 6 +-----
3 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
index e4d57cb969f1..fe2a4c3dae42 100644
--- a/fs/iomap/ioend.c
+++ b/fs/iomap/ioend.c
@@ -182,8 +182,6 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio,
ioend_flags |= IOMAP_IOEND_UNWRITTEN;
if (wpc->iomap.flags & IOMAP_F_SHARED)
ioend_flags |= IOMAP_IOEND_SHARED;
- if (folio_test_dropbehind(folio))
- ioend_flags |= IOMAP_IOEND_DONTCACHE;
if (pos == wpc->iomap.offset && (wpc->iomap.flags & IOMAP_F_BOUNDARY))
ioend_flags |= IOMAP_IOEND_BOUNDARY;
@@ -200,6 +198,9 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio,
if (!bio_add_folio(&ioend->io_bio, folio, map_len, poff))
goto new_ioend;
+ if (folio_test_dropbehind(folio))
+ bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
+
/*
* Clamp io_offset and io_size to the incore EOF so that ondisk
* file size updates in the ioend completion are byte-accurate.
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 76678814f46f..0d469b91377d 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -510,10 +510,6 @@ xfs_ioend_needs_wq_completion(
if (ioend->io_flags & (IOMAP_IOEND_UNWRITTEN | IOMAP_IOEND_SHARED))
return true;
- /* Page cache invalidation cannot be done in irq context. */
- if (ioend->io_flags & IOMAP_IOEND_DONTCACHE)
- return true;
-
return false;
}
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 99b7209dabd7..a5d6401ebd80 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -392,16 +392,12 @@ sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
#define IOMAP_IOEND_BOUNDARY (1U << 2)
/* is direct I/O */
#define IOMAP_IOEND_DIRECT (1U << 3)
-/* is DONTCACHE I/O */
-#define IOMAP_IOEND_DONTCACHE (1U << 4)
-
/*
* Flags that if set on either ioend prevent the merge of two ioends.
* (IOMAP_IOEND_BOUNDARY also prevents merges, but only one-way)
*/
#define IOMAP_IOEND_NOMERGE_FLAGS \
- (IOMAP_IOEND_SHARED | IOMAP_IOEND_UNWRITTEN | IOMAP_IOEND_DIRECT | \
- IOMAP_IOEND_DONTCACHE)
+ (IOMAP_IOEND_SHARED | IOMAP_IOEND_UNWRITTEN | IOMAP_IOEND_DIRECT)
/*
* Structure for writeback I/O completions.
--
2.39.5
^ permalink raw reply related
* [PATCH RFC v5 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Tal Zussman @ 2026-04-08 23:08 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, Dave Chinner, Bart Van Assche, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm, Tal Zussman
In-Reply-To: <20260408-blk-dontcache-v5-0-0f080c20a96f@columbia.edu>
Some bio completion handlers need to run in task context but bio_endio()
can be called from IRQ context (e.g. buffer_head writeback). Add a
BIO_COMPLETE_IN_TASK flag that bio submitters can set to request
task-context completion of their bi_end_io callback.
When bio_endio() sees this flag and is running in non-task context, it
queues the bio to a per-cpu lockless list and schedules a delayed work
item to call bi_end_io() from task context. The delayed work uses a
1-jiffie delay to allow batches of completions to accumulate before
processing. A CPU hotplug dead callback drains any remaining bios from
the departing CPU's batch.
This will be used to enable RWF_DONTCACHE for block devices, and could
be used for other subsystems like fscrypt that need task-context bio
completion.
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
block/bio.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/blk_types.h | 7 +++-
2 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 8203bb7455a9..21b403eb1c04 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -18,6 +18,7 @@
#include <linux/highmem.h>
#include <linux/blk-crypto.h>
#include <linux/xarray.h>
+#include <linux/llist.h>
#include <trace/events/block.h>
#include "blk.h"
@@ -1714,6 +1715,51 @@ void bio_check_pages_dirty(struct bio *bio)
}
EXPORT_SYMBOL_GPL(bio_check_pages_dirty);
+struct bio_complete_batch {
+ struct llist_head list;
+ struct delayed_work work;
+ int cpu;
+};
+
+static DEFINE_PER_CPU(struct bio_complete_batch, bio_complete_batch);
+static struct workqueue_struct *bio_complete_wq;
+
+static void bio_complete_work_fn(struct work_struct *w)
+{
+ struct delayed_work *dw = to_delayed_work(w);
+ struct bio_complete_batch *batch =
+ container_of(dw, struct bio_complete_batch, work);
+ struct llist_node *node;
+ struct bio *bio, *next;
+
+ do {
+ node = llist_del_all(&batch->list);
+ if (!node)
+ break;
+
+ node = llist_reverse_order(node);
+ llist_for_each_entry_safe(bio, next, node, bi_llist)
+ bio->bi_end_io(bio);
+
+ if (need_resched()) {
+ if (!llist_empty(&batch->list))
+ mod_delayed_work_on(batch->cpu,
+ bio_complete_wq,
+ &batch->work, 0);
+ break;
+ }
+ } while (1);
+}
+
+static void bio_queue_completion(struct bio *bio)
+{
+ struct bio_complete_batch *batch = this_cpu_ptr(&bio_complete_batch);
+
+ if (llist_add(&bio->bi_llist, &batch->list))
+ mod_delayed_work_on(batch->cpu, bio_complete_wq,
+ &batch->work, 1);
+}
+
static inline bool bio_remaining_done(struct bio *bio)
{
/*
@@ -1788,7 +1834,9 @@ void bio_endio(struct bio *bio)
}
#endif
- if (bio->bi_end_io)
+ if (!in_task() && bio_flagged(bio, BIO_COMPLETE_IN_TASK))
+ bio_queue_completion(bio);
+ else if (bio->bi_end_io)
bio->bi_end_io(bio);
}
EXPORT_SYMBOL(bio_endio);
@@ -1974,6 +2022,24 @@ int bioset_init(struct bio_set *bs,
}
EXPORT_SYMBOL(bioset_init);
+/*
+ * Drain a dead CPU's deferred bio completions.
+ */
+static int bio_complete_batch_cpu_dead(unsigned int cpu)
+{
+ struct bio_complete_batch *batch =
+ per_cpu_ptr(&bio_complete_batch, cpu);
+ struct llist_node *node;
+ struct bio *bio, *next;
+
+ node = llist_del_all(&batch->list);
+ node = llist_reverse_order(node);
+ llist_for_each_entry_safe(bio, next, node, bi_llist)
+ bio->bi_end_io(bio);
+
+ return 0;
+}
+
static int __init init_bio(void)
{
int i;
@@ -1988,6 +2054,21 @@ static int __init init_bio(void)
SLAB_HWCACHE_ALIGN | SLAB_PANIC, NULL);
}
+ for_each_possible_cpu(i) {
+ struct bio_complete_batch *batch =
+ per_cpu_ptr(&bio_complete_batch, i);
+
+ init_llist_head(&batch->list);
+ INIT_DELAYED_WORK(&batch->work, bio_complete_work_fn);
+ batch->cpu = i;
+ }
+
+ bio_complete_wq = alloc_workqueue("bio_complete", WQ_MEM_RECLAIM, 0);
+ if (!bio_complete_wq)
+ panic("bio: can't allocate bio_complete workqueue\n");
+
+ cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "block/bio:complete:dead",
+ NULL, bio_complete_batch_cpu_dead);
cpuhp_setup_state_multi(CPUHP_BIO_DEAD, "block/bio:dead", NULL,
bio_cpu_dead);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 8808ee76e73c..0b55159d110d 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -11,6 +11,7 @@
#include <linux/device.h>
#include <linux/ktime.h>
#include <linux/rw_hint.h>
+#include <linux/llist.h>
struct bio_set;
struct bio;
@@ -208,7 +209,10 @@ typedef unsigned int blk_qc_t;
* stacking drivers)
*/
struct bio {
- struct bio *bi_next; /* request queue link */
+ union {
+ struct bio *bi_next; /* request queue link */
+ struct llist_node bi_llist; /* deferred completion */
+ };
struct block_device *bi_bdev;
blk_opf_t bi_opf; /* bottom bits REQ_OP, top bits
* req_flags.
@@ -322,6 +326,7 @@ enum {
BIO_REMAPPED,
BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
+ BIO_COMPLETE_IN_TASK, /* complete bi_end_io() in task context */
BIO_FLAG_LAST
};
--
2.39.5
^ permalink raw reply related
* [PATCH RFC v5 0/3] block: enable RWF_DONTCACHE for block devices
From: Tal Zussman @ 2026-04-08 23:08 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, Dave Chinner, Bart Van Assche, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm, Tal Zussman
Add support for using RWF_DONTCACHE with block devices.
Dropbehind pruning needs to be done in non-IRQ context, but block
devices complete writeback in IRQ context.
To fix this, we can defer dropbehind invalidation to task context. We
introduce a new BIO_COMPLETE_IN_TASK flag that allows the bio submitter
to request task-context completion of bi_end_io. When bio_endio() sees
this flag in non-task context, it queues the bio to a per-CPU lockless
list and schedules a delayed work item to do bio completion.
Patch 1 adds the BIO_COMPLETE_IN_TASK infrastructure in the block
layer.
Patch 2 wires BIO_COMPLETE_IN_TASK into iomap writeback for dropbehind
folios, removes IOMAP_IOEND_DONTCACHE, and removes the DONTCACHE
workqueue deferral from XFS.
Patch 3 enables RWF_DONTCACHE for block devices, setting
BIO_COMPLETE_IN_TASK in submit_bh_wbc() for the CONFIG_BUFFER_HEAD
path.
This support is useful for databases that operate on raw block devices,
among other userspace applications.
I tested this (with CONFIG_BUFFER_HEAD=y) for reads and writes on a
single block device on a VM, so results may be noisy.
Reads were tested on the root partition with a 45GB range (~2x RAM).
Writes were tested on a disabled swap parition (~1GB) in a memcg of size
244MB to force reclaim pressure.
Results:
===== READS (/dev/nvme0n1p2) =====
sec normal MB/s dontcache MB/s
---- ------------ --------------
1 1098.6 1609.0
2 1270.3 1506.6
3 1093.3 1576.5
4 1141.8 2393.9
5 1365.3 2793.8
6 1324.6 2065.9
7 879.6 1920.7
8 1434.1 1662.4
9 1184.9 1857.9
10 1166.4 1702.8
11 1161.4 1653.4
12 1086.9 1555.4
13 1198.5 1718.9
14 1111.9 1752.2
---- ------------ --------------
avg 1173.7 1828.8 (+56%)
==== WRITES (/dev/nvme0n1p3) =====
sec normal MB/s dontcache MB/s
---- ------------ --------------
1 692.4 9297.7
2 4810.8 9342.8
3 5221.7 2955.2
4 396.7 8488.3
5 7249.2 9249.3
6 6695.4 1376.2
7 122.9 9125.8
8 5486.5 9414.7
9 6921.5 8743.5
10 27.9 8997.8
---- ------------ --------------
avg 3762.5 7699.1 (+105%)
---
Changes in v5:
- 1/3: Replace local_lock + bio_list with struct llist, per Dave.
- 1/3: Use delayed_work with 1-jiffie delay, per Dave.
- 1/3: Add dedicated workqueue to avoid deadlocks, per Christoph.
- 1/3: Restructure work function as do/while loop and only schedule work
originally when the list was previously empty, per Jens.
- 2/3: Delete IOMAP_IOEND_DONTCACHE and its NOMERGE entry, per Matthew
and Christoph.
- Link to v4: https://lore.kernel.org/r/20260325-blk-dontcache-v4-0-c4b56db43f64@columbia.edu
Changes in v4:
- 1/3: Move dropbehind deferral from folio-level to bio-level using
BIO_COMPLETE_IN_TASK, per Matthew and Jan.
- 1/3: Work function yields on need_resched() to avoid hogging the CPU,
per Jan.
- 2/3: New patch. Set BIO_COMPLETE_IN_TASK on iomap writeback bios for
DONTCACHE folios, removing the need for XFS-specific workqueue
deferral.
- 3/3: Set BIO_COMPLETE_IN_TASK in submit_bh_wbc() for buffer_head
path.
- 3/3: Update commit message to mention CONFIG_BUFFER_HEAD=n path.
- Link to v3: https://lore.kernel.org/r/20260227-blk-dontcache-v3-0-cd309ccd5868@columbia.edu
Changes in v3:
- 1/2: Convert dropbehind deferral to per-CPU folio_batches protected by
local_lock using per-CPU work items, to reduce contention, per Jens.
- 1/2: Call folio_end_dropbehind_irq() directly from
folio_end_writeback(), per Jens.
- 1/2: Add CPU hotplug dead callback to drain the departing CPU's folio
batch.
- 2/2: Introduce block_write_begin_iocb(), per Christoph.
- 2/2: Dropped R-b due to changes.
- Link to v2: https://lore.kernel.org/r/20260225-blk-dontcache-v2-0-70e7ac4f7108@columbia.edu
Changes in v2:
- Add R-b from Jan Kara for 2/2.
- Add patch to defer dropbehind completion from IRQ context via a work
item (1/2).
- Add initial performance numbers to cover letter.
- Link to v1: https://lore.kernel.org/r/20260218-blk-dontcache-v1-1-fad6675ef71f@columbia.edu
---
Tal Zussman (3):
block: add BIO_COMPLETE_IN_TASK for task-context completion
iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
block: enable RWF_DONTCACHE for block devices
block/bio.c | 83 ++++++++++++++++++++++++++++++++++++++++++++-
block/fops.c | 5 +--
fs/buffer.c | 22 ++++++++++--
fs/iomap/ioend.c | 5 +--
fs/xfs/xfs_aops.c | 4 ---
include/linux/blk_types.h | 7 +++-
include/linux/buffer_head.h | 3 ++
include/linux/iomap.h | 6 +---
8 files changed, 117 insertions(+), 18 deletions(-)
---
base-commit: f384d0b7710d3edaab718c02bbae46a4d3fd09de
change-id: 20260218-blk-dontcache-338133dd045e
Best regards,
--
Tal Zussman <tz2294@columbia.edu>
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Tal Zussman @ 2026-04-08 22:51 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, linux-block, linux-kernel, linux-xfs,
linux-fsdevel, linux-mm
In-Reply-To: <699933c1-c150-4e10-a5fc-6f128260d0c1@kernel.dk>
On 4/8/26 3:51 PM, Jens Axboe wrote:
> On 4/8/26 12: 48 PM, Tal Zussman wrote: > On 3/25/26 4: 14 PM, Jens Axboe wrote:
>>
>> Thanks! I'm going to give Dave's llist suggestion a shot on top of
>> this as it seems like it'll simplify this nicely. Looks like that'll
>> involve turning bio::bi_next into a union with a struct llist_node.
>
> Since these lists can get long, I'd keep an eye on llist reversal
> overhead there...
>
Going to send v5 shortly -- tested with and without the llist reversal and
it didn't seem to make much of a difference. This was on a single-disk VM
though, so any stress testing you could do would be very helpful.
Thanks,
Tal
^ permalink raw reply
* Re: [PATCH RFC v4 2/3] iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
From: Tal Zussman @ 2026-04-08 20:10 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jens Axboe, Christian Brauner, Darrick J. Wong, Carlos Maiolino,
Alexander Viro, Jan Kara, Christoph Hellwig, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <ada0KnRjpYnY_QYh@casper.infradead.org>
On 4/8/26 4:01 PM, Matthew Wilcox wrote:
> On Wed, Apr 08, 2026 at 03:44:37PM -0400, Tal Zussman wrote:
>> On 3/25/26 4:21 PM, Matthew Wilcox wrote:
>> > On Wed, Mar 25, 2026 at 02:43:01PM -0400, Tal Zussman wrote:
>> >> Set BIO_COMPLETE_IN_TASK on iomap writeback bios when
>> >> IOMAP_IOEND_DONTCACHE is set. This ensures that bi_end_io runs in task
>> >> context, where folio_end_dropbehind() can safely invalidate folios.
>> >>
>> >> With the bio layer now handling task-context deferral generically, XFS
>> >> no longer needs to route DONTCACHE ioends through its completion
>> >> workqueue for page cache invalidation. Remove the DONTCACHE check from
>> >> xfs_ioend_needs_wq_completion().
>> >>
>> >> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>> >> ---
>> >> fs/iomap/ioend.c | 2 ++
>> >> fs/xfs/xfs_aops.c | 4 ----
>> >> 2 files changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
>> >> index e4d57cb969f1..6b8375d11cc0 100644
>> >> --- a/fs/iomap/ioend.c
>> >> +++ b/fs/iomap/ioend.c
>> >> @@ -113,6 +113,8 @@ static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc,
>> >> GFP_NOFS, &iomap_ioend_bioset);
>> >> bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos);
>> >> bio->bi_write_hint = wpc->inode->i_write_hint;
>> >> + if (ioend_flags & IOMAP_IOEND_DONTCACHE)
>> >> + bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
>> >> wbc_init_bio(wpc->wbc, bio);
>> >> wpc->nr_folios = 0;
>> >> return iomap_init_ioend(wpc->inode, bio, pos, ioend_flags);
>> >
>> > Can't we delete IOMAP_IOEND_DONTCACHE, and just do:
>> >
>> > if (folio_test_dropbehind(folio))
>> > bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
>> >
>> > It'd need to move down a few lines in iomap_add_to_ioend() to after
>> > bio_add_folio() succeeds.
>> >
>>
>> Actually, looking into it more, IOMAP_IOEND_DONTCACHE is used as part of
>> IOMAP_IOEND_NOMERGE_FLAGS. I think deleting it while maintaining the
>> no-merge behavior would be uglier than leaving it in.
>
> But why was it added to NOMERGE in the first place? I don't think it's
> harmful to merge writeback I/Os which are COMPLETE_IN_TASK and I/Os
> which are not, as long as the final I/O is completed in a task.
>
Looks like it was added in commit 34ecde3c5606 to ensure XFS DONTCACHE
completions were actually punted to task context... which we now ensure
elsewhere. Will delete and mention the side effect on regular I/Os in the
commit message.
^ permalink raw reply
* Re: [PATCH RFC v4 2/3] iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
From: Matthew Wilcox @ 2026-04-08 20:01 UTC (permalink / raw)
To: Tal Zussman
Cc: Jens Axboe, Christian Brauner, Darrick J. Wong, Carlos Maiolino,
Alexander Viro, Jan Kara, Christoph Hellwig, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <9631d652-fd3a-45ac-b8a3-b632b26f6fa5@columbia.edu>
On Wed, Apr 08, 2026 at 03:44:37PM -0400, Tal Zussman wrote:
> On 3/25/26 4:21 PM, Matthew Wilcox wrote:
> > On Wed, Mar 25, 2026 at 02:43:01PM -0400, Tal Zussman wrote:
> >> Set BIO_COMPLETE_IN_TASK on iomap writeback bios when
> >> IOMAP_IOEND_DONTCACHE is set. This ensures that bi_end_io runs in task
> >> context, where folio_end_dropbehind() can safely invalidate folios.
> >>
> >> With the bio layer now handling task-context deferral generically, XFS
> >> no longer needs to route DONTCACHE ioends through its completion
> >> workqueue for page cache invalidation. Remove the DONTCACHE check from
> >> xfs_ioend_needs_wq_completion().
> >>
> >> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> >> ---
> >> fs/iomap/ioend.c | 2 ++
> >> fs/xfs/xfs_aops.c | 4 ----
> >> 2 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
> >> index e4d57cb969f1..6b8375d11cc0 100644
> >> --- a/fs/iomap/ioend.c
> >> +++ b/fs/iomap/ioend.c
> >> @@ -113,6 +113,8 @@ static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc,
> >> GFP_NOFS, &iomap_ioend_bioset);
> >> bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos);
> >> bio->bi_write_hint = wpc->inode->i_write_hint;
> >> + if (ioend_flags & IOMAP_IOEND_DONTCACHE)
> >> + bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
> >> wbc_init_bio(wpc->wbc, bio);
> >> wpc->nr_folios = 0;
> >> return iomap_init_ioend(wpc->inode, bio, pos, ioend_flags);
> >
> > Can't we delete IOMAP_IOEND_DONTCACHE, and just do:
> >
> > if (folio_test_dropbehind(folio))
> > bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
> >
> > It'd need to move down a few lines in iomap_add_to_ioend() to after
> > bio_add_folio() succeeds.
> >
>
> Actually, looking into it more, IOMAP_IOEND_DONTCACHE is used as part of
> IOMAP_IOEND_NOMERGE_FLAGS. I think deleting it while maintaining the
> no-merge behavior would be uglier than leaving it in.
But why was it added to NOMERGE in the first place? I don't think it's
harmful to merge writeback I/Os which are COMPLETE_IN_TASK and I/Os
which are not, as long as the final I/O is completed in a task.
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Jens Axboe @ 2026-04-08 19:51 UTC (permalink / raw)
To: Tal Zussman, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, linux-block, linux-kernel, linux-xfs,
linux-fsdevel, linux-mm
In-Reply-To: <01e6c582-fbab-40ec-97ac-02675e6a08ed@columbia.edu>
On 4/8/26 12:48 PM, Tal Zussman wrote:
> On 3/25/26 4:14 PM, Jens Axboe wrote:
>> On 3/25/26 12:43 PM, Tal Zussman wrote:
>>> +static void bio_complete_work_fn(struct work_struct *w)
>>> +{
>>> + struct bio_complete_batch *batch;
>>> + struct bio_list list;
>>> +
>>> +again:
>>> + local_lock_irq(&bio_complete_batch.lock);
>>> + batch = this_cpu_ptr(&bio_complete_batch);
>>> + list = batch->list;
>>> + bio_list_init(&batch->list);
>>> + local_unlock_irq(&bio_complete_batch.lock);
>>> +
>>> + while (!bio_list_empty(&list)) {
>>> + struct bio *bio = bio_list_pop(&list);
>>> + bio->bi_end_io(bio);
>>> + }
>>> +
>>> + local_lock_irq(&bio_complete_batch.lock);
>>> + batch = this_cpu_ptr(&bio_complete_batch);
>>> + if (!bio_list_empty(&batch->list)) {
>>> + local_unlock_irq(&bio_complete_batch.lock);
>>> +
>>> + if (!need_resched())
>>> + goto again;
>>> +
>>> + schedule_work_on(smp_processor_id(), &batch->work);
>>> + return;
>>> + }
>>> + local_unlock_irq(&bio_complete_batch.lock);
>>> +}
>>
>> bool looped = false;
>>
>> do {
>> if (looped && need_resched()) {
>> schedule_work_on(smp_processor_id(), &batch->work);
>> break;
>> }
>>
>> local_lock_irq(&bio_complete_batch.lock);
>> batch = this_cpu_ptr(&bio_complete_batch);
>> list = batch->list;
>> bio_list_init(&batch->list);
>> local_unlock_irq(&bio_complete_batch.lock);
>>
>> if (bio_list_empty(&list))
>> break;
>>
>> do {
>> struct bio *bio = bio_list_pop(&list);
>> bio->bi_end_io(bio);
>> } while (!bio_list_empty(&list));
>> looped = true;
>> } while (1);
>>
>> would be a lot easier to read, and avoid needing the list manipulation
>> included twice.
>
> Yep, that looks cleaner. Although do we really need the looped variable?
> Can't we just move the need_resched() check right before the while (1)?
If you do that, then you'd also want to check if the list is empty. You
don't want to schedule_work() for a potentially empty list. Either way,
you need some check.
>>> +static void bio_queue_completion(struct bio *bio)
>>> +{
>>> + struct bio_complete_batch *batch;
>>> + unsigned long flags;
>>> +
>>> + local_lock_irqsave(&bio_complete_batch.lock, flags);
>>> + batch = this_cpu_ptr(&bio_complete_batch);
>>> + bio_list_add(&batch->list, bio);
>>> + local_unlock_irqrestore(&bio_complete_batch.lock, flags);
>>> +
>>> + schedule_work_on(smp_processor_id(), &batch->work);
>>> +}
>>
>> Maybe do something ala:
>>
>> static void bio_queue_completion(struct bio *bio)
>> {
>> struct bio_complete_batch *batch;
>> unsigned long flags;
>> bool was_empty;
>>
>> local_lock_irqsave(&bio_complete_batch.lock, flags);
>> batch = this_cpu_ptr(&bio_complete_batch);
>> was_empty = bio_list_empty(&batch->list);
>> bio_list_add(&batch->list, bio);
>> local_unlock_irqrestore(&bio_complete_batch.lock, flags);
>>
>> if (was_empty)
>> schedule_work_on(smp_processor_id(), &batch->work);
>> }
>
> Makes sense, will do!
>
>> Outside of these mostly nits, I like this approach. It avoids my main
>> worry with this, which was contention on the list locks. And on the
>> io_uring side, we'll never hit the !in_task() path anyway, as the
>> completions are run from the task always. The bio flag makes sense for
>> this.
>
> Thanks! I'm going to give Dave's llist suggestion a shot on top of
> this as it seems like it'll simplify this nicely. Looks like that'll
> involve turning bio::bi_next into a union with a struct llist_node.
Since these lists can get long, I'd keep an eye on llist reversal
overhead there...
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH RFC v4 2/3] iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
From: Tal Zussman @ 2026-04-08 19:44 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jens Axboe, Christian Brauner, Darrick J. Wong, Carlos Maiolino,
Alexander Viro, Jan Kara, Christoph Hellwig, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <acRDyJrkJhnHHMSe@casper.infradead.org>
On 3/25/26 4:21 PM, Matthew Wilcox wrote:
> On Wed, Mar 25, 2026 at 02:43:01PM -0400, Tal Zussman wrote:
>> Set BIO_COMPLETE_IN_TASK on iomap writeback bios when
>> IOMAP_IOEND_DONTCACHE is set. This ensures that bi_end_io runs in task
>> context, where folio_end_dropbehind() can safely invalidate folios.
>>
>> With the bio layer now handling task-context deferral generically, XFS
>> no longer needs to route DONTCACHE ioends through its completion
>> workqueue for page cache invalidation. Remove the DONTCACHE check from
>> xfs_ioend_needs_wq_completion().
>>
>> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>> ---
>> fs/iomap/ioend.c | 2 ++
>> fs/xfs/xfs_aops.c | 4 ----
>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
>> index e4d57cb969f1..6b8375d11cc0 100644
>> --- a/fs/iomap/ioend.c
>> +++ b/fs/iomap/ioend.c
>> @@ -113,6 +113,8 @@ static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc,
>> GFP_NOFS, &iomap_ioend_bioset);
>> bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos);
>> bio->bi_write_hint = wpc->inode->i_write_hint;
>> + if (ioend_flags & IOMAP_IOEND_DONTCACHE)
>> + bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
>> wbc_init_bio(wpc->wbc, bio);
>> wpc->nr_folios = 0;
>> return iomap_init_ioend(wpc->inode, bio, pos, ioend_flags);
>
> Can't we delete IOMAP_IOEND_DONTCACHE, and just do:
>
> if (folio_test_dropbehind(folio))
> bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
>
> It'd need to move down a few lines in iomap_add_to_ioend() to after
> bio_add_folio() succeeds.
>
Actually, looking into it more, IOMAP_IOEND_DONTCACHE is used as part of
IOMAP_IOEND_NOMERGE_FLAGS. I think deleting it while maintaining the
no-merge behavior would be uglier than leaving it in.
^ permalink raw reply
* Re: [PATCH RFC v4 2/3] iomap: use BIO_COMPLETE_IN_TASK for dropbehind writeback
From: Tal Zussman @ 2026-04-08 19:36 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jens Axboe, Christian Brauner, Darrick J. Wong, Carlos Maiolino,
Alexander Viro, Jan Kara, Christoph Hellwig, linux-block,
linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <acRDyJrkJhnHHMSe@casper.infradead.org>
On 3/25/26 4:21 PM, Matthew Wilcox wrote:
> On Wed, Mar 25, 2026 at 02:43:01PM -0400, Tal Zussman wrote:
>> Set BIO_COMPLETE_IN_TASK on iomap writeback bios when
>> IOMAP_IOEND_DONTCACHE is set. This ensures that bi_end_io runs in task
>> context, where folio_end_dropbehind() can safely invalidate folios.
>>
>> With the bio layer now handling task-context deferral generically, XFS
>> no longer needs to route DONTCACHE ioends through its completion
>> workqueue for page cache invalidation. Remove the DONTCACHE check from
>> xfs_ioend_needs_wq_completion().
>>
>> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>> ---
>> fs/iomap/ioend.c | 2 ++
>> fs/xfs/xfs_aops.c | 4 ----
>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
>> index e4d57cb969f1..6b8375d11cc0 100644
>> --- a/fs/iomap/ioend.c
>> +++ b/fs/iomap/ioend.c
>> @@ -113,6 +113,8 @@ static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc,
>> GFP_NOFS, &iomap_ioend_bioset);
>> bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos);
>> bio->bi_write_hint = wpc->inode->i_write_hint;
>> + if (ioend_flags & IOMAP_IOEND_DONTCACHE)
>> + bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
>> wbc_init_bio(wpc->wbc, bio);
>> wpc->nr_folios = 0;
>> return iomap_init_ioend(wpc->inode, bio, pos, ioend_flags);
>
> Can't we delete IOMAP_IOEND_DONTCACHE, and just do:
>
> if (folio_test_dropbehind(folio))
> bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
>
> It'd need to move down a few lines in iomap_add_to_ioend() to after
> bio_add_folio() succeeds.
>
Will do, thanks Matthew!
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Tal Zussman @ 2026-04-08 19:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara,
linux-block, linux-kernel, linux-xfs, linux-fsdevel, linux-mm
In-Reply-To: <acYdOOpo51xrzNBg@infradead.org>
On 3/27/26 2:01 AM, Christoph Hellwig wrote:
> On Wed, Mar 25, 2026 at 02:43:00PM -0400, Tal Zussman wrote:
>> +static void bio_complete_work_fn(struct work_struct *w)
>> +{
>> + struct bio_complete_batch *batch;
>> + struct bio_list list;
>> +
>> +again:
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + list = batch->list;
>> + bio_list_init(&batch->list);
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +
>> + while (!bio_list_empty(&list)) {
>> + struct bio *bio = bio_list_pop(&list);
>> + bio->bi_end_io(bio);
>> + }
>
> bio_list_pop already does a NULL check, so this could be:
>
> while ((bio = bio_list_pop(&batch->list)))
> bio->bi_end_io(bio);
>
> In fact that same pattern is repeated later, so maybe just add a helper
> for it? But I think Dave's idea of just using a llist (and adding a
> new llist member to the bio for this) seems sensible. Just don't forget
> the llist_reverse_order call to avoid reordering.
Will switch to llist.
>> +
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + if (!bio_list_empty(&batch->list)) {
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +
>> + if (!need_resched())
>> + goto again;
>> +
>> + schedule_work_on(smp_processor_id(), &batch->work);
>> + return;
>> + }
>> + local_unlock_irq(&bio_complete_batch.lock);
>
> I don't really understand this requeue logic. Can you explain it?
If new bios were queued while we were processing this batch, we loop back
and process them, if we don't need to reschedule. If we do need to
reschedule, we re-queue the work item and return.
>> + schedule_work_on(smp_processor_id(), &batch->work);
>
> We'll probably want a dedicated workqueue here to avoid deadlocks
> vs other system wq uses.
>
>> +static int bio_complete_batch_cpu_dead(unsigned int cpu)
>> +{
>> + struct bio_complete_batch *batch = per_cpu_ptr(&bio_complete_batch, cpu);
>
> Overly long line.
>
Will fix.
Thanks,
Tal
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Tal Zussman @ 2026-04-08 18:50 UTC (permalink / raw)
To: Dave Chinner
Cc: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara,
Christoph Hellwig, linux-block, linux-kernel, linux-xfs,
linux-fsdevel, linux-mm
In-Reply-To: <acRE8pp6HxjK7Z74@dread>
On 3/25/26 4:26 PM, Dave Chinner wrote:
> On Wed, Mar 25, 2026 at 02:43:00PM -0400, Tal Zussman wrote:
>> Some bio completion handlers need to run in task context but bio_endio()
>> can be called from IRQ context (e.g. buffer_head writeback). Add a
>> BIO_COMPLETE_IN_TASK flag that bio submitters can set to request
>> task-context completion of their bi_end_io callback.
>>
>> When bio_endio() sees this flag and is running in non-task context, it
>> queues the bio to a per-cpu list and schedules a work item to call
>> bi_end_io() from task context. A CPU hotplug dead callback drains any
>> remaining bios from the departing CPU's batch.
>>
>> This will be used to enable RWF_DONTCACHE for block devices, and could
>> be used for other subsystems like fscrypt that need task-context bio
>> completion.
>>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>> ---
>> block/bio.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/blk_types.h | 1 +
>> 2 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 8203bb7455a9..69ee0d93041f 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -18,6 +18,7 @@
>> #include <linux/highmem.h>
>> #include <linux/blk-crypto.h>
>> #include <linux/xarray.h>
>> +#include <linux/local_lock.h>
>>
>> #include <trace/events/block.h>
>> #include "blk.h"
>> @@ -1714,6 +1715,60 @@ void bio_check_pages_dirty(struct bio *bio)
>> }
>> EXPORT_SYMBOL_GPL(bio_check_pages_dirty);
>>
>> +struct bio_complete_batch {
>> + local_lock_t lock;
>> + struct bio_list list;
>> + struct work_struct work;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct bio_complete_batch, bio_complete_batch) = {
>> + .lock = INIT_LOCAL_LOCK(lock),
>> +};
>> +
>> +static void bio_complete_work_fn(struct work_struct *w)
>> +{
>> + struct bio_complete_batch *batch;
>> + struct bio_list list;
>> +
>> +again:
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + list = batch->list;
>> + bio_list_init(&batch->list);
>> + local_unlock_irq(&bio_complete_batch.lock);
>
> This is just a FIFO processing queue, and it is so wanting to be a
> struct llist for lockless queuing and dequeueing.
>
> We do this lockless per-cpu queue + per-cpu workqueue in XFS for
> background inode GC processing. See struct xfs_inodegc and all the
> xfs_inodegc_*() functions - it may be useful to have a generic
> lockless per-cpu queue processing so we don't keep open coding this
> repeating pattern everywhere.
>
>> +
>> + while (!bio_list_empty(&list)) {
>> + struct bio *bio = bio_list_pop(&list);
>> + bio->bi_end_io(bio);
>> + }
>> +
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + if (!bio_list_empty(&batch->list)) {
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +
>> + if (!need_resched())
>> + goto again;
>> +
>> + schedule_work_on(smp_processor_id(), &batch->work);
>
> We've learnt that immediately scheduling per-cpu batch
> processing work can cause context switch storms as the queue/dequeue
> steps one work item at a time.
>
> Hence we use a delayed work with a scheduling delay of a singel
> jiffie to allow batches of queue work from a single context to
> complete before (potentially) being pre-empted by the per-cpu
> kworker task that will process the queue...
>
>> + return;
>> + }
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +}
>> +
>> +static void bio_queue_completion(struct bio *bio)
>> +{
>> + struct bio_complete_batch *batch;
>> + unsigned long flags;
>> +
>> + local_lock_irqsave(&bio_complete_batch.lock, flags);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + bio_list_add(&batch->list, bio);
>> + local_unlock_irqrestore(&bio_complete_batch.lock, flags);
>> +
>> + schedule_work_on(smp_processor_id(), &batch->work);
>> +}
>
> Yeah, we definitely want to queue all the pending bio completions
> the interrupt is delivering before we run the batch processing...
>
>> +
>> static inline bool bio_remaining_done(struct bio *bio)
>> {
>> /*
>> @@ -1788,7 +1843,9 @@ void bio_endio(struct bio *bio)
>> }
>> #endif
>>
>> - if (bio->bi_end_io)
>> + if (!in_task() && bio_flagged(bio, BIO_COMPLETE_IN_TASK))
>> + bio_queue_completion(bio);
>> + else if (bio->bi_end_io)
>> bio->bi_end_io(bio);
>> }
>> EXPORT_SYMBOL(bio_endio);
>> @@ -1974,6 +2031,21 @@ int bioset_init(struct bio_set *bs,
>> }
>> EXPORT_SYMBOL(bioset_init);
>>
>> +/*
>> + * Drain a dead CPU's deferred bio completions. The CPU is dead so no locking
>> + * is needed -- no new bios will be queued to it.
>> + */
>> +static int bio_complete_batch_cpu_dead(unsigned int cpu)
>> +{
>> + struct bio_complete_batch *batch = per_cpu_ptr(&bio_complete_batch, cpu);
>> + struct bio *bio;
>> +
>> + while ((bio = bio_list_pop(&batch->list)))
>> + bio->bi_end_io(bio);
>> +
>> + return 0;
>> +}
>
> If you use a llist for the queue, this code is no different to the
> normal processing work.
>
Thanks Dave, these suggestions + the pointer to the XFS GC code were helpful.
I'll incorporate them into the next version.
- Tal
^ permalink raw reply
* Re: [PATCH RFC v4 1/3] block: add BIO_COMPLETE_IN_TASK for task-context completion
From: Tal Zussman @ 2026-04-08 18:48 UTC (permalink / raw)
To: Jens Axboe, Matthew Wilcox (Oracle), Christian Brauner,
Darrick J. Wong, Carlos Maiolino, Alexander Viro, Jan Kara
Cc: Christoph Hellwig, linux-block, linux-kernel, linux-xfs,
linux-fsdevel, linux-mm
In-Reply-To: <c4453f4e-c1a2-4f5a-b278-76b63d5dc4a3@kernel.dk>
On 3/25/26 4:14 PM, Jens Axboe wrote:
> On 3/25/26 12:43 PM, Tal Zussman wrote:
>> +static void bio_complete_work_fn(struct work_struct *w)
>> +{
>> + struct bio_complete_batch *batch;
>> + struct bio_list list;
>> +
>> +again:
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + list = batch->list;
>> + bio_list_init(&batch->list);
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +
>> + while (!bio_list_empty(&list)) {
>> + struct bio *bio = bio_list_pop(&list);
>> + bio->bi_end_io(bio);
>> + }
>> +
>> + local_lock_irq(&bio_complete_batch.lock);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + if (!bio_list_empty(&batch->list)) {
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +
>> + if (!need_resched())
>> + goto again;
>> +
>> + schedule_work_on(smp_processor_id(), &batch->work);
>> + return;
>> + }
>> + local_unlock_irq(&bio_complete_batch.lock);
>> +}
>
> bool looped = false;
>
> do {
> if (looped && need_resched()) {
> schedule_work_on(smp_processor_id(), &batch->work);
> break;
> }
>
> local_lock_irq(&bio_complete_batch.lock);
> batch = this_cpu_ptr(&bio_complete_batch);
> list = batch->list;
> bio_list_init(&batch->list);
> local_unlock_irq(&bio_complete_batch.lock);
>
> if (bio_list_empty(&list))
> break;
>
> do {
> struct bio *bio = bio_list_pop(&list);
> bio->bi_end_io(bio);
> } while (!bio_list_empty(&list));
> looped = true;
> } while (1);
>
> would be a lot easier to read, and avoid needing the list manipulation
> included twice.
Yep, that looks cleaner. Although do we really need the looped variable?
Can't we just move the need_resched() check right before the while (1)?
>> +static void bio_queue_completion(struct bio *bio)
>> +{
>> + struct bio_complete_batch *batch;
>> + unsigned long flags;
>> +
>> + local_lock_irqsave(&bio_complete_batch.lock, flags);
>> + batch = this_cpu_ptr(&bio_complete_batch);
>> + bio_list_add(&batch->list, bio);
>> + local_unlock_irqrestore(&bio_complete_batch.lock, flags);
>> +
>> + schedule_work_on(smp_processor_id(), &batch->work);
>> +}
>
> Maybe do something ala:
>
> static void bio_queue_completion(struct bio *bio)
> {
> struct bio_complete_batch *batch;
> unsigned long flags;
> bool was_empty;
>
> local_lock_irqsave(&bio_complete_batch.lock, flags);
> batch = this_cpu_ptr(&bio_complete_batch);
> was_empty = bio_list_empty(&batch->list);
> bio_list_add(&batch->list, bio);
> local_unlock_irqrestore(&bio_complete_batch.lock, flags);
>
> if (was_empty)
> schedule_work_on(smp_processor_id(), &batch->work);
> }
Makes sense, will do!
> Outside of these mostly nits, I like this approach. It avoids my main
> worry with this, which was contention on the list locks. And on the
> io_uring side, we'll never hit the !in_task() path anyway, as the
> completions are run from the task always. The bio flag makes sense for
> this.
Thanks! I'm going to give Dave's llist suggestion a shot on top of this as
it seems like it'll simplify this nicely. Looks like that'll involve turning
bio::bi_next into a union with a struct llist_node.
- Tal
^ permalink raw reply
* Re: [PATCH 07/13] libmultipath: Add delayed removal support
From: John Garry @ 2026-04-08 16:28 UTC (permalink / raw)
To: Hannes Reinecke, 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: <6d7a4076-a4ad-4185-8e82-8e27d704d20e@suse.de>
On 08/04/2026 16:41, Hannes Reinecke wrote:
> On 4/8/26 13:28, John Garry wrote:
>> 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).
>>
>
> That look patently like the 'queue_if_no_path' feature from dm-
> multipath. Any chance of reconciling these two?
You mean a common blktests testcase, right?
For NVMe, that test would:
a. try to remove NVMe ko when we have the delayed removal active
b. ensure that we can queue for no path
I suppose that a common testcase could be possible (with dm mpath), but
doesn't dm have its own testsuite?
Thanks,
John
^ permalink raw reply
* Re: [PATCH v10 13/13] docs: add io_queue flag to isolcpus
From: Aaron Tomlin @ 2026-04-08 15:58 UTC (permalink / raw)
To: Ming Lei
Cc: axboe, kbusch, hch, sagi, mst, aacraid, James.Bottomley,
martin.petersen, liyihang9, kashyap.desai, sumit.saxena,
shivasharan.srikanteshwara, chandrakanth.patil, sathya.prakash,
sreekanth.reddy, suganath-prabu.subramani, ranjan.kumar,
jinpu.wang, tglx, mingo, peterz, juri.lelli, vincent.guittot,
akpm, maz, ruanjinjie, bigeasy, yphbchou0911, wagi, frederic,
longman, chenridong, hare, kch, steve, sean, chjohnst, neelx,
mproche, linux-block, linux-kernel, virtualization, linux-nvme,
linux-scsi, megaraidlinux.pdl, mpi3mr-linuxdrv.pdl,
MPT-FusionLinux.pdl
In-Reply-To: <adMoon3Zf6gO-UbA@fedora>
[-- Attachment #1: Type: text/plain, Size: 3971 bytes --]
On Mon, Apr 06, 2026 at 11:29:38AM +0800, Ming Lei wrote:
> I don't think there is such breaking isolation thing. For iopoll, if
> applications won't submit polled IO on isolated CPUs, everything is just
> fine. If they do it, IO may be reaped from isolated CPUs, that is just their
> choice, anything is wrong?
Hi Ming,
Thank you for your follow up. You make a fair point regarding polling
queues and application choice; if an application explicitly binds to an
isolated CPU and submits polled operations, it is indeed actively electing
to utilise that core and accept the resulting behaviour.
However, the architectural challenge arises from how the kernel handles
these queues structurally when the application does not explicitly make
that choice. Because poll queues never utilise interrupts, they are
completely invisible to the managed interrupt subsystem.
If we were to rely exclusively on the managed irq flag, the block layer
would blindly map these non interrupt driven polling queues to isolated
CPUs. If a general background storage operation were then routed to
that queue, the isolated core would be forced to spin actively in a tight
loop waiting for the hardware completion. This would completely monopolise
the core and destroy any real time isolation guarantees without the user
space application ever having requested it.
This illustrates precisely why the io queue flag is a mechanical necessity.
Its primary objective is to act as a comprehensive block layer isolation
boundary. It structurally restricts both hardware queue placement and
managed interrupt affinity strictly to housekeeping CPUs, ensuring that no
storage queue operations of any kind are mapped to an isolated CPU.
To achieve this reliably, this series expands the struct irq affinity
structure to incorporate a new CPU mask [1]. This mask is explicitly set to
the result of blk mq online queue affinity. By passing this housekeeping
mask directly through the interrupt affinity parameters, we ensure that the
native affinity calculation is strictly bounded to non isolated CPUs from
the moment the device probes.
This structural enhancement allows device drivers to seamlessly inherit the
isolation constraints without requiring bespoke, driver specific logic. A
clear example of this application can be seen in the modifications to the
Broadcom MPI3 Storage Controller [2]. By leveraging the expanded struct irq
affinity, the driver guarantees that its queues and corresponding managed
interrupts are perfectly aligned with the system housekeeping
configuration, completely avoiding the isolated CPUs during allocation.
[1]: https://lore.kernel.org/lkml/20260401222312.772334-5-atomlin@atomlin.com/
[2]: https://lore.kernel.org/lkml/20260401222312.772334-8-atomlin@atomlin.com/
I hope this better illustrates the mechanical necessity of the io_queue
flag and the corresponding changes to the interrupt affinity structures.
> > Every logical CPU, including the isolated ones, must logically map to a
> > hardware context in order to submit input and output requests, saying they
> > are completely restricted is indeed stale and technically inaccurate. The
> > isolation mechanism actually ensures that the hardware contexts themselves
> > are serviced by the housekeeping CPUs, while the isolated CPUs are simply
> > mapped onto these housekeeping queues for submission purposes. I will
> > rewrite this paragraph to accurately reflect this topology, ensuring it
> > aligns perfectly with the behaviour introduced in patch 10.
>
> I am not sure if the above words is helpful from administrator viewpoint about
> the two kernel parameters.
>
> IMO, only two differences from this viewpoint:
>
> 1) `io_queue` may reduce nr_hw_queues
>
> 2) when application submits IO from isolated CPUs, `io_queue` can complete
> IO from housekeeping CPUs.
Acknowledged.
Kind regards,
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 07/13] libmultipath: Add delayed removal support
From: Hannes Reinecke @ 2026-04-08 15:41 UTC (permalink / raw)
To: John Garry, 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: <74eb1f9b-265e-4264-9575-177de6c924a0@oracle.com>
On 4/8/26 13:28, John Garry wrote:
> 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).
>
That look patently like the 'queue_if_no_path' feature from
dm-multipath. Any chance of reconciling these two?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply
* Re: [PATCH v2 02/10] ublk: add PFN-based buffer matching in I/O path
From: Caleb Sander Mateos @ 2026-04-08 15:28 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block
In-Reply-To: <adW_EQW1AAJdaWqN@fedora>
On Tue, Apr 7, 2026 at 7:36 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> 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.
Accessing the struct ublksrv_io_desc on completion of the I/O is a
likely cache miss, whereas struct ublk_io has just been accessed.
Additionally, struct ublk_queue isn't easily accessible in
ublk_put_req_ref() without going through req->mq_hctx->driver_data.
That's why I'd prefer to use a flag in struct ublk_io.
>
> >
> > > + 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;
Yeah, that looks good. I guess you could also remove base_pfn from
struct ublk_range, and just use mas.index here?
Best,
Caleb
> +
> 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
* 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
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