[PATCH v3 0/1] md: freeze the queue in mddev_suspend()

[PATCH v3 0/1] md: freeze the queue in mddev_suspend()

[Jack Wang]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

Writing to a queue limits attribute of an md array while a spare is
being re-added deadlocks the array.  I reported this earlier here:

  https://lore.kernel.org/linux-raid/CAMGffE=heGA3y8FjQ0Sm1jj-kd-=H9Y54WozKASSEZhc9UNKjA@mail.gmail.com/

Four tasks, one array:

  udev-worker    queue_attr_store() holds q->limits_lock, waits in
                 blk_mq_freeze_queue() for q_usage_counter to drain
  fio            holds a q_usage_counter reference, parked in
                 md_handle_request()'s is_suspended() loop
  mdadm          suspended the array, waits for reconfig_mutex
  md_start_sync  holds reconfig_mutex, waits for q->limits_lock

Since commit c99f66e4084a ("block: fix queue freeze vs limits lock
order in sysfs store methods") the sysfs store holds q->limits_lock
across the freeze.  A bio that is parked in md_handle_request() while
the array is suspended still holds a q_usage_counter reference, so that
freeze can never complete, and everything behind it stalls.

v2 reworked the lock ordering so that q->limits_lock is taken outside
reconfig_mutex and the suspend everywhere, which took eight patches.
Yu Kuai pointed out in his reply to v2 that the freeze is of little use
for bio based devices on its own, since it cannot guarantee that
in-flight bios have drained, and that the deadlock is really a
blk_mq_freeze_queue() running concurrently with mddev_suspend().  He
suggested freezing the queue in mddev_suspend() instead.  This version
does that, and replaces the whole of v2 with one patch.

With the queue frozen by the suspend, bios submitted while the array is
suspended block in bio_queue_enter() without holding a reference, so a
concurrent limits update can finish and release q->limits_lock.  No
lock ordering has to change.

The freeze cannot always wait for the queue to drain.  A writer waiting
on the suspend_lo/suspend_hi window and raid456 I/O waiting on an
interrupted reshape sit in submit_bio() holding a reference, and only
make progress once the suspend has completed.  In those two cases the
freeze is started but not waited for.  The original deadlock can still
happen there, the same as before the patch.  dm-raid has no gendisk and
is not frozen.

v2: https://lore.kernel.org/linux-raid/e2c36f31-d198-40a7-a18d-156e1fd6fbb2@fygo.io/
v1: https://lore.kernel.org/linux-raid/20260909063029.GC29874@lst.de/T/#t

Changes since v2:
 - Drop the series.  Freeze the queue in mddev_suspend() as suggested
   by Yu Kuai, instead of changing the q->limits_lock vs
   reconfig_mutex / disk->open_mutex ordering.
 - Skip the drain wait for the suspend_lo/suspend_hi window and for an
   interrupted raid456 reshape.
 - Do not freeze dm-raid, which has no gendisk.
 - Rebased on v7.4-rc1.

Testing, on a 6.12 based kernel with the same change, lockdep enabled
(PROVE_LOCKING, DEBUG_LOCK_ALLOC):

 - the reproducer below, 20 fail/remove/add cycles with fio in flight
   and a loop writing queue/max_sectors_kb, which wedges the array
   without the patch
 - a raid5 3 -> 4 reshape with fio writing, which moves the
   suspend_lo/suspend_hi window while writers are active, and
   mdadm --stop afterwards

No lockdep reports and no hung tasks.  I have not yet run the mdadm
test tests/25raid456-reshape-deadlock, dm-raid, or an upstream kernel
with this patch.

A blktests case covering this deadlock was sent separately:

  https://lore.kernel.org/all/20260910100916.1638378-1-jinpu.wang@ionos.com/

The reproducer:

  mdadm -C /dev/md111 --force -e 1.2 --assume-clean -l 1 \
        --bitmap=internal -n 2 /dev/ram0 /dev/ram1

  fio --direct=1 --rw=randrw --ioengine=libaio --iodepth=32 --numjobs=4 \
      --time_based=1 --runtime=180 --filename=/dev/md111 --name=repro &

  while :; do
      echo 128 > /sys/block/md111/queue/max_sectors_kb 2>/dev/null
  done &

  for i in $(seq 20); do
      mdadm /dev/md111 --fail /dev/ram0
      mdadm /dev/md111 --remove /dev/ram0
      mdadm /dev/md111 --add /dev/ram0
      mdadm --wait /dev/md111
  done

Jack Wang (1):
  md: freeze the queue in mddev_suspend()

 drivers/md/md.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

[PATCH v3 1/1] md: freeze the queue in mddev_suspend()

[Jack Wang]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

queue_attr_store() holds q->limits_lock and waits in
blk_mq_freeze_queue() for q_usage_counter to drain. A bio parked in the
is_suspended() loop of md_handle_request() still holds a reference, so
the freeze never completes. If the suspender waits for reconfig_mutex
held by md_start_sync(), which waits for q->limits_lock, the tasks
deadlock.

Freeze the queue in mddev_suspend() and unfreeze it on resume, so bios
submitted while suspended block in bio_queue_enter() without holding a
reference.

Skip the drain wait when parked bios can only progress after the suspend
completes: writers waiting on the suspend_lo/suspend_hi window and
raid456 IO waiting on an interrupted reshape. dm-raid has no gendisk and
is not frozen.

Fixes: c99f66e4084a ("block: fix queue freeze vs limits lock order in sysfs store methods")
Suggested-by: Yu Kuai <yukuai@fygo.io>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/md/md.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 03e35386a802..29737a624844 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -472,6 +472,42 @@ static void md_submit_bio(struct bio *bio)
 	md_handle_request(mddev, bio);
 }
 
