Re: [PATCH v2 2/3] md/raid1,raid10: fix error-path detection with md_cloned_bio()

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Kuai

On Thu, May 21, 2026 at 15:29 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/5/21 15:08, Xiao Ni 写道:
>> On Thu, May 21, 2026 at 9:25 AM Yu Kuai <yukuai@fnnas.com> wrote:
>>> Hi,
>>>
>>> 在 2026/5/1 19:46, Abd-Alrhman Masalkhi 写道:
>>>> Detect the error path using md_cloned_bio() instead of relying
>>>> on r1_bio in raid1 or r10_bio->read_slot in raid10, which may be
>>>> NULL or -1 after splitting and resubmitting a failed bio.
>>>>
>>>> As a result, the error path may not be recognized and memory
>>>> allocations can incorrectly use GFP_NOIO instead of
>>>> (GFP_NOIO | __GFP_HIGH), which can lead to a deadlock under
>>>> memory pressure.
>>> I don't think this patch will fix this problem. Because split bio
>>> is a new bio that will be resubmit by md_submit_bio(), such bio is
>>> not a md_cloned_bio yet until md_account_bio().
>> Hi Kuai
>>
>>          bio = bio_submit_split_bioset(bio, max_sectors,
>>                            &conf->bio_split);
>> bio_submit_split_bioset returns the split bio and the remainder bio
>> will be submitted again which is already a md_cloned_bio.
>
> Okay, I understand now this is a special case that current->bio_list is
> NULL. However, the problem still exist in the case original IO split again
> while resubmitting, and in this case current->bio_list will be set so new
> split io will be resubmit by md_submit_bio(). Which means patch 1 can't
> fix this case as well.

The original bio resubmitted via bio_list is not executing in the raid1d
thread context, so blocking on is_suspended() is correct and will not
cause a deadlock. The suspension deadlock risk only exists for
md_cloned_bio executing in the raid1d thread context in the error path.
And in this case it is always true that if we are executing in the
raid1d thread context the bio will already be a md_cloned_bio on entry
to md_handle_request(), even if current->bio_list was set, because (and as
@Xiao has mentioned) every time we split a bio in the error path we are
resubmitting the md_cloned_bio remainder, never the original bio.

>
> Perhaps it's better to check task_struct instead of bio_set.
>
>>
>> Regards
>> Xiao
>>>> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
>>>> Fixes: 545250f24809 ("md/raid10: simplify handle_read_error()")
>>>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>>>> ---
>>>> This patch depends on patch 1.
>>>>
>>>> Changes in v2:
>>>>    - New patch.
>>>> ---
>>>>    drivers/md/raid1.c  | 13 ++++++++++---
>>>>    drivers/md/raid10.c | 20 ++++++++++++++------
>>>>    2 files changed, 24 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>>> index cc9914bd15c1..c52ecd38c163 100644
>>>> --- a/drivers/md/raid1.c
>>>> +++ b/drivers/md/raid1.c
>>>> @@ -1321,11 +1321,18 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>>>>        bool r1bio_existed = !!r1_bio;
>>>>
>>>>        /*
>>>> -      * If r1_bio is set, we are blocking the raid1d thread
>>>> -      * so there is a tiny risk of deadlock.  So ask for
>>>> +      * An md cloned bio indicates we are in the error path.
>>>> +      * This is more reliable than checking r1_bio, which might
>>>> +      * be NULL even in the error path if a failed bio was split.
>>>> +      */
>>>> +     bool err_path = md_cloned_bio(mddev, bio);
>>>> +
>>>> +     /*
>>>> +      * If we are in the error path, we are blocking the raid1d
>>>> +      * thread so there is a tiny risk of deadlock.  So ask for
>>>>         * emergency memory if needed.
>>>>         */
>>>> -     gfp_t gfp = r1_bio ? (GFP_NOIO | __GFP_HIGH) : GFP_NOIO;
>>>> +     gfp_t gfp = err_path ? (GFP_NOIO | __GFP_HIGH) : GFP_NOIO;
>>>>
>>>>        /*
>>>>         * Still need barrier for READ in case that whole
>>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>>> index 3a591e60a144..8c6fc398260e 100644
>>>> --- a/drivers/md/raid10.c
>>>> +++ b/drivers/md/raid10.c
>>>> @@ -1155,7 +1155,20 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>>>        char b[BDEVNAME_SIZE];
>>>>        int slot = r10_bio->read_slot;
>>>>        struct md_rdev *err_rdev = NULL;
>>>> -     gfp_t gfp = GFP_NOIO;
>>>> +
>>>> +     /*
>>>> +      * An md cloned bio indicates we are in the error path.
>>>> +      * This is more reliable than checking slot, which might
>>>> +      * be -1 even in the error path if a failed bio was split.
>>>> +      */
>>>> +     bool err_path = md_cloned_bio(mddev, bio);
>>>> +
>>>> +     /*
>>>> +      * If we are in the error path, we are blocking the raid10d
>>>> +      * thread so there is a tiny risk of deadlock.  So ask for
>>>> +      * emergency memory if needed.
>>>> +      */
>>>> +     gfp_t gfp = err_path ? (GFP_NOIO | __GFP_HIGH) : GFP_NOIO;
>>>>
>>>>        if (slot >= 0 && r10_bio->devs[slot].rdev) {
>>>>                /*
>>>> @@ -1166,11 +1179,6 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>>>                 * we lose the device name in error messages.
>>>>                 */
>>>>                int disk;
>>>> -             /*
>>>> -              * As we are blocking raid10, it is a little safer to
>>>> -              * use __GFP_HIGH.
>>>> -              */
>>>> -             gfp = GFP_NOIO | __GFP_HIGH;
>>>>
>>>>                disk = r10_bio->devs[slot].devnum;
>>>>                err_rdev = conf->mirrors[disk].rdev;
>>> --
>>> Thansk,
>>> Kuai
>>>
> -- 
> Thansk,
> Kuai

-- 
Best Regards,
Abd-Alrhman