Re: [PATCH] cfq-iosched: NCQ SSDs do not need read queue merging

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Jan 11, 2010 at 03:53:00PM +0100, Corrado Zoccolo wrote:
> On Mon, Jan 11, 2010 at 2:18 PM, Jeff Garzik <jeff@garzik.org> wrote:
> > On 01/11/2010 08:13 AM, Jens Axboe wrote:
> >>
> >> On Mon, Jan 11 2010, Corrado Zoccolo wrote:
> >>>
> >>> On Mon, Jan 11, 2010 at 12:25 PM, Jeff Garzik<jeff@garzik.org>  wrote:
> >>>>
> >>>> On 01/10/2010 04:04 PM, Corrado Zoccolo wrote:
> >>>>>
> >>>>> NCQ SSDs' performances are not affected by
> >>>>> distance of read requests, so there is no point in having
> >>>>> overhead to merge such queues.
> >>>>>
> >>>>> Non-NCQ SSDs showed regression in some special cases, so
> >>>>> they are ruled out by this patch.
> >>>>>
> >>>>> This patch intentionally doesn't affect writes, so
> >>>>> it changes the queued[] field, to be indexed by
> >>>>> READ/WRITE instead of SYNC/ASYNC, and only compute proximity
> >>>>> for queues with WRITE requests.
> >>>>>
> >>>>> Signed-off-by: Corrado Zoccolo<czoccolo@gmail.com>
> >>>>
> >>>> That's not really true.  Overhead always increases as the total number
> >>>> of
> >>>> ATA commands issued increases.
> >>>
> >>> Jeff Moyer tested the patch on the workload that mostly benefit of
> >>> queue merging, and found that
> >>> the performance was improved by the patch.
> >>> So removing the CPU overhead helps much more than the marginal gain
> >>> given by merging on this hardware.
> >>
> >> It's not always going to be true. On SATA the command overhead is fairly
> >> low, but on other hardware that may not be the case. Unless you are CPU
> >> bound by your IO device, then merging will always be beneficial. I'm a
> >> little behind on emails after my vacation, Jeff what numbers did you
> >> generate and on what hardware?
> >
> >  ...and on what workload?   "the workload that mostly benefit of queue
> > merging" is highly subjective, and likely does not cover most workloads SSDs
> > will see in the field.
> Hi Jeff,
> exactly.
> The workloads that benefits from queue merging are the ones in which a
> sequential
> read is actually splitt, and carried out by different processes in
> different I/O context, each
> sending requests with strides. This is clearly not the best way of
> doing sequential access
> (I would happily declare those programs as buggy).
> CFQ has code that merges queues in this case. I'm disabling the READ
> part for NCQ SSDs,
> since, as Jeff measured, the code overhead outweight the gain from
> merging (if any).

Hi Corrado,

In Jeff's test case of running read-test2, I am not even sure if any
merging between the queues took place or not as on NCQ SSD, we are driving
deeper queue depths and unless read-test2 is creating more than 32
threads, there might not be any merging taking place at all.

We also don't have any data/numbers what kind of cpu savings does this
patch bring in.

Vivek

> 
> As you said, most workloads don't benefit from queue merging. On those
> ones, the patch
> just removes an overhead.
> 
> Thanks,
> Corrado
> 
> >        Jeff
> >
> >
> >
> >