+/*
+ * Freeze the queue so a concurrent blk_mq_freeze_queue() (e.g. a limits
+ * update) can't wait on a bio parked in md_handle_request() while suspended.
+ * Skip the drain wait when parked bios hold a reference and only progress
+ * after suspend: the suspend_lo/hi window, or an interrupted raid456 reshape.
+ * Resume may run in another task, hence the non_owner variants.
+ */
+static int mddev_freeze_queue(struct mddev *mddev, bool interruptible)
+{
+	struct request_queue *q;
+
+	if (mddev_is_dm(mddev))
+		return 0;
+
+	q = mddev->gendisk->queue;
+	blk_freeze_queue_start_non_owner(q);
+
+	if (READ_ONCE(mddev->suspend_lo) < READ_ONCE(mddev->suspend_hi) ||
+	    (mddev->pers && mddev->pers->prepare_suspend &&
+	     reshape_interrupted(mddev)))
+		return 0;
+
+	if (!interruptible) {
+		blk_mq_freeze_queue_wait(q);
+		return 0;
+	}
+	return wait_event_interruptible(q->mq_freeze_wq,
+			percpu_ref_is_zero(&q->q_usage_counter));
+}
+
+static void mddev_unfreeze_queue(struct mddev *mddev)
+{
+	if (!mddev_is_dm(mddev))
+		blk_mq_unfreeze_queue_non_owner(mddev->gendisk->queue);
+}
+
 /*
  * Make sure no new requests are submitted to the device, and any requests that
  * have been submitted are completely handled.
@@ -500,6 +536,13 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
 		return 0;
 	}
 
+	err = mddev_freeze_queue(mddev, interruptible);
+	if (err) {
+		mddev_unfreeze_queue(mddev);
+		mutex_unlock(&mddev->suspend_mutex);
+		return err;
+	}
+
 	percpu_ref_kill(&mddev->active_io);
 
 	/*
@@ -520,6 +563,7 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
 				percpu_ref_is_zero(&mddev->active_io));
 	if (err) {
 		percpu_ref_resurrect(&mddev->active_io);
+		mddev_unfreeze_queue(mddev);
 		mutex_unlock(&mddev->suspend_mutex);
 		return err;
 	}
@@ -555,6 +599,7 @@ static void __mddev_resume(struct mddev *mddev, bool recovery_needed)
 	md_wakeup_thread(mddev->thread);
 	md_wakeup_thread(mddev->sync_thread); /* possibly kick off a reshape */
 
+	mddev_unfreeze_queue(mddev);
 	mutex_unlock(&mddev->suspend_mutex);
 }
 
-- 
2.43.0