Re: [PATCH] bcache: improve writeback throughput when frontend I/O is idle

On Sat, Aug 30, 2025 at 02:17:29AM +0800, Coly Li wrote:
> On Fri, Aug 29, 2025 at 03:56:42PM +0800, jifeng zhou wrote:
>> On Fri, 29 Aug 2025 at 00:20, <[email protected]> wrote:
>>> 
>>> Currently in order to write dirty blocks to backend device in LBA order
>>> for better performance, inside write_dirty() the I/O is issued only when
>>> its sequence matches current expected sequence. Otherwise the kworker
>>> will repeat check-wait-woken loop until the sequence number matches.
>>> 
>>> When frontend I/O is idle, the writeback rate is set to INT_MAX, but the
>>> writeback thoughput doesn't increase much. There are two reasons,
>>> - The check-wait-woken loop is inefficient.
>>> - I/O depth on backing device is every low.
>>> 
>>> To improve the writeback throughput, this patch continues to use LBA re-
>>> order idea, but improves it by the following means,
>>> - Do the reorder from write_dirty() to read_dirty().
>>>  Inside read_dirty(), use a min_heap to order all the to-be-writebacked
>>>  keys, and read dirty blocks in LBA order. Although each read requests
>>>  are not completed in issue order, there is no check-wait-woken loop so
>>>  that the dirty blocks are issued in a small time range and they can be
>>>  ordered by I/O schedulers efficiently.
>>> 
>>> - Read more dirty keys when frontend I/O is idle
>>>  Define WRITEBACKS_IN_PASS (5), MAX_WRITEBACKS_IN_PASS (80) for write-
>>>  back dirty keys in each pass, and define WRITESIZE_IN_PASS (5000) and
>>>  MAX_WRITESIZE_IN_PASS (80000) for total writeback data size in each
>>>  pass. When frontend I/O is idle, new values MAX_WRITEBACKS_IN_PASS and
>>>  MAX_WRITESIZE_IN_PASS are used to read more dirty keys and data size
>>>  from cache deice, then more dirty blocks will be written to backend
>>>  device in almost LBA order.
>>> 
>>> By this effort, when there is frontend I/O, the IOPS and latency almost
>>> has no difference observed, identical from previous read_dirty() and
>>> write_dirty() implementation. When frontend I/O is idle, with this patch
>>> the average queue size increases from 2.5 to 21, writeback thoughput on
>>> backing device increases from 12MiB/s to 20MiB/s.
>>> 
>>> Writeback throughput increases around 67% when frontend I/O is idle.
>>> 
>>> Signed-off-by: Coly Li <[email protected]>
>>> ---
>>> drivers/md/bcache/bcache.h    |  1 +
>>> drivers/md/bcache/util.h      |  8 ++++
>>> drivers/md/bcache/writeback.c | 82 +++++++++++++++++------------------
>>> drivers/md/bcache/writeback.h |  6 ++-
>>> 4 files changed, 52 insertions(+), 45 deletions(-)
>>> 

[snipped]
>>> static void read_dirty(struct cached_dev *dc)
>>> {
>>>        unsigned int delay = 0;
>>> -       struct keybuf_key *next, *keys[MAX_WRITEBACKS_IN_PASS], *w;
>>> -       size_t size;
>>> -       int nk, i;
>>> +       struct keybuf_key *next, *w;
>>>        struct dirty_io *io;
>>>        struct closure cl;
>>> -       uint16_t sequence = 0;
>>> +       size_t size;
>>> +       int nk, i;
>>> 
>>>        BUG_ON(!llist_empty(&dc->writeback_ordering_wait.list));
>>> -       atomic_set(&dc->writeback_sequence_next, sequence);
>>>        closure_init_stack(&cl);
>>> 
>>>        /*
>>> @@ -495,46 +481,49 @@ static void read_dirty(struct cached_dev *dc)
>>>        while (!kthread_should_stop() &&
>>>               !test_bit(CACHE_SET_IO_DISABLE, &dc->disk.c->flags) &&
>>>               next) {
>>> +               size_t max_size_in_pass;
>>> +               int max_writebacks_in_pass;
>>> +
>>>                size = 0;
>>>                nk = 0;
>>> +               reset_heap(&dc->read_dirty_heap);
>>> 
>>>                do {
>>>                        BUG_ON(ptr_stale(dc->disk.c, &next->key, 0));
>>> 
>>> +                       if (atomic_read(&dc->disk.c->at_max_writeback_rate)) {
>>> +                               max_writebacks_in_pass = MAX_WRITEBACKS_IN_PASS;
>>> +                               max_size_in_pass = MAX_WRITESIZE_IN_PASS;
>>> +                       } else {
>>> +                               max_writebacks_in_pass = WRITEBACKS_IN_PASS;
>>> +                               max_size_in_pass = WRITESIZE_IN_PASS;
>>> +                       }
>>> +
>>>                        /*
>>>                         * Don't combine too many operations, even if they
>>>                         * are all small.
>>>                         */
>>> -                       if (nk >= MAX_WRITEBACKS_IN_PASS)
>>> +                       if (nk >= max_writebacks_in_pass)
>>>                                break;
>>> 
>>>                        /*
>>>                         * If the current operation is very large, don't
>>>                         * further combine operations.
>>>                         */
>>> -                       if (size >= MAX_WRITESIZE_IN_PASS)
>>> +                       if (size >= max_size_in_pass)
>>>                                break;
>>> 
>>> -                       /*
>>> -                        * Operations are only eligible to be combined
>>> -                        * if they are contiguous.
>>> -                        *
>>> -                        * TODO: add a heuristic willing to fire a
>>> -                        * certain amount of non-contiguous IO per pass,
>>> -                        * so that we can benefit from backing device
>>> -                        * command queueing.
>>> -                        */
>>> -                       if ((nk != 0) && bkey_cmp(&keys[nk-1]->key,
>>> -                                               &START_KEY(&next->key)))
>>> +                       if (!heap_add(&dc->read_dirty_heap, next,
>>> +                                     keybuf_key_cmp))
>>>                                break;
>> 
>> The bkeys retrieved from the dc->writeback_keys rbtree are in a specific order.
>> Can the heap sorting here be omitted?
> 
> Indeed the keys in dc->writeback_keys are not in a specific order, yes they are
> in the order from oldest to newest, but there is *NO* overlap betwen any two
> keys in dc->writeback_keys. See RB_INSERT() inside refill_keybuf_fn().
> 
> Because there is no overlap inside keys of dc->writeback_keys, re-order them by
> LBA incremental order won't make trouble here.
> 

I want to say thank you! I re-read the refill_dirty() implementation, it seems in
read_dirty(), it is unnecessary to call bch_keybuf_del(). Let me try whethere
there is chance to do more improvement.

Coly Li