Re: [PATCH 3/3] block: by default, limit maximum discard size to 64MB

[Mike Snitzer <snitzer@redhat.com>]

On Wed, Jul 15 2015 at  6:14pm -0400,
Jens Axboe <axboe@fb.com> wrote:

> On 07/15/2015 10:29 AM, Mike Snitzer wrote:
> >On Wed, Jul 15 2015 at 11:30am -0400,
> >Jens Axboe <axboe@fb.com> wrote:
> >
> >>On 07/15/2015 05:46 AM, Austin S Hemmelgarn wrote:
> >>>On 2015-07-14 17:48, Jens Axboe wrote:
> >>>>On 07/14/2015 02:45 PM, Jens Axboe wrote:
> >>>>>On 07/14/2015 02:44 PM, Mike Snitzer wrote:
> >>>>>>On Tue, Jul 14 2015 at  2:48pm -0400,
> >>>>>>Jens Axboe <axboe@fb.com> wrote:
> >>>>>>
> >>>>>>>Lots of devices exhibit very high latencies for big discards, hurting
> >>>>>>>reads and writes. By default, limit the max discard we will build to
> >>>>>>>64MB. This value has shown good results across a number of devices.
> >>>>>>>
> >>>>>>>This will potentially hurt discard throughput, from a provisioning
> >>>>>>>point of view (when the user does mkfs.xfs, for instance, and mkfs
> >>>>>>>issues a full device discard). If that becomes an issue, we could
> >>>>>>>have different behavior for provisioning vs runtime discards.
> >>>>>>>
> >>>>>>>Signed-off-by: Jens Axboe <axboe@fb.com>
> >>>>>>
> >>>>>>Christoph suggested you impose this default for the specific
> >>>>>>drivers/devices that benefit.  I'm not following why imposing a 64MB
> >>>>>>default is the right thing to do for all devices.
> >>>>>
> >>>>>I'd argue that's most of them... But the testing we did was on NVMe. I
> >>>>>can limit it to NVMe, no big deal.
> >>>>
> >>>>Oh, and LSI flash too, so not just NVMe.
> >>>>
> >>>While I don't have time to test it, I have a feeling that such a limit
> >>>would help with many of the consumer SSD's out there.  Secondarily, once
> >>>this gets in and discard is fixed for BTRFS, I'll have some performance
> >>>testing to do WRT dm-thinp.
> >>
> >>Right, that was the point of it. After more consideration, a default
> >>"sane" limit should be applied to any non-stacked device.
> >
> >Sounds good.
> >
> >For DM thinp, it can handle really large discards efficiently (without
> >passing discards down to the underlying data device).  But if/when
> >discard passdown is enabled it'll obviously split those larger discards
> >based on this new "sane" limit of the underlying data device.
> >
> >Which would then potentially usher in the problem of discard latency
> >being high for DM thinp (if discard passdown is enabled).  But in
> >practice I doubt that will be much of a concern.  I'll keep both pieces
> >if I'm wrong ;)
> 
> Lets focus on patch 1+2 for now, so I can queue those up.
> Acked/reviewed-by's welcome. Then we can tackle the "what is a sane
> default and for whom" patch 3 later, it's orthogonal to exposing the
> knob.

Hey Jens,

I happened upon the 2 patches you staged for 4.3:
http://git.kernel.dk/?p=linux-block.git;a=commit;h=2bb4cd5cc472b191a46938becb7dafdd44644329
http://git.kernel.dk/?p=linux-block.git;a=commit;h=0034af036554c39eefd14d835a8ec3496ac46712

I noticed that none of DM is touched by the first wrapper patch
(e.g. thin_io_hints should use it).  I guess I should go through and
prepare DM changes for 4.3 that go along with your patches (rebase
dm-4.3 ontop of your for-4.3-core)?

But that aside, I also checked with Joe Thornber about your desire to
have the soft limit default to 64MB.  Joe said he'd be fine with that;
so you could go ahead and queue up that change if you like.

Mike