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

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(-)
> >
> > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> > index d43fcccf297c..88fb9bb69ce9 100644
> > --- a/drivers/md/bcache/bcache.h
> > +++ b/drivers/md/bcache/bcache.h
> > @@ -345,6 +345,7 @@ struct cached_dev {
> >         struct workqueue_struct *writeback_write_wq;
> >
> >         struct keybuf           writeback_keys;
> > +       DECLARE_HEAP(struct keybuf_key *, read_dirty_heap);
> >
> >         struct task_struct      *status_update_thread;
> >         /*
> > diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
> > index f61ab1bada6c..3f5f85bdeafe 100644
> > --- a/drivers/md/bcache/util.h
> > +++ b/drivers/md/bcache/util.h
> > @@ -46,6 +46,14 @@ struct closure;
> >         (heap)->data;                                                   \
> >  })
> >
> > +#define reset_heap(heap)                                               \
> > +({                                                                     \
> > +       size_t _bytes;                                                  \
> > +       _bytes = (heap)->size * sizeof(*(heap)->data);                  \
> > +       memset((heap)->data, 0, _bytes);                                \
> > +       (heap)->used = 0;                                               \
> > +})
> > +
> >  #define free_heap(heap)                                                        \
> >  do {                                                                   \
> >         kvfree((heap)->data);                                           \
> > diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> > index 302e75f1fc4b..4f0e47c841aa 100644
> > --- a/drivers/md/bcache/writeback.c
> > +++ b/drivers/md/bcache/writeback.c
> > @@ -406,26 +406,6 @@ static CLOSURE_CALLBACK(write_dirty)
> >         struct keybuf_key *w = io->bio.bi_private;
> >         struct cached_dev *dc = io->dc;
> >
> > -       uint16_t next_sequence;
> > -
> > -       if (atomic_read(&dc->writeback_sequence_next) != io->sequence) {
> > -               /* Not our turn to write; wait for a write to complete */
> > -               closure_wait(&dc->writeback_ordering_wait, cl);
> > -
> > -               if (atomic_read(&dc->writeback_sequence_next) == io->sequence) {
> > -                       /*
> > -                        * Edge case-- it happened in indeterminate order
> > -                        * relative to when we were added to wait list..
> > -                        */
> > -                       closure_wake_up(&dc->writeback_ordering_wait);
> > -               }
> > -
> > -               continue_at(cl, write_dirty, io->dc->writeback_write_wq);
> > -               return;
> > -       }
> > -
> > -       next_sequence = io->sequence + 1;
> > -
> >         /*
> >          * IO errors are signalled using the dirty bit on the key.
> >          * If we failed to read, we should not attempt to write to the
> > @@ -443,7 +423,6 @@ static CLOSURE_CALLBACK(write_dirty)
> >                 closure_bio_submit(io->dc->disk.c, &io->bio, cl);
> >         }
> >
> > -       atomic_set(&dc->writeback_sequence_next, next_sequence);
> >         closure_wake_up(&dc->writeback_ordering_wait);
> >
> >         continue_at(cl, write_dirty_finish, io->dc->writeback_write_wq);
> > @@ -471,18 +450,25 @@ static CLOSURE_CALLBACK(read_dirty_submit)
> >         continue_at(cl, write_dirty, io->dc->writeback_write_wq);
> >  }
> >
> > +static uint64_t keybuf_key_cmp(const struct keybuf_key *l,
> > +                              const struct keybuf_key *r)
> > +{
> > +       if (unlikely((KEY_INODE(&l->key) != KEY_INODE(&r->key))))
> > +               return KEY_INODE(&l->key) > KEY_INODE(&r->key);
> > +       else
> > +               return KEY_OFFSET(&l->key) > KEY_OFFSET(&r->key);
> > +}
> > +
> >  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.

Thanks.

Coly Li