Re: [PATCH v6 1/4] block: add task-context bio completion infrastructure

[Tal Zussman]

On 7/28/26 12:10 AM, Christoph Hellwig wrote:
> On Mon, Jun 22, 2026 at 12:45:25PM -0400, Tal Zussman wrote:
>> > 
>> > Any progress here? The patchset looks really promising so I'd love to have
>> > it completed :)
>> > 
>> Sorry for the delay - got caught up with some other work and had to set this
>> aside for a couple weeks, but haven't forgotten about this. Planning to pick
>> it back up some time this week.
> 
> Sorry for another ping, but we have a lot of code waiting on this
> kind of infrastructure now, it would be great to get a version in.
> 

No worries, and apologies again for the delay. I had some deadlines and
travel that kept me busy, but I have some additional results below:

First, I investigated the workload C behavior further. The underlying
problem is that ioends routed to the XFS completion workqueue were also
carrying BIO_COMPLETE_IN_TASK, so unwritten completions bounced through the
block-layer worker (plus its delay) only to be queued on
m_unwritten_workqueue afterwards. The ioend workqueue already provides the
task context the flag asks for, so something like this avoids the bounce:

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -552,8 +552,10 @@ xfs_writeback_submit(
        /*
         * Send ioends that might require a transaction to the completion wq.
         */
-       if (xfs_ioend_needs_wq_completion(ioend))
+       if (xfs_ioend_needs_wq_completion(ioend)) {
                ioend->io_bio.bi_end_io = xfs_end_bio;
+               bio_clear_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
+       }

        return iomap_ioend_writeback_submit(wpc, error);
 }
@@ -664,6 +666,7 @@ xfs_zoned_writeback_submit(
        struct iomap_ioend              *ioend = wpc->wb_ctx;

        ioend->io_bio.bi_end_io = xfs_end_bio;
+       bio_clear_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
        if (error) {
                ioend->io_bio.bi_status = errno_to_blk_status(error);
                bio_endio(&ioend->io_bio);

(Diffs may be whitespace-damaged, for illustrative purpose only)

On a fresh-file run of workload C (every completion doing an unwritten
conversion) this stops conversion-heavy writeback from entering the
block-layer deferral entirely and cuts write p50/p99 by 7-8% (3381 -> 3151us
/ 6761 -> 6193us) at equal bandwidth. I tested a sync waiter workload on a
fresh file too, and it also hit this even harder: a 4k write+fdatasync loop
goes from 2.0 to 17.0 MB/s with this change on top of v6.

I don't love this patch, as it feels a little hacky as currently
constructed, but the perf impact on this worst case is substantial.

I also implemented (a version) of Jan's suggestion of only delaying the
completion work when the offload rate hits a threshold, and tested it
against both delay=1 and delay=0.

We keep a per-CPU count of completions offloaded during the current jiffie
and dispatch the worker immediately unless that count crosses
BIO_COMPLETE_BATCH_RATE (16000/s per CPU, i.e. 16 per jiffie at HZ=1000):

diff --git a/block/bio.c b/block/bio.c
--- a/block/bio.c
+++ b/block/bio.c
@@ -1749,10 +1749,15 @@ void bio_check_pages_dirty(struct bio *bio)
  */
+#define BIO_COMPLETE_BATCH_RATE                16000   /* per second, per CPU */
+#define BIO_COMPLETE_BATCH_THRESH      (BIO_COMPLETE_BATCH_RATE / HZ)

 struct bio_complete_batch {
        struct bio_list list;
        struct delayed_work work;
        int cpu;
+       unsigned long window;   /* jiffie that count refers to */
+       unsigned int count;     /* completions offloaded during window */
 };

@@ -1784,16 +1789,27 @@ void __bio_complete_in_task(struct bio *bio)
 {
        struct bio_complete_batch *batch;
+       unsigned long now = jiffies;
+       unsigned long delay = 0;
        unsigned long flags;
        bool was_empty;

        local_irq_save(flags);
        batch = this_cpu_ptr(&bio_complete_batch);
+
+       if (batch->window != now) {
+               batch->count = 0;
+               batch->window = now;
+       }
+       batch->count++;
+       if (batch->count > BIO_COMPLETE_BATCH_THRESH)
+               delay = 1;
+
        was_empty = bio_list_empty(&batch->list);
        bio_list_add(&batch->list, bio);
        local_irq_restore(flags);

        if (was_empty)
                mod_delayed_work_on(batch->cpu, bio_complete_wq,
-                                   &batch->work, 1);
+                                   &batch->work, delay);
 }
 EXPORT_SYMBOL_GPL(__bio_complete_in_task);


I used 16000/s (at HZ=1000) as the threshold rather than the 1000/s
suggested. For async completions the 1-jiffie delay would at most double the
latency, but a sync waiter that can reach the threshold instead gets stuck
at it. Each deferral adds a jiffie, which drops its rate back below the
threshold, which re-arms a few ms later. This has a significant impact on
the qd1 fdatasync workload below (W1). My main concern is that this turns
out to be device-specific and 16000 is too low for faster drives.

Results are below.
fdatasync latencies are fio's sync percentiles. All three kernels
include the xfs patch above: delay=1 is the current unconditional
delay, delay=0 unconditional immediate dispatch, and gated is the diff above.
C and D are the same workloads as in the previous results.

                                 delay=1     delay=0       gated
W1) 4k write+fdatasync, qd1 (~14k completions/s)
  bandwidth (MB/s)                   2.0        52.6        52.5
  fdatasync p50 (us)              1990.7        69.8        70.1
  fdatasync p99.9 (us)            2965.5       118.3       119.6
  context switches                  212k       4.17M       4.18M
  cycles                          13.8e9     119.3e9     122.2e9

W2) 64k write+fdatasync, qd1 (~10k completions/s)
  bandwidth (MB/s)                  32.7       657.5       655.4
  fdatasync p50 (us)              1974.3        82.8        83.1
  fdatasync p99.9 (us)            2987.3       137.6       136.9
  context switches                  403k       3.30M       3.30M
  cycles                          22.8e9     144.9e9     144.5e9

C) 4k buffered randwrite, qd32 (~118k completions/s)
  bandwidth (MB/s)                 649.5       655.7       643.4
  write lat p50 (us)               215.4       214.7       216.1
  write lat p99.9 (us)             703.1       984.4       730.5
  context switches                20.69M      21.98M      21.44M
  cycles                         922.5e9     838.1e9     911.3e9

D) 64k buffered mixed r/w, qd32 (~11k completions/s)
  bandwidth (MB/s)                 761.2       753.2       757.2
  write lat p50 (us)              2605.1      2637.8      2605.1
  write lat p99.9 (us)            4358.1      4358.1      4358.1
  context switches                 5.99M       6.92M       6.92M
  cycles                         399.0e9     414.3e9     414.4e9

On W1/W2 the delay=1 caps a sync waiter at ~500 syncs/s, since every
fdatasync takes a full jiffie. The gated version never fires on either
workload, so it matches delay=0.

For C, delay=0 regresses write p99.9 by 40%, while the gated version stays
within 4% of delay=1's tail.

D is similar across configurations, but adds +3.9% cycles and +15% context
switches vs. delay=1.

Again, my main concern with this approach is that 16000 (or 1000) is a
somewhat arbitrary value that depends on disk performance. If the C p99.9
perf is an acceptable trade-off, I think we can get rid of the delay, but
I'd appreciate more thoughts on that and the XFS diff above.

I'll send v7 with the rest of the changes (Sashiko review, etc.) in a day or
two, pending any additional feedback.

Thanks,
Tal