Re: [PATCH] btrfs-progs: scrub: warn if scrub started on a device has mq-deadline

[Graham Cobb <g.btrfs@cobb.uk.net>]

On 11/12/2020 16:42, Johannes Thumshirn wrote:
> On 11/12/2020 17:02, Johannes Thumshirn wrote:
>> [+Cc Damien ]
>> On 11/12/2020 16:55, David Sterba wrote:
>>> On Fri, Dec 11, 2020 at 06:50:23AM +0000, Johannes Thumshirn wrote:
>>>> On 10/12/2020 21:22, David Sterba wrote:
>>>>> On Mon, Dec 07, 2020 at 07:23:03AM +0000, Johannes Thumshirn wrote:
>>>>>> On 05/12/2020 19:51, Sidong Yang wrote:
>>>>>>> Warn if scurb stared on a device that has mq-deadline as io-scheduler
>>>>>>> and point documentation. mq-deadline doesn't work with ionice value and
>>>>>>> it results performance loss. This warning helps users figure out the
>>>>>>> situation. This patch implements the function that gets io-scheduler
>>>>>>> from sysfs and check when scrub stars with the function.
>>>>>>
>>>>>> From a quick grep it seems to me that only bfq is supporting ioprio settings.
>>>>>
>>>>> Yeah it's only BFQ.
>>>>>
>>>>>> Also there's some features like write ordering guarantees that currently 
>>>>>> only mq-deadline provides.
>>>>>>
>>>>>> This warning will trigger a lot once the zoned patchset for btrfs is merged,
>>>>>> as for example SMR drives need this ordering guarantees and therefore select
>>>>>> mq-deadline (via the ELEVATOR_F_ZBD_SEQ_WRITE elevator feature).
>>>>>
>>>>> This won't affect the default case and for zoned fs we can't simply use
>>>>> BFQ and thus the ionice interface. Which should be IMHO acceptable.
>>>>
>>>> But then the patch must check if bfq is set and otherwise warn. My only fear
>>>> is though, people will blindly select bfq then and get all kinds of 
>>>> penalties/problems.
>>>
>>> Hm right, and we know that once such recommendations stick, it's very
>>> hard to get rid of them even if there's a proper solution implemented.
>>>
>>>> And it's not only mq-deadline and bfq here, there are also
>>>> kyber and none. On a decent nvme I'd like to have none instead of bfq, otherwise
>>>> performance goes down the drain. If my workload is sensitive to buffer bloat, I'd
>>>> use kyber not bfq.
>>>
>>> I'm not sure about high performance devices if the scrub io load on the
>>> normal priority is the same problem as with spinning devices. Assuming
>>> it is, we need some low priority/idle class for all schedulers at least.
>>>
>>>> So IMHO this patch just makes things worse. But who am I to judge.
>>>
>>> In this situation we need broader perspective, thanks for the input,
>>> we'll hopefully come to some conclusion. We don't want to make things
>>> worse, now we're left with workarounds or warnings until some brave soul
>>> implements the missing io scheduler functionality.
>>>
>>
>> I think that's all we can do now.
>>
>> Damien (CCed) did some work on mq-deadline, maybe he has an idea if this is
>> possible/doable.
>>
> 
> Ha I have a stop gap solution, we could temporarily change the io scheduler to bfq
> while the scrub job is running and then back to whatever was configured.
> 
> Of cause only if bfq supports the elevator_features the block device requires.
> 
> Thoughts?
> 

I would prefer you didn't mess with the scheduler I have chosen (or only
if asked to with a new option). My suggestion:

1. If -c or -n have been specified explicitly, check the scheduler and
error if it is known that it is incompatible.

2. If -c/-n have not been specified, just warn (not fail) if the
scheduler is known to be incompatible with the default idle class request.

3. Provide a way to suppress the warning in step 2 (is -q enough, or
does that also suppress other useful/important messages?).

4. Expand the description in the man page which currently says "Note
that not all IO schedulers honor the ionice settings." It could read
something like:

"Note that not all IO schedulers honor the ionice settings. At time of
writing, only bfq honors the ionice settings although newer kernels may
change that. A warning will be shown if the currently selected IO
scheduler is known to not support ionice."