Re: [PATCH v2 1/3] md/raid1,raid10: fix deadlock in read error recovery path

On Fri, May 1, 2026 at 7:47 PM Abd-Alrhman Masalkhi
<[email protected]> wrote:
>
> raid1d and raid10d may resubmit a split md cloned bio while handling
> a read error. In this case, resubmitting the bio can lead to a deadlock
> if the array is suspended before md_handle_request() acquires an
> active_io reference via percpu_ref_tryget_live().
>
> Since the cloned bio already holds an active_io reference,
> trying to acquire another reference via percpu_ref_tryget_live()
> can lead to a deadlock while the array is suspended.
>
> Fix this by using percpu_ref_get() for md cloned bios.
>
> Fixes: bb2a9acefaf9 ("md/raid1: switch to use md_account_bio() for io accounting")
> Fixes: 820455238366 ("md/raid10: switch to use md_account_bio() for io accounting")
> Signed-off-by: Abd-Alrhman Masalkhi <[email protected]>
> ---
> Changes in v2:
>  - Use md_cloned_bio() consistently to detect cloned bios.
>  - Recognize that raid10 has the same issue and fix it in this series
>  - Allow splitting bios.
>  - Handle md cloned bios explicitly in md_handle_request()
>  - Link v1: https://lore.kernel.org/linux-raid/[email protected]/
>
> Please let me know if I should add a Suggested-by tag for Yu Kuai,
> as the solution approach was suggested during review.
>
> Link to Yu Kuai' email: https://lore.kernel.org/linux-raid/[email protected]/T/#m714020a38b60fc5f84b9a24f0c46acbe5d7342d6
>
> Thanks
> Abd-alrhman
> ---
>  drivers/md/md.c | 25 ++++++++++++++++---------
>  drivers/md/md.h |  5 +++++
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e926aef9ec43..96db1e7850e9 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -396,17 +396,24 @@ static bool is_suspended(struct mddev *mddev, struct bio *bio)
>  bool md_handle_request(struct mddev *mddev, struct bio *bio)
>  {
>  check_suspended:
> -       if (is_suspended(mddev, bio)) {
> -               /* Bail out if REQ_NOWAIT is set for the bio */
> -               if (bio->bi_opf & REQ_NOWAIT) {
> -                       bio_wouldblock_error(bio);
> -                       return true;
> +       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 {
> +               if (is_suspended(mddev, bio)) {
> +                       /* Bail out if REQ_NOWAIT is set for the bio */
> +                       if (bio->bi_opf & REQ_NOWAIT) {
> +                               bio_wouldblock_error(bio);
> +                               return true;
> +                       }
> +                       wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
>                 }
> -               wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
> +               if (!percpu_ref_tryget_live(&mddev->active_io))
> +                       goto check_suspended;
>         }
> -       if (!percpu_ref_tryget_live(&mddev->active_io))
> -               goto check_suspended;
> -
>         if (!mddev->pers->make_request(mddev, bio)) {
>                 percpu_ref_put(&mddev->active_io);
>                 if (mddev_is_dm(mddev) && mddev->pers->prepare_suspend)
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 3bfbee595156..e44074d30cf9 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -1038,6 +1038,11 @@ void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes);
>
>  extern const struct block_device_operations md_fops;
>
> +static inline bool md_cloned_bio(struct mddev *mddev, struct bio *bio)
> +{
> +       return bio->bi_pool == &mddev->io_clone_set;
> +}
> +
>  /*
>   * MD devices can be used undeneath by DM, in which case ->gendisk is NULL.
>   */
> --
> 2.43.0
>
>

This patch looks good to me.
Reviewed-by: Xiao Ni <[email protected]>