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

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

[Sidong Yang]

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.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 cmds/scrub.c          |  8 ++++++++
 common/device-utils.c | 28 ++++++++++++++++++++++++++++
 common/device-utils.h |  1 +
 3 files changed, 37 insertions(+)

diff --git a/cmds/scrub.c b/cmds/scrub.c
index e96dc998..1932b097 100644
--- a/cmds/scrub.c
+++ b/cmds/scrub.c
@@ -40,6 +40,7 @@
 #include "kernel-shared/ctree.h"
 #include "ioctl.h"
 #include "common/utils.h"
+#include "common/device-utils.h"
 #include "kernel-shared/volumes.h"
 #include "kernel-shared/disk-io.h"
 
@@ -1188,6 +1189,7 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv,
 	DIR *dirstream = NULL;
 	int force = 0;
 	int nothing_to_resume = 0;
+	char scheduler_name[256];
 
 	while ((c = getopt(argc, argv, "BdqrRc:n:f")) != -1) {
 		switch (c) {
@@ -1314,6 +1316,12 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv,
 
 	for (i = 0; i < fi_args.num_devices; ++i) {
 		devid = di_args[i].devid;
+		if (!btrfs_io_scheduler((char*)di_args[i].path, scheduler_name, 256))
+			if (!strcmp(scheduler_name, "mq-deadline"))
+				warning_on(!do_quiet,
+					   "ionice doesn't work with current "
+					   "mq-deadline scheduler "
+					   "(see btrfs-scrub(8) manpage)");
 		ret = pthread_mutex_init(&sp[i].progress_mutex, NULL);
 		if (ret) {
 			errno = ret;
diff --git a/common/device-utils.c b/common/device-utils.c
index c860b946..3cd5ca4a 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -252,3 +252,31 @@ u64 get_partition_size(const char *dev)
 	return result;
 }
 
+int btrfs_io_scheduler(const char *dev, char *scheduler, int max_len)
+{
+	char path[PATH_MAX];
+	char names[256];
+	FILE *file;
+	char fmt[20];
+	char *this_char, *save_ptr;
+
+	snprintf(path, PATH_MAX, "/sys/block/%s/queue/scheduler",
+		 basename(dev));
+	if ((file = fopen(path, "r"))) {
+		if (fgets(names, 255, file)) {
+			for (this_char = strtok_r(names, " ", &save_ptr);
+			     this_char != NULL;
+			     this_char = strtok_r(NULL, " ", &save_ptr)) {
+				snprintf(fmt, 20, "[%%%i[a-z-]", max_len - 1);
+				if (sscanf(this_char, fmt, scheduler)) {
+					fclose(file);
+					return 0;
+				}
+			}
+
+		}
+		fclose(file);
+	}
+
+	return -1;
+}
diff --git a/common/device-utils.h b/common/device-utils.h
index 70d19cae..bd892d54 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -29,5 +29,6 @@ u64 disk_size(const char *path);
 u64 btrfs_device_size(int fd, struct stat *st);
 int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
 		u64 max_block_count, unsigned opflags);
+int btrfs_io_scheduler(const char *dev, char *scheduler, int max_len);
 
 #endif
-- 
2.25.1

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

[Johannes Thumshirn]

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.
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).

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

[David Sterba]

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.

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

[Johannes Thumshirn]

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. 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.

So IMHO this patch just makes things worse. But who am I to judge.

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

[David Sterba]

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.

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

[Johannes Thumshirn]

[+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.

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

[Johannes Thumshirn]

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?

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

[Graham Cobb]

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."

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

[Johannes Thumshirn]

On 11/12/2020 18:05, Graham Cobb wrote:
> 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."
> 
> 


Works for me as well. My only concern is, people will default to bfq and then 
complain for XY because of the change to bfq. Note, I'm not against bfq at all,
I'm just arguing it's not a one size fits all decision.

I'd also like to see if scrub on a nvme really is beneficial with bfq instead of
none. I have my doubt's but that's a gut feeling and I have no numbers to back up
this statement.

Nice weekend,
	Johannes

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

[Damien Le Moal]

On Sat, 2020-12-12 at 10:34 +0000, Johannes Thumshirn wrote:
> On 11/12/2020 18:05, Graham Cobb wrote:
> > 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.

Elevator not compatible with zoned block devices will not even be shown in
sysfs. E.g. bfq is not even listed for SMR drives. So the user/FS cannot select
a wrong scheduler beside "none". For SMR, that currently means deadline only.

For ZNS, the ELEVATOR_F_ZBD_SEQ_WRITE feature is ignored, so any scheduler can
be used. Using one would not be a good idea in any case, but if bfq is needed
it can be used: we do not have the sequential write constraint thanks to the
use of zone append writes only for sequential zones. 

> > 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."
> > 
> > 
> 
> 
> Works for me as well. My only concern is, people will default to bfq and then 
> complain for XY because of the change to bfq. Note, I'm not against bfq at all,
> I'm just arguing it's not a one size fits all decision.
> 
> I'd also like to see if scrub on a nvme really is beneficial with bfq instead of
> none. I have my doubt's but that's a gut feeling and I have no numbers to back up
> this statement.
> 
> Nice weekend,
> 	Johannes
> 

-- 
Damien Le Moal
Western Digital

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

[Sidong Yang]

On Sat, Dec 12, 2020 at 11:05:04AM +0000, Damien Le Moal wrote:
> On Sat, 2020-12-12 at 10:34 +0000, Johannes Thumshirn wrote:
> > On 11/12/2020 18:05, Graham Cobb wrote:
> > > 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.
> 
> Elevator not compatible with zoned block devices will not even be shown in
> sysfs. E.g. bfq is not even listed for SMR drives. So the user/FS cannot select
> a wrong scheduler beside "none". For SMR, that currently means deadline only.
> 
> For ZNS, the ELEVATOR_F_ZBD_SEQ_WRITE feature is ignored, so any scheduler can
> be used. Using one would not be a good idea in any case, but if bfq is needed
> it can be used: we do not have the sequential write constraint thanks to the
> use of zone append writes only for sequential zones. 
> 
> > > 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."
> > > 
> > > 
> > 
> > 
> > Works for me as well. My only concern is, people will default to bfq and then 
> > complain for XY because of the change to bfq. Note, I'm not against bfq at all,
> > I'm just arguing it's not a one size fits all decision.
> > 
> > I'd also like to see if scrub on a nvme really is beneficial with bfq instead of
> > none. I have my doubt's but that's a gut feeling and I have no numbers to back up
> > this statement.
> > 
> > Nice weekend,
> > 	Johannes

Hello All, 
I tried to understand but I don't seem to understand all. All I know is
that some people said that the patch worried about forcing to use bfq
and so the patch should be dropped. I agree that user application should
be independent to scheduler. and someone commented that there is option
when user provide arguments for ionice.
IMHO, it's good to warn when user provide option for ionice explicit in
verbose mode. I don't think it will affect the users who used it the way
it was before.
I'm just a newbie and thanks to all who gave me their good comments.

Thanks,
Sidong
> > 
> 
> -- 
> Damien Le Moal
> Western Digital

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

[Johannes Thumshirn]

On 12/12/2020 17:45, Sidong Yang wrote:
> On Sat, Dec 12, 2020 at 11:05:04AM +0000, Damien Le Moal wrote:
>> On Sat, 2020-12-12 at 10:34 +0000, Johannes Thumshirn wrote:
>>> On 11/12/2020 18:05, Graham Cobb wrote:
>>>> 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.
>>
>> Elevator not compatible with zoned block devices will not even be shown in
>> sysfs. E.g. bfq is not even listed for SMR drives. So the user/FS cannot select
>> a wrong scheduler beside "none". For SMR, that currently means deadline only.
>>
>> For ZNS, the ELEVATOR_F_ZBD_SEQ_WRITE feature is ignored, so any scheduler can
>> be used. Using one would not be a good idea in any case, but if bfq is needed
>> it can be used: we do not have the sequential write constraint thanks to the
>> use of zone append writes only for sequential zones. 
>>
>>>> 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."
>>>>
>>>>
>>>
>>>
>>> Works for me as well. My only concern is, people will default to bfq and then 
>>> complain for XY because of the change to bfq. Note, I'm not against bfq at all,
>>> I'm just arguing it's not a one size fits all decision.
>>>
>>> I'd also like to see if scrub on a nvme really is beneficial with bfq instead of
>>> none. I have my doubt's but that's a gut feeling and I have no numbers to back up
>>> this statement.
>>>
>>> Nice weekend,
>>> 	Johannes
> 
> Hello All, 
> I tried to understand but I don't seem to understand all. All I know is
> that some people said that the patch worried about forcing to use bfq
> and so the patch should be dropped. I agree that user application should
> be independent to scheduler. and someone commented that there is option
> when user provide arguments for ionice.
> IMHO, it's good to warn when user provide option for ionice explicit in
> verbose mode. I don't think it will affect the users who used it the way
> it was before.
> I'm just a newbie and thanks to all who gave me their good comments.

Hi Sidong,

This wasn't meant to discourage you, sorry if it came across this way. I 
understand your intention, but I fear it will have negative implications.

Thanks,
	Johannes 

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

[Nikolay Borisov]

On 5.12.20 г. 20:49 ч., 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.

NAK, use applications should be oblivious to what scheduler the admin
has set up. It's the responsibility of the admin to configure their
system properly, at most there could be a note that scrub is an
io-intensive process and leave the rest to the admin.

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

[David Sterba]

On Mon, Dec 07, 2020 at 10:00:50AM +0200, Nikolay Borisov wrote:
> On 5.12.20 г. 20:49 ч., 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.
> 
> NAK, use applications should be oblivious to what scheduler the admin
> has set up. It's the responsibility of the admin to configure their
> system properly, at most there could be a note that scrub is an
> io-intensive process and leave the rest to the admin.

If the admin is user of the machine, eg. on a desktop and starts a
weekly scrub job that renders the machine unusable, then the warning
makes sense. And actually this is the situation that happened and
prompted updates to documentation and also the warning to make it more
visible.

That mq-deadline scheduler lacks ionice support is not nice, we're
trying to minimize the impact at least.