Message-ID: <29e337bc-9eee-4794-ae1e-184ef91b9d24@mgml.me>
Date: Sun, 31 Aug 2025 03:10:40 +0900
From: Kenta Akagi <k@...l.me>
To: Li Nan <linan666@...weicloud.com>, Song Liu <song@...nel.org>,
        Yu Kuai <yukuai3@...wei.com>, Mariusz Tkaczyk <mtkaczyk@...nel.org>,
        Guoqing Jiang <jgq516@...il.com>
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kenta Akagi <k@...l.me>
Subject: Re: [PATCH v3 1/3] md/raid1,raid10: Do not set MD_BROKEN on failfast
 io failure



On 2025/08/30 17:48, Li Nan wrote:
> 
> 
> 在 2025/8/29 20:21, Kenta Akagi 写道:
>>
>>
>> On 2025/08/29 11:54, Li Nan wrote:
>>>
>>> 在 2025/8/29 0:32, Kenta Akagi 写道:
>>>> This commit ensures that an MD_FAILFAST IO failure does not put
>>>> the array into a broken state.
>>>>
>>>> When failfast is enabled on rdev in RAID1 or RAID10,
>>>> the array may be flagged MD_BROKEN in the following cases.
>>>> - If MD_FAILFAST IOs to multiple rdevs fail simultaneously
>>>> - If an MD_FAILFAST metadata write to the 'last' rdev fails
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>>> index 408c26398321..8a61fd93b3ff 100644
>>>> --- a/drivers/md/raid1.c
>>>> +++ b/drivers/md/raid1.c
>>>> @@ -470,6 +470,7 @@ static void raid1_end_write_request(struct bio *bio)
>>>>                (bio->bi_opf & MD_FAILFAST) &&
>>>>                /* We never try FailFast to WriteMostly devices */
>>>>                !test_bit(WriteMostly, &rdev->flags)) {
>>>> +            set_bit(FailfastIOFailure, &rdev->flags);
>>>>                md_error(r1_bio->mddev, rdev);
>>>>            }
>>>>    @@ -1746,8 +1747,12 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
>>>>     *    - recovery is interrupted.
>>>>     *    - &mddev->degraded is bumped.
>>>>     *
>>>> - * @rdev is marked as &Faulty excluding case when array is failed and
>>>> - * &mddev->fail_last_dev is off.
>>>> + * If @rdev has &FailfastIOFailure and it is the 'last' rdev,
>>>> + * then @mddev and @rdev will not be marked as failed.
>>>> + *
>>>> + * @rdev is marked as &Faulty excluding any cases:
>>>> + *    - when @mddev is failed and &mddev->fail_last_dev is off
>>>> + *    - when @rdev is last device and &FailfastIOFailure flag is set
>>>>     */
>>>>    static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
>>>>    {
>>>> @@ -1758,6 +1763,13 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
>>>>          if (test_bit(In_sync, &rdev->flags) &&
>>>>            (conf->raid_disks - mddev->degraded) == 1) {
>>>> +        if (test_and_clear_bit(FailfastIOFailure, &rdev->flags)) {
>>>> +            spin_unlock_irqrestore(&conf->device_lock, flags);
>>>> +            pr_warn_ratelimited("md/raid1:%s: Failfast IO failure on %pg, "
>>>> +                "last device but ignoring it\n",
>>>> +                mdname(mddev), rdev->bdev);
>>>> +            return;
>>>> +        }
>>>>            set_bit(MD_BROKEN, &mddev->flags);
>>>>              if (!mddev->fail_last_dev) {
>>>> @@ -2148,6 +2160,7 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
>>>>        if (test_bit(FailFast, &rdev->flags)) {
>>>>            /* Don't try recovering from here - just fail it
>>>>             * ... unless it is the last working device of course */
>>>> +        set_bit(FailfastIOFailure, &rdev->flags);
>>>>            md_error(mddev, rdev);
>>>>            if (test_bit(Faulty, &rdev->flags))
>>>>                /* Don't try to read from here, but make sure
>>>> @@ -2652,6 +2665,7 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
>>>>            fix_read_error(conf, r1_bio);
>>>>            unfreeze_array(conf);
>>>>        } else if (mddev->ro == 0 && test_bit(FailFast, &rdev->flags)) {
>>>> +        set_bit(FailfastIOFailure, &rdev->flags);
>>>>            md_error(mddev, rdev);
>>>>        } else {
>>>>            r1_bio->bios[r1_bio->read_disk] = IO_BLOCKED;
>>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>>> index b60c30bfb6c7..530ad6503189 100644
>>>> --- a/drivers/md/raid10.c
>>>> +++ b/drivers/md/raid10.c
>>>> @@ -488,6 +488,7 @@ static void raid10_end_write_request(struct bio *bio)
>>>>                dec_rdev = 0;
>>>>                if (test_bit(FailFast, &rdev->flags) &&
>>>>                    (bio->bi_opf & MD_FAILFAST)) {
>>>> +                set_bit(FailfastIOFailure, &rdev->flags);
>>>>                    md_error(rdev->mddev, rdev);
>>>>                }
>>>>    
>>>
>>> Thank you for the patch. There may be an issue with 'test_and_clear'.
>>>
>>> If two write IO go to the same rdev, MD_BROKEN may be set as below:
>>
>>> IO1                    IO2
>>> set FailfastIOFailure
>>>                      set FailfastIOFailure
>>>   md_error
>>>    raid1_error
>>>     test_and_clear FailfastIOFailur
>>>                         md_error
>>>                        raid1_error
>>>                         //FailfastIOFailur is cleared
>>>                         set MD_BROKEN
>>>
>>> Maybe we should check whether FailfastIOFailure is already set before
>>> setting it. It also needs to be considered in metadata writes.
>> Thank you for reviewing.
>>
>> I agree, this seems to be as you described.
>> So, should it be implemented as follows?
>>
>> bool old=false;
>> do{
>>   spin_lock_irqsave(&conf->device_lock, flags);
>>   old = test_and_set_bit(FailfastIOFailure, &rdev->flags);
>>   spin_unlock_irqrestore(&conf->device_lock, flags);
>> }while(old);
>>
>> However, since I am concerned about potential deadlocks,
>> so I am considering two alternative approaches:
>>
>> * Add an atomic_t counter to md_rdev to track failfast IO failures.
>>
>> This may set MD_BROKEN at a slightly incorrect timing, but mixing
>> error handling of Failfast and non-Failfast IOs appears to be rare.
>> In any case, the final outcome would be the same, i.e. the array
>> ends up with MD_BROKEN. Therefore, I think this should not cause
>> issues. I think the same applies to test_and_set_bit.
>>
>> IO1                    IO2                    IO3
>> FailfastIOFailure      Normal IOFailure       FailfastIOFailure
>> atomic_inc
>>                                                  md_error                                     atomic_inc
>>    raid1_error
>>     atomic_dec //2to1
>>                         md_error
>>                          raid1_error           md_error
>>                           atomic_dec //1to0     raid1_error
>>                                                  atomic_dec //0
>>                                                    set MD_BROKEN
>>
>> * Alternatively, create a separate error handler,
>>    e.g. md_error_failfast(), that clearly does not fail the array.
>>
>> This approach would require somewhat larger changes and may not
>> be very elegant, but it seems to be a reliable way to ensure
>> MD_BROKEN is never set at the wrong timing.
>>
>> Which of these three approaches would you consider preferable?
>> I would appreciate your feedback.
>>
>>
>> For metadata writes, I plan to clear_bit MD_FAILFAST_SUPPORTED
>> when the array is degraded.
>>
>> Thanks,
>> Akagi
>>
> 
> I took a closer look at the FailFast code and found a few issues, using
> RAID1 as an example:
> 
> For normal read/write IO, FailFast is only triggered when there is another
> disk is available, as seen in read_balance() and raid1_write_request().
> In raid1_error(), MD_BROKEN is set only when no other disks are available.

Hi,
Agree, I think so too.

> So, the FailFast for normal read/write is not triggered in the scenario you
> described in cover-letter.

This corresponds to the case described in the commit message of PATCH v3 1/3.
"Normally, MD_FAILFAST IOs are not issued to the 'last' rdev, so this is
an edge case; however, it can occur if rdevs in a non-degraded
array share the same path and that path is lost, or if a metadata
write is triggered in a degraded array and fails due to failfast."

To describe it in more detail, the flow is as follows:

Prerequisites:

- Both rdevs are in-sync
- Both rdevs have in-flight MD_FAILFAST bios
- Both rdevs depend on the same lower-level path
  (e.g., nvme-tcp over a single Ethernet interface)

Sequence:

- A bios with REQ_FAILFAST_DEV fails (e.g., due to a temporary network outage),
  in the case of nvme-tcp:
   - The Ethernet connection is lost on the node where md is running over 5 seconds
   - Then the connection is restored. Idk the details of nvme-tcp implementation, 
     but it seems that failfast IOs finish only after the connection is back.
- All failfast bios fail, raid1_end_write_request is called.
- md_error() marks one rdev Faulty; the other rdev becomes the 'last' rdev.
- md_error() on the last rdev sets MD_BROKEN on the array - fail_last_dev=1 is unlikely.
- The write is retried via handle_write_finished -> narrow_write_error, usually succeeding.
- MD_BROKEN remains set, leaving the array in a state where no further writes can occur.

> Normal writes may call md_error() in narrow_write_error. Normal reads do
> not execute md_error() on the last disk.
> 
> Perhaps you should get more information to confirm how MD_BROKEN is set in
> normal read/write IO.

Should I add the above sequence of events to the cover letter, or commit message?

Thanks,
Akagi

> -- 
> Thanks,
> Nan
> 
> 

Powered by blists - more mailing lists