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

["yu kuai" <yukuai@fygo.io>]

Hi,

在 2026/11/3 18:03, Jinpu Wang 写道:
> On Mon, Nov 2, 2026 at 5:20 PM <sashiko-bot@kernel.org> wrote:
>> 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
>> --
> Hi Kuai,
>
> Sashiko flagged two issues with this patch, and I think both are valid:
>
> 1) mddev_freeze_queue() checks reshape_interrupted() only once. If a
>     raid456 reshape is interrupted or frozen while we wait for
>     q_usage_counter to drain, a bio already in md_handle_request() loops
>     back to check_suspended without dropping its q_usage_counter
>     reference. The wait never ends, because active_io is not killed yet
>     and prepare_suspend() has not been called.
>
> 2) Freezing the queue before killing active_io deadlocks with
>     raid1d/raid10d. If a read error makes raid1_read_request() split an
>     md_io_clone bio, the remainder is resubmitted from the md thread
>     through bio_queue_enter(). That blocks on the frozen queue, so the
>     md_cloned_bio() bypass in md_handle_request() is never reached and
>     active_io never drains.
>
> For (1) I plan to re-evaluate the skip conditions periodically in the
> wait. For (2) I don't see a small fix. The options are:
>    a) have the raid1/raid10 read-error path submit the split remainder
>       directly to md_handle_request() and skip bio_queue_enter(), or
>    b) drop the freeze from mddev_suspend() and fix the original
>       queue_attr_store()/limits_lock vs reconfig_mutex deadlock on the
>       other side.
>
> Which do you prefer, or do you see another approach?

I feel this is too complex and it's not worth it.

Since the reason to freeze queue in queue_attr_store() is to avoid updating
limits with I/O inflight, and call blk_mq_freeze_queue() for bio-based disks
is useless, which means there is still real problem for bio-based disks that
queue_attr_store() can update limits with bios inflight.

Christoph, how about adding a new suspend bio disk ops, and replace blk_mq_freeze_queue()
with the new ops for bio-based disks in queue_limits_commit_update_frozen()?

>
> Thanks,
>
>
>> 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

-- 
Thanks,
Kuai