[PATCH] md/raid5: fix write stalls when a write is queued during a drain

[PATCH] md/raid5: fix write stalls when a write is queued during a drain

[Miroslav Cimerman]

With continuous O_DIRECT writes to a RAID-5/6 array that uses
read-modify-writes, a few 4k writes can take seconds to complete, up
to the length of the run.

The pre-read permission (STRIPE_PREREAD_ACTIVE) belongs to a stripe.
A REQ_SYNC write queued while an earlier write on the same stripe is
being drained finds the bit set and gets no count of its own in
preread_active_stripes. handle_stripe() clears the bit when the drain
result is handled, so the queued write gets STRIPE_DELAYED, and the
stripe goes to delayed_list together with the completion of the
drained write. raid5_activate_delayed() only moves it on once
preread_active_stripes is below IO_THRESHOLD, which with other
REQ_SYNC writes in flight may happen until the load stops.

Check for writes queued on the stripe, under stripe_lock, after
clearing the bit, and keep the bit and its count if there are any.
A write queued later tests the bit after the clear and either takes
the permission itself or finds it kept. Do this for any queued write:
a queued buffered write would otherwise hold back the completion of
the drained write the same way.

md-7.4, RAM disks, 4k random writes, io_uring, QD64, 20 s, slowest
write without -> with this patch, md defaults (1 job) and
group_thread_cnt=8 (16 jobs):

  RAID-6 4+2 (rmw_level=2) 18.0 s -> 2.0 ms  19.3 s -> 5.8 ms
  RAID-6 8+2               13.4 s -> 2.2 ms  13.0 s -> 7.0 ms
  RAID-5 5+1               16.9 s -> 1.8 ms  18.3 s -> 4.9 ms
  RAID-5 9+1               17.4 s -> 2.2 ms  12.0 s -> 5.8 ms

IOPS are unchanged with md defaults and within a few percent with 8
workers. Buffered writes are not affected.

Fixes: e9c7469bb4f5 ("md: implment REQ_FLUSH/FUA support")
Cc: stable@vger.kernel.org
Signed-off-by: Miroslav Cimerman <mc@raidlab.org>
---
 drivers/md/raid5.c | 41 +++++++++++++++++++++++++++++++++++++++--
 drivers/md/raid5.h |  4 +++-
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index efb4b0d37cb0..b103cc8d142a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5011,6 +5011,27 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
 		set_bit(STRIPE_HANDLE, &head_sh->state);
 }
 
+/*
+ * Check if a stripe head has any writes queued. Takes sh->stripe_lock under
+ * which writes are queued.
+ */
+static bool stripe_has_queued_writes(struct stripe_head *sh)
+{
+	bool queued = false;
+	int i;
+
+	spin_lock(&sh->stripe_lock);
+	for (i = 0; i < sh->disks; i++) {
+		if (sh->dev[i].towrite) {
+			queued = true;
+			break;
+		}
+	}
+	spin_unlock(&sh->stripe_lock);
+
+	return queued;
+}
+
 /*
  * handle_stripe - do things to a stripe.
  *
@@ -5165,8 +5186,24 @@ static void handle_stripe(struct stripe_head *sh)
 					set_bit(STRIPE_INSYNC, &sh->state);
 			}
 		}
-		if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
-			s.dec_preread_active = 1;
+		if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) {
+			/*
+			 * A write queued while the previous one was drained
+			 * has no pre-read permission of its own: a REQ_SYNC
+			 * write or discard found the bit set, other writes
+			 * never take it. Keep the permission for it, or it
+			 * gets STRIPE_DELAYED and the stripe, together with
+			 * the completion of the drained write, may wait on
+			 * delayed_list until preread_active_stripes drops
+			 * below IO_THRESHOLD. A write queued after
+			 * stripe_has_queued_writes() tests the bit after our
+			 * clear, so it takes the permission itself or finds
+			 * it kept.
+			 */
+			if (!stripe_has_queued_writes(sh) ||
+			    test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+				s.dec_preread_active = 1;
+		}
 	}
 
 	/*
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 1a61b474fc22..c3feb7779dbe 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -448,7 +448,9 @@ enum {
  * When preread is initiated on a stripe, we set PREREAD_ACTIVE and add
  * it to the count of prereading stripes.
  * When write is initiated, or the stripe refcnt == 0 (just in case) we
- * clear the PREREAD_ACTIVE flag and decrement the count
+ * clear the PREREAD_ACTIVE flag and decrement the count. When write is
+ * initiated and another write is already queued on the stripe, the flag and
+ * the count are kept for that write (see handle_stripe()).
  * Whenever the 'handle' queue is empty and the device is not plugged, we
  * move any strips from delayed to handle and clear the DELAYED flag and set
  * PREREAD_ACTIVE.

base-commit: 47e1b18f38d74015cb9fd30b5ff2a1eb88e77d31
-- 
2.53.0