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

[Jens Axboe <axboe@fb.com>]

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.

-- 
Jens Axboe