Re: [PATCH] cfq: priority boost on meta/prio marked IO

Jens Axboe [off-list ref] writes:

On 06/09/2016 03:28 PM, Jeff Moyer wrote:

quoted

Jens Axboe [off-list ref] writes:

quoted

At Facebook, we have a number of cases where people use ionice to set a
lower priority, then end up having tasks stuck for a long time because
eg meta data updates from an idle priority tasks is blocking out higher
priority processes. It's bad enough that it will trigger the softlockup
warning.

This patch adds code to CFQ that bumps the priority class and data for
an idle task, if is doing IO marked as PRIO or META. With this, we no
longer see the softlockups.

Signed-off-by: Jens Axboe <redacted>

diff --git a/block/blk-core.c b/block/blk-core.c
index 32a283eb7274..3cfd67d006fb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -1781,6 +1781,11 @@ get_rq:
  		rw_flags |= REQ_SYNC;

  	/*
+	 * Add in META/PRIO flags, if set, before we get to the IO scheduler
+	 */
+	rw_flags |= (bio->bi_rw & (REQ_META | REQ_PRIO));
+
+	/*

This needs a docbook update.  It now reads:

  * @rw_flags: RW and SYNC flags

so whatever flags we're adding should be specified, I guess.

Speaking of which, after much waffling, I think I've decided it would be
cleaner to limit the priority boost to REQ_PRIO requests only.

I went and checked, but I don't see it. Where is this?

Oops, sorry.  I meant that get_request and __get_request need updates to
their documentation.

On the second part (in case there was confusion on what I meant there),
what I meant was only do the prio boost for REQ_PRIO requests instead
of also doing it for REQ_META.  The way I arrived at that conclusion was
when I was going to ask you to update the documentation for REQ_META to
state that it implied REQ_PRIO, at which point, one has to wonder why we
need two flags.

There are cases where REQ_PRIO is used without REQ_META.
There are cases where REQ_META is used withoug REQ_PRIO.
And of course, there are cases where they're both sent down.

REQ_META itself is useful for tracing, and also makes the code
self-documenting.

REQ_PRIO pretty clearly means that we should boost priority for this
request.  And I think Christoph was making a case for REQ_META that
doesn't require a priority boost (if I read what he said correctly).

So, I think they serve different purposes.  Have I convinced you?  It'll
make your patch smaller!  ;-)

-Jeff