cfq: priority boost on meta/prio marked IO

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.

I expect a higher prio process could be blocked on a lower prio process reading the same metadata, too. I had a hard time tracking down where REQ_META WRITE I/O was issued outside of the journal or writeback paths (and I hope you're not ionice-ing those!). Eventually, with the help of sandeen, I found some oddball cases that I doubt you're running into. Can you enlighten me as to where this (REQ_META write I/O) is happening? I don't disagree that it's a problem, I just would like to understand your problem case better.

Anyway, it seems to me you could just set REQ_PRIO in the code paths you care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as the same thing, which essentially undoes commit 65299a3b788bd ("block:
separate priority boosting from REQ_META") from Christoph.

Quoted 8 lines · Jeff Moyer

I expect a higher prio process could be blocked on a lower prio process reading the same metadata, too. I had a hard time tracking down where REQ_META WRITE I/O was issued outside of the journal or writeback paths (and I hope you're not ionice-ing those!). Eventually, with the help of sandeen, I found some oddball cases that I doubt you're running into. Can you enlighten me as to where this (REQ_META write I/O) is happening? I don't disagree that it's a problem, I just would like to understand your problem case better.

XFS does lots of REQ_META writes from _xfs_buf_ioapply(). But none of those should be in the critical path as the all metadata is logged first and then written back later.

Quoted 4 lines

Anyway, it seems to me you could just set REQ_PRIO in the code paths you care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as the same thing, which essentially undoes commit 65299a3b788bd ("block:
separate priority boosting from REQ_META") from Christoph.

And I'm still waiting for someone to explain when exactly REQ_PRIO should be used..

Hi, Jens,

Quoted 5 lines · Jens Axboe

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.

I expect a higher prio process could be blocked on a lower prio process reading the same metadata, too. I had a hard time tracking down where REQ_META WRITE I/O was issued outside of the journal or writeback paths (and I hope you're not ionice-ing those!). Eventually, with the help of sandeen, I found some oddball cases that I doubt you're running into. Can you enlighten me as to where this (REQ_META write I/O) is happening? I don't disagree that it's a problem, I just would like to understand your problem case better.

Anyway, it seems to me you could just set REQ_PRIO in the code paths you care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as the same thing, which essentially undoes commit 65299a3b788bd ("block:
separate priority boosting from REQ_META") from Christoph.

Quoted 19 lines · Jeff Moyer

Hi, Jens,

Quoted 5 lines · Jens Axboe

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.

I expect a higher prio process could be blocked on a lower prio process reading the same metadata, too. I had a hard time tracking down where REQ_META WRITE I/O was issued outside of the journal or writeback paths (and I hope you're not ionice-ing those!). Eventually, with the help of sandeen, I found some oddball cases that I doubt you're running into. Can you enlighten me as to where this (REQ_META write I/O) is happening? I don't disagree that it's a problem, I just would like to understand your problem case better.

Quoted 4 lines

Anyway, it seems to me you could just set REQ_PRIO in the code paths you care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as the same thing, which essentially undoes commit 65299a3b788bd ("block:
separate priority boosting from REQ_META") from Christoph.

I personally don't care too much about boosting for just PRIO, or for both PRIO and META. For the important paths, we basically have both set anyway.

These are reads.

It's a classic case of starting some operation that ends up holding a file system resource, then making very slow progress (due to task being scheduled as idle IO), and hence blocking out potentially much important tasks.

The IO is marked META|PRIO, so if folks don't [care] for boosting on meta, we can just kill that. "Normal" meta data could be scheduled as idle, but anything scheduled under a fs lock or similar should be marked PRIO and get boosted.

Quoted 25 lines · Jens Axboe

Quoted 23 lines · Jeff Moyer

Hi, Jens,

Quoted 5 lines · Jens Axboe

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.

I expect a higher prio process could be blocked on a lower prio process reading the same metadata, too. I had a hard time tracking down where REQ_META WRITE I/O was issued outside of the journal or writeback paths (and I hope you're not ionice-ing those!). Eventually, with the help of sandeen, I found some oddball cases that I doubt you're running into. Can you enlighten me as to where this (REQ_META write I/O) is happening? I don't disagree that it's a problem, I just would like to understand your problem case better.

Anyway, it seems to me you could just set REQ_PRIO in the code paths you care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as the same thing, which essentially undoes commit 65299a3b788bd ("block:
separate priority boosting from REQ_META") from Christoph.

Quoted 3 lines

I personally don't care too much about boosting for just PRIO, or for both PRIO and META. For the important paths, we basically have both set anyway.

Yeah, I had considered that. I like that REQ_META is separated out for logging purposes. I'm not too concerned about whether we imply boosted priority from that or not.

Quoted 1 line

These are reads.

I was curious about writes. ;-) Anyway, it's good to validate that the read case is also relevant.

Quoted 9 lines

It's a classic case of starting some operation that ends up holding a file system resource, then making very slow progress (due to task being scheduled as idle IO), and hence blocking out potentially much important tasks.

The IO is marked META|PRIO, so if folks don't [care] for boosting on meta, we can just kill that. "Normal" meta data could be scheduled as idle, but anything scheduled under a fs lock or similar should be marked PRIO and get boosted.

Interesting. I would have thought that the cfqd->active_queue would have been preempted by a request marked with REQ_PRIO. But you're suggesting that did not happen?

Specifically, cfq_insert_request would call cfq_rq_enqueued, and that would call cfq_preempt_queue if cfq_should_preempt (and I think it should, since the new request has REQ_PRIO set).

Quoted 2 lines

I was curious about writes. ;-) Anyway, it's good to validate that the read case is also relevant.

You mean O_DIRECT writes? Most of the buffered writes will come out of the associated threads, so I don't think it's a big of an issue there. We've only seen it for reads.

Quoted 3 lines

Interesting. I would have thought that the cfqd->active_queue would have been preempted by a request marked with REQ_PRIO. But you're suggesting that did not happen?

We seem to handily mostly ignore prio_pending for the idle class. If

the new queue is idle, then we don't look at prio pending. I'd rather make this more explicit, the patch is pretty similar to what we had in the past. It's somewhat of a regression caused by commit 4aede84b33d, except I like using the request flags for this a lot more than the old current->fs_excl. REQ_PRIO should always be set for cases where we hold fs (or even directory) specific resources.

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 <[email protected]>

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

Quoted 17 lines · Jens Axboe

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 <[email protected]>

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

block/blk-core.c get_rq: · line 1788

Quoted 8 lines

block/blk-core.c +5 -0

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

Other than that, I think this looks fine.

Quoted 31 lines · Jeff Moyer

Quoted 22 lines · Jens Axboe

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 <[email protected]>

block/blk-core.c +1 -0

index 32a283eb7274..3cfd67d006fb 100644
@@ -1781,6 +1781,11 @@ get_rq:
          rw_flags |= REQ_SYNC;
 
      /*
+     * Add in META/PRIO flags, if set, before we get to the IO

scheduler

block/blk-core.c

Quoted 4 lines

block/blk-core.c +4 -0

+     */
+    rw_flags |= (bio->bi_rw & (REQ_META | REQ_PRIO));
+
+    /*

This needs a docbook update. It now reads:

Quoted 6 lines

* @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?

Quoted 1 line

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

Ah now I see, you're looking at current -git. The patch is against for-4.8/core.

Updated version below, dropping REQ_META and changing the naming s/meta/prio.

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

Quoted 4 lines · Jens Axboe

Quoted 1 line

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

Ah now I see, you're looking at current -git. The patch is against for-4.8/core.

Ah, right, Mike's patches went in.

Quoted 10 lines

Updated version below, dropping REQ_META and changing the naming s/meta/prio.

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644
@@ -1781,6 +1781,11 @@ get_rq:
  		rw_flags |= REQ_SYNC;
 

block/blk-core.c get_rq: · line 1788

Quoted 6 lines

block/blk-core.c +5 -0

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

Do we still need to pass in META here?

Quoted 19 lines · Jeff Moyer

Quoted 4 lines · Jens Axboe

Quoted 1 line

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

Ah now I see, you're looking at current -git. The patch is against for-4.8/core.

Ah, right, Mike's patches went in.

Quoted 10 lines

Updated version below, dropping REQ_META and changing the naming s/meta/prio.

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644
@@ -1781,6 +1781,11 @@ get_rq:
  		rw_flags |= REQ_SYNC;
 

Quoted 8 lines

block/blk-core.c get_rq: · line 1788

Quoted 6 lines

block/blk-core.c +5 -0

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

Do we still need to pass in META here?

We don't have to, but it doesn't really hurt. Frankly, we should pass in the whole damn thing.

Quoted 29 lines · Jens Axboe

Quoted 27 lines · Jeff Moyer

Quoted 4 lines · Jens Axboe

Quoted 1 line

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

Ah now I see, you're looking at current -git. The patch is against for-4.8/core.

Ah, right, Mike's patches went in.

Quoted 8 lines

Updated version below, dropping REQ_META and changing the naming s/meta/prio.

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

block/blk-core.c get_rq: · line 1788

Quoted 8 lines

block/blk-core.c +5 -0

@@ -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));
+
+	/*

Do we still need to pass in META here?

Quoted 2 lines

We don't have to, but it doesn't really hurt. Frankly, we should pass in the whole damn thing.

Heh, okay.

Fine by me, Jens. :)

Reviewed-by Jeff Moyer <[email protected]>

Quoted 30 lines · Jeff Moyer

Quoted 17 lines · Jens Axboe

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 <[email protected]>

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

block/blk-core.c get_rq: · line 1788

Quoted 8 lines

block/blk-core.c +5 -0

@@ -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:

Quoted 6 lines

* @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?

Quoted 35 lines · Jens Axboe

Quoted 34 lines · Jeff Moyer

Quoted 17 lines · Jens Axboe

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 <[email protected]>

block/blk-core.c +0 -0

index 32a283eb7274..3cfd67d006fb 100644

block/blk-core.c get_rq: · line 1788

Quoted 8 lines

block/blk-core.c +5 -0

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

Quoted 4 lines

Quoted 2 lines

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! ;-)