[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] md: deadlock in raid1d() and raid10d() due to queue freeze in mddev_suspend()
- [High] md: deadlock in mddev_freeze_queue() when reshape is interrupted during wait
--

commit bc4b2b2abc71e036c550ab67e56cbc50b4eb867f
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

md: freeze the queue in mddev_suspend()

This commit freezes the request queue in mddev_suspend() and unfreezes it in
__mddev_resume() so that bios submitted while the array is suspended block in
bio_queue_enter() without holding q_usage_counter, avoiding a deadlock with
concurrent queue_attr_store() limits updates.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..29737a624844a 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));
> +}

[Severity: High]
Could this wait in mddev_freeze_queue() deadlock if a RAID456 reshape is
interrupted or frozen after the initial reshape_interrupted(mddev) check
passes, while an unsplit bio is already inside md_handle_request()?

Consider an active RAID456 reshape where reshape_interrupted(mddev) is
initially false and an unsplit bio enters:

__submit_bio()
  bio_queue_enter() /* acquires q->q_usage_counter */
  md_submit_bio()
    md_handle_request()
      raid5_make_request()

If mddev_suspend() runs concurrently (for example via suspend_lo_store() or
suspend_hi_store(), which do not hold reconfig_mutex), mddev_freeze_queue()
sees reshape_interrupted(mddev) == false and blocks waiting for
q->q_usage_counter to reach zero before mddev_suspend() reaches
percpu_ref_kill(&mddev->active_io) or mddev->pers->prepare_suspend(mddev).

If the reshape is then interrupted or frozen while mddev_freeze_queue() is
waiting (for example via action_store() setting MD_RECOVERY_FROZEN or
MD_RECOVERY_INTR, or md_error() setting MD_RECOVERY_INTR on a disk failure),
make_stripe_request() sees reshape_interrupted(mddev) == true and returns
STRIPE_WAIT_RESHAPE:

drivers/md/raid5.c:raid5_make_request() {
	...
	if (res == STRIPE_WAIT_RESHAPE) {
		...
		if (split) {
			submit_bio_noacct(front_bio);
			return true;
		}
		return false;
	}
}

For an unsplit bio, raid5_make_request() returns false to md_handle_request():

drivers/md/md.c:md_handle_request() {
	...
	if (!mddev->pers->make_request(mddev, bio)) {
		percpu_ref_put(&mddev->active_io);
		if (mddev_is_dm(mddev) && mddev->pers->prepare_suspend)
			return false;
		goto check_suspended;
	}
}

Previously, mddev_suspend() killed mddev->active_io first, so dropping
mddev->active_io allowed mddev_suspend() to finish while the bio parked in
check_suspended. With the queue freeze wait happening first, the bio loops
back to check_suspended inside md_handle_request() without exiting
md_submit_bio() or dropping q->q_usage_counter (and busy-loops because
mddev->active_io has not been killed yet), or sleeps in conf->wait_for_reshape
because prepare_suspend() has not been called, so q->q_usage_counter never
drains.

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

[Severity: High]
Does freezing mddev->gendisk->queue before mddev->active_io drains cause a
deadlock with raid1d() and raid10d() when handling read errors?

For bio-based MD devices, __submit_bio() drops q->q_usage_counter in
blk_queue_exit() as soon as md_submit_bio() returns, while the cloned
r1_bio->master_bio or r10_bio->master_bio (allocated from mddev->io_clone_set
by md_account_bio()) keeps holding a reference on mddev->active_io until the
underlying member-disk read completes.

If mddev_suspend() is called while a read bio is in flight on a member disk:
1. mddev_freeze_queue() sees q->q_usage_counter is already zero and returns
   with mddev->gendisk->queue frozen.
2. mddev_suspend() kills mddev->active_io and blocks in wait_event() waiting
   for percpu_ref_is_zero(&mddev->active_io).
3. The member-disk read fails, and raid1d() or raid10d() runs
   handle_read_error() -> raid1_read_request() / raid10_read_request() using
   the md_io_clone master_bio:

drivers/md/raid1.c:raid1_read_request() {
	...
	if (max_sectors < bio_sectors(bio)) {
		bio = bio_submit_split_bioset(bio, max_sectors,
					      &conf->bio_split);
	...
}

If the surviving mirror has a smaller max_sectors (for example, due to bad
blocks), bio_submit_split_bioset() splits the md_io_clone bio and calls
submit_bio_noacct_nocheck() on the remainder targeting the MD device.

Commit 7b15c24f8053 ("md: don't hang mddev_suspend() on resubmitted bios")
added the md_cloned_bio() check in md_handle_request() so that resubmitted
md_io_clone bios from raid1d() or raid10d() would bypass the suspend wait and
allow mddev->active_io to drain:

drivers/md/md.c:md_handle_request() {
check_suspended:
	if (unlikely(md_cloned_bio(mddev, bio))) {
		/*
		 * This bio is an MD cloned bio and already holds an
		 * active_io reference, so percpu_ref_get() is safe here.
		 */
		percpu_ref_get(&mddev->active_io);
	} else {
	...
}

However, because raid1d() and raid10d() run with current->bio_list == NULL,
submit_bio_noacct_nocheck() synchronously calls __submit_bio() ->
bio_queue_enter() on the frozen MD queue and blocks on q->mq_freeze_wq before
reaching md_submit_bio() and md_handle_request(). Won't raid1d() or raid10d()
wait forever for the queue to be unfrozen by __mddev_resume(), while
mddev_suspend() waits forever for the split md_io_clone bio to complete and
drop mddev->active_io?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102154847.198472-1-jinpu.wang@ionos.com?part=1