Re: [PATCH] block: check more requests for multiple_queues in blk_attempt_plug_merge

[Ming Lei <ming.lei@redhat.com>]

On Thu, Mar 10, 2022 at 06:21:33PM -0700, Jens Axboe wrote:
> On 3/10/22 6:14 PM, Ming Lei wrote:
> > On Thu, Mar 10, 2022 at 05:36:44PM -0700, Jens Axboe wrote:
> >> On 3/10/22 5:31 PM, Song Liu wrote:
> >>> On Thu, Mar 10, 2022 at 4:07 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>
> >>>> On 3/10/22 4:33 PM, Song Liu wrote:
> >>>>> On Thu, Mar 10, 2022 at 3:02 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>>
> >>>>>> On 3/10/22 3:37 PM, Song Liu wrote:
> >>>>>>> On Thu, Mar 10, 2022 at 2:15 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>>>>
> >>>>>>>> On 3/8/22 11:42 PM, Song Liu wrote:
> >>>>>>>>> RAID arrays check/repair operations benefit a lot from merging requests.
> >>>>>>>>> If we only check the previous entry for merge attempt, many merge will be
> >>>>>>>>> missed. As a result, significant regression is observed for RAID check
> >>>>>>>>> and repair.
> >>>>>>>>>
> >>>>>>>>> Fix this by checking more than just the previous entry when
> >>>>>>>>> plug->multiple_queues == true.
> >>>>>>>>>
> >>>>>>>>> This improves the check/repair speed of a 20-HDD raid6 from 19 MB/s to
> >>>>>>>>> 103 MB/s.
> >>>>>>>>
> >>>>>>>> Do the underlying disks not have an IO scheduler attached? Curious why
> >>>>>>>> the merges aren't being done there, would be trivial when the list is
> >>>>>>>> flushed out. Because if the perf difference is that big, then other
> >>>>>>>> workloads would be suffering they are that sensitive to being within a
> >>>>>>>> plug worth of IO.
> >>>>>>>
> >>>>>>> The disks have mq-deadline by default. I also tried kyber, the result
> >>>>>>> is the same. Raid repair work sends IOs to all the HDDs in a
> >>>>>>> round-robin manner. If we only check the previous request, there isn't
> >>>>>>> much opportunity for merge. I guess other workloads may have different
> >>>>>>> behavior?
> >>>>>>
> >>>>>> Round robin one at the time? I feel like there's something odd or
> >>>>>> suboptimal with the raid rebuild, if it's that sensitive to plug
> >>>>>> merging.
> >>>>>
> >>>>> It is not one request at a time, but more like (for raid456):
> >>>>>    read 4kB from HDD1, HDD2, HDD3...,
> >>>>>    then read another 4kB from HDD1, HDD2, HDD3, ...
> >>>>
> >>>> Ehm, that very much looks like one-at-the-time from each drive, which is
> >>>> pretty much the worst way to do it :-)
> >>>>
> >>>> Is there a reason for that? Why isn't it using 64k chunks or something
> >>>> like that? You could still do that as a kind of read-ahead, even if
> >>>> you're still processing in chunks of 4k.
> >>>
> >>> raid456 handles logic in the granularity of stripe. Each stripe is 4kB from
> >>> every HDD in the array. AFAICT, we need some non-trivial change to
> >>> enable the read ahead.
> >>
> >> Right, you'd need to stick some sort of caching in between so instead of
> >> reading 4k directly, you ask the cache for 4k and that can manage
> >> read-ahead.
> >>
> >>>>>> Plug merging is mainly meant to reduce the overhead of merging,
> >>>>>> complement what the scheduler would do. If there's a big drop in
> >>>>>> performance just by not getting as efficient merging on the plug side,
> >>>>>> that points to an issue with something else.
> >>>>>
> >>>>> We introduced blk_plug_max_rq_count() to give md more opportunities to
> >>>>> merge at plug side, so I guess the behavior has been like this for a
> >>>>> long time. I will take a look at the scheduler side and see whether we
> >>>>> can just merge later, but I am not very optimistic about it.
> >>>>
> >>>> Yeah I remember, and that also kind of felt like a work-around for some
> >>>> underlying issue. Maybe there's something about how the IO is issued
> >>>> that makes it go straight to disk and we never get any merging? Is it
> >>>> because they are sync reads?
> >>>>
> >>>> In any case, just doing larger reads would likely help quite a bit, but
> >>>> would still be nice to get to the bottom of why we're not seeing the
> >>>> level of merging we expect.
> >>>
> >>> Let me look more into this. Maybe we messed something up in the
> >>> scheduler.
> >>
> >> I'm assuming you have a plug setup for doing the reads, which is why you
> >> see the big difference (or there would be none). But
> >> blk_mq_flush_plug_list() should really take care of this when the plug
> >> is flushed, requests should be merged at that point. And from your
> >> description, doesn't sound like they are at all.
> > 
> > requests are shared, when running out of request, plug list will be
> > flushed early.
> 
> That is true, but I don't think that's the problem here with the round
> robin approach. Seems like it'd drive a pretty low queue depth, even
> considering SATA.

Another one may be plug list not sorted before inserting requests to
scheduler in blk_mq_flush_plug_list(), looks you have mentioned.

Thanks,
Ming