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

[sashiko-bot@kernel.org]

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