Re: [PATCH 4/5] bcache: writeback: collapse contiguous IO better

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I just fixed the problem you pointed out, and ran a test where I did

sync;fio --randrepeat=1 --ioengine=libaio --direct=1 --gtod_reduce=1
--name=test --filename=test --bs=8k --iodepth=256 --size=25G
--readwrite=randwrite --ramp_time=4

to generate a lot of 8k extents.

After letting things quiet down, writeback looks like this:

$ iostat 1 | grep sdb
sdb               3.00         0.00        40.00          0         40
sdb               0.00         0.00         0.00          0          0
sdb               0.00         0.00         0.00          0          0
sdb               2.00         0.00        16.00          0         16
sdb               0.00         0.00         0.00          0          0
sdb               3.00         0.00        40.00          0         40
sdb               0.00         0.00         0.00          0          0
sdb               0.00         0.00         0.00          0          0
sdb               3.00         0.00        40.00          0         40

So this is proven to be nicely grouping contiguous extents for
writeback in less frequent, mostly-40k chunks.

Mike

On Wed, Sep 27, 2017 at 12:47 AM, Michael Lyle <mlyle@xxxxxxxx> wrote:
> Tang--
>
> This is a first step towards further stuff I want to do--
>
> 1. I want to allow blk_plug-- but to do that, a request needs to know
> there are subsequent contiguous requests after it when issuing the
> write.  The new structure allows that.
> 2. I want to allow tuning to issue multiple requests and control IO
> priorities for them, so that we can make use of queue depth on backing
> devices.  The new code structure allows for inserting heuristics to do
> that very easily.  When 4 operations are issued at a time, latency
> doesn't suffer very much and throughput can be 30-40% higher.
> 3. There's a small change to the delay calculation that will allow the
> actual inner delay controller to be improved to do lower rates for
> laptops and allow backing disk spindown.
>
> But I need more testing and more time to complete those, and there's
> already a benefit with the current structure.
>
> Mike
>
> On Wed, Sep 27, 2017 at 12:32 AM,  <tang.junhui@xxxxxxxxxx> wrote:
>> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>>
>> Hello Mike:
>>
>> For the second question, I thinks this modification is somewhat complex,
>> cannot we do something simple to resolve it? I remember there were some
>> patches trying to avoid too small writeback rate, Coly, is there any
>> progress now?
>>
>> -------
>> Tang Junhui
>>
>>> Ah-- re #1 -- I was investigating earlier why not as much was combined
>>> as I thought should be when idle.  This is surely a factor.  Thanks
>>> for the catch-- KEY_OFFSET is correct.  I will fix and retest.
>>>
>>> (Under heavy load, the correct thing still happens, but not under
>>> light or intermediate load0.
>>>
>>> About #2-- I wanted to attain a bounded amount of "combining" of
>>> operations.  If we have 5 4k extents in a row to dispatch, it seems
>>> really wasteful to issue them as 5 IOs 60ms apart, which the existing
>>> code would be willing to do-- I'd rather do a 20k write IO (basically
>>> the same cost as a 4k write IO) and then sleep 300ms.  It is dependent
>>> on the elevator/IO scheduler merging the requests.  At the same time,
>>> I'd rather not combine a really large request.
>>>
>>> It would be really neat to blk_plug the backing device during the
>>> write issuance, but that requires further work.
>>>
>>> Thanks
>>>
>>> Mike
>>>
>>> On Tue, Sep 26, 2017 at 11:51 PM,  <tang.junhui@xxxxxxxxxx> wrote:
>>> > From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>>> >
>>> > Hello Lyle:
>>> >
>>> > Two questions:
>>> > 1) In keys_contiguous(), you judge I/O contiguous in cache device, but not
>>> > in backing device. I think you should judge it by backing device (remove
>>> > PTR_CACHE() and use KEY_OFFSET() instead of PTR_OFFSET()?).
>>> >
>>> > 2) I did not see you combine samll contiguous I/Os to big I/O, so I think
>>> > it is useful when writeback rate was low by avoiding single I/O write, but
>>> > have no sense in high writeback rate, since previously it is also write
>>> > I/Os asynchronously.
>>> >
>>> > -----------
>>> > Tang Junhui
>>> >
>>> >> Previously, there was some logic that attempted to immediately issue
>>> >> writeback of backing-contiguous blocks when the writeback rate was
>>> >> fast.
>>> >>
>>> >> The previous logic did not have any limits on the aggregate size it
>>> >> would issue, nor the number of keys it would combine at once.  It
>>> >> would also discard the chance to do a contiguous write when the
>>> >> writeback rate was low-- e.g. at "background" writeback of target
>>> >> rate = 8, it would not combine two adjacent 4k writes and would
>>> >> instead seek the disk twice.
>>> >>
>>> >> This patch imposes limits and explicitly understands the size of
>>> >> contiguous I/O during issue.  It also will combine contiguous I/O
>>> >> in all circumstances, not just when writeback is requested to be
>>> >> relatively fast.
>>> >>
>>> >> It is a win on its own, but also lays the groundwork for skip writes to
>>> >> short keys to make the I/O more sequential/contiguous.
>>> >>
>>> >> Signed-off-by: Michael Lyle <mlyle@xxxxxxxx>
>>> >> ---
>>> >>  drivers/md/bcache/bcache.h    |   6 --
>>> >>  drivers/md/bcache/writeback.c | 131 ++++++++++++++++++++++++++++++------------
>>> >>  2 files changed, 93 insertions(+), 44 deletions(-)
>>> >>
>>> >> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
>>> >> index eb83be693d60..da803a3b1981 100644
>>> >> --- a/drivers/md/bcache/bcache.h
>>> >> +++ b/drivers/md/bcache/bcache.h
>>> >> @@ -321,12 +321,6 @@ struct cached_dev {
>>> >>                struct bch_ratelimit            writeback_rate;
>>> >>                struct delayed_work             writeback_rate_update;
>>> >>
>>> >> -              /*
>>> >> -               * Internal to the writeback code, so read_dirty() can keep track of
>>> >> -               * where it's at.
>>> >> -               */
>>> >> -              sector_t                                last_read;
>>> >> -
>>> >>                /* Limit number of writeback bios in flight */
>>> >>                struct semaphore                in_flight;
>>> >>                struct task_struct              *writeback_thread;
>>> >> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
>>> >> index 0b7c89813635..cf29c44605b9 100644
>>> >> --- a/drivers/md/bcache/writeback.c
>>> >> +++ b/drivers/md/bcache/writeback.c
>>> >> @@ -229,10 +229,26 @@ static void read_dirty_submit(struct closure *cl)
>>> >>                continue_at(cl, write_dirty, io->dc->writeback_write_wq);
>>> >>  }
>>> >>
>>> >> +static inline bool keys_contiguous(struct cached_dev *dc,
>>> >> +                              struct keybuf_key *first, struct keybuf_key *second)
>>> >> +{
>>> >> +              if (PTR_CACHE(dc->disk.c, &second->key, 0)->bdev !=
>>> >> +                                              PTR_CACHE(dc->disk.c, &first->key, 0)->bdev)
>>> >> +                              return false;
>>> >> +
>>> >> +              if (PTR_OFFSET(&second->key, 0) !=
>>> >> +                                              (PTR_OFFSET(&first->key, 0) + KEY_SIZE(&first->key)))
>>> >> +                              return false;
>>> >> +
>>> >> +              return true;
>>> >> +}
>>> >> +
>>> >>  static void read_dirty(struct cached_dev *dc)
>>> >>  {
>>> >>                unsigned delay = 0;
>>> >> -              struct keybuf_key *w;
>>> >> +              struct keybuf_key *next, *keys[5], *w;
>>> >> +              size_t size;
>>> >> +              int nk, i;
>>> >>                struct dirty_io *io;
>>> >>                struct closure cl;
>>> >>
>>> >> @@ -243,45 +259,84 @@ static void read_dirty(struct cached_dev *dc)
>>> >>                 * mempools.
>>> >>                 */
>>> >>
>>> >> -              while (!kthread_should_stop()) {
>>> >> -
>>> >> -                              w = bch_keybuf_next(&dc->writeback_keys);
>>> >> -                              if (!w)
>>> >> -                                              break;
>>> >> -
>>> >> -                              BUG_ON(ptr_stale(dc->disk.c, &w->key, 0));
>>> >> -
>>> >> -                              if (KEY_START(&w->key) != dc->last_read ||
>>> >> -                                  jiffies_to_msecs(delay) > 50)
>>> >> -                                              while (!kthread_should_stop() && delay)
>>> >> -                                                              delay = schedule_timeout_interruptible(delay);
>>> >> -
>>> >> -                              dc->last_read           = KEY_OFFSET(&w->key);
>>> >> -
>>> >> -                              io = kzalloc(sizeof(struct dirty_io) + sizeof(struct bio_vec)
>>> >> -                                                   * DIV_ROUND_UP(KEY_SIZE(&w->key), PAGE_SECTORS),
>>> >> -                                                   GFP_KERNEL);
>>> >> -                              if (!io)
>>> >> -                                              goto err;
>>> >> -
>>> >> -                              w->private              = io;
>>> >> -                              io->dc                          = dc;
>>> >> -
>>> >> -                              dirty_init(w);
>>> >> -                              bio_set_op_attrs(&io->bio, REQ_OP_READ, 0);
>>> >> -                              io->bio.bi_iter.bi_sector = PTR_OFFSET(&w->key, 0);
>>> >> -                              bio_set_dev(&io->bio, PTR_CACHE(dc->disk.c, &w->key, 0)->bdev);
>>> >> -                              io->bio.bi_end_io               = read_dirty_endio;
>>> >> -
>>> >> -                              if (bio_alloc_pages(&io->bio, GFP_KERNEL))
>>> >> -                                              goto err_free;
>>> >> -
>>> >> -                              trace_bcache_writeback(&w->key);
>>> >> +              next = bch_keybuf_next(&dc->writeback_keys);
>>> >> +
>>> >> +              while (!kthread_should_stop() && next) {
>>> >> +                              size = 0;
>>> >> +                              nk = 0;
>>> >> +
>>> >> +                              do {
>>> >> +                                              BUG_ON(ptr_stale(dc->disk.c, &next->key, 0));
>>> >> +
>>> >> +                                              /* Don't combine too many operations, even if they
>>> >> +                                               * are all small.
>>> >> +                                               */
>>> >> +                                              if (nk >= 5)
>>> >> +                                                              break;
>>> >> +
>>> >> +                                              /* If the current operation is very large, don't
>>> >> +                                               * further combine operations.
>>> >> +                                               */
>>> >> +                                              if (size > 5000)
>>> >> +                                                              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 && !keys_contiguous(dc, keys[nk-1], next))
>>> >> +                                                              break;
>>> >> +
>>> >> +                                              size += KEY_SIZE(&next->key);
>>> >> +                                              keys[nk++] = next;
>>> >> +                              } while ((next = bch_keybuf_next(&dc->writeback_keys)));
>>> >> +
>>> >> +                              /* Now we have gathered a set of 1..5 keys to write back. */
>>> >> +
>>> >> +                              for (i = 0; i < nk; i++) {
>>> >> +                                              w = keys[i];
>>> >> +
>>> >> +                                              io = kzalloc(sizeof(struct dirty_io) +
>>> >> +                                                                   sizeof(struct bio_vec) *
>>> >> +                                                                   DIV_ROUND_UP(KEY_SIZE(&w->key), PAGE_SECTORS),
>>> >> +                                                                   GFP_KERNEL);
>>> >> +                                              if (!io)
>>> >> +                                                              goto err;
>>> >> +
>>> >> +                                              w->private              = io;
>>> >> +                                              io->dc                          = dc;
>>> >> +
>>> >> +                                              dirty_init(w);
>>> >> +                                              bio_set_op_attrs(&io->bio, REQ_OP_READ, 0);
>>> >> +                                              io->bio.bi_iter.bi_sector = PTR_OFFSET(&w->key, 0);
>>> >> +                                              bio_set_dev(&io->bio,
>>> >> +                                                                  PTR_CACHE(dc->disk.c, &w->key, 0)->bdev);
>>> >> +                                              io->bio.bi_end_io               = read_dirty_endio;
>>> >> +
>>> >> +                                              if (bio_alloc_pages(&io->bio, GFP_KERNEL))
>>> >> +                                                              goto err_free;
>>> >> +
>>> >> +                                              trace_bcache_writeback(&w->key);
>>> >> +
>>> >> +                                              down(&dc->in_flight);
>>> >> +
>>> >> +                                              /* We've acquired a semaphore for the maximum
>>> >> +                                               * simultaneous number of writebacks; from here
>>> >> +                                               * everything happens asynchronously.
>>> >> +                                               */
>>> >> +                                              closure_call(&io->cl, read_dirty_submit, NULL, &cl);
>>> >> +                              }
>>> >>
>>> >> -                              down(&dc->in_flight);
>>> >> -                              closure_call(&io->cl, read_dirty_submit, NULL, &cl);
>>> >> +                              delay = writeback_delay(dc, size);
>>> >>
>>> >> -                              delay = writeback_delay(dc, KEY_SIZE(&w->key));
>>> >> +                              while (!kthread_should_stop() && delay) {
>>> >> +                                              schedule_timeout_interruptible(delay);
>>> >> +                                              delay = writeback_delay(dc, 0);
>>> >> +                              }
>>> >>                }
>>> >>
>>> >>                if (0) {
>>> >> --
>>> > --
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux ARM Kernel]     [Linux Filesystem Development]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux