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

[David Sterba <dsterba@suse.cz>]

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.