On 2025/08/15 10:26, Yu Kuai wrote:
> Hi,
>
> 在 2025/08/14 23:54, Kenta Akagi 写道:
>> On 2025/08/13 9:59, Yu Kuai wrote:
>>> Hi,
>>>
>>> 在 2025/08/12 17:01, Kenta Akagi 写道:
>>>> It is not intended for the array to fail when a metadata write with
>>>> MD_FAILFAST fails.
>>>> After commit 9631abdbf406 ("md: Set MD_BROKEN for RAID1 and RAID10"),
>>>> when md_error is called on the last device in RAID1/10,
>>>> the MD_BROKEN flag is set on the array.
>>>> Because of this, a failfast metadata write failure will
>>>> make the array "broken" state.
>>>>
>>>> If rdev is not Faulty even after calling md_error,
>>>> the rdev is the last device, and there is nothing except
>>>> MD_BROKEN that prevents writes to the array.
>>>> Therefore, by clearing MD_BROKEN, the array will not become
>>>> "broken" after a failfast metadata write failure.
>>>
>>> I don't understand here, I think MD_BROKEN is expected, the last
>>> rdev has IO error while updating metadata, the array is now broken
>>> and you can only read it afterwards. Allow using this broken array
>>> read-write might causing more severe problem like data loss.
>>>
>> Thank you for reviewing.
>>
>> I think that only when the bio has the MD_FAILFAST flag,
>> a metadata write failure to the last rdev should not make it
>> broken array at that point.
>>
>> This is because a metadata write with MD_FAILFAST is retried after
>> failure as follows:
>>
>> 1. In super_written, MD_SB_NEED_REWRITE is set in sb_flags.
>>
>> 2. In md_super_wait, which is called by the function that
>> executed md_super_write and waits for completion,
>> -EAGAIN is returned because MD_SB_NEED_REWRITE is set.
>>
>> 3. The caller of md_super_wait (such as md_update_sb)
>> receives a negative return value and then retries md_super_write.
>>
>> 4. The md_super_write function, which is called to perform
>> the same metadata write, issues a write bio
>> without MD_FAILFAST this time, because the rdev has LastDev flag.
>>
>> When a bio from super_written without MD_FAILFAST fails,
>> the array is truly broken, and MD_BROKEN should be set.
>>
>> A failfast bio, for example in the case of nvme-tcp ,
>> will fail immediately if the connection to the target is
>> lost for a few seconds and the device enters a reconnecting
>> state - even though it would recover if given a few seconds.
>> This behavior is exactly as intended by the design of failfast.
>>
>> However, md treats super_write operations fails with failfast as fatal.
>> For example, if an initiator - that is, a machine loading the md module -
>> loses all connections for a few seconds, the array becomes
>> broken and subsequent write is no longer possible.
>> This is the issue I am currently facing, and which this patch aims to fix.
>>
>> Should I add more context to the commit message? Please advise.
>
> Yes, please explain in detail in commit message.
>>
>> Thanks,
>> AKAGI
>>
>>> Thanks,
>>> Kuai
>>>
>>>>
>>>> Fixes: 9631abdbf406 ("md: Set MD_BROKEN for RAID1 and RAID10")
>>>> Signed-off-by: Kenta Akagi <k@xxxxxxx>
>>>> ---
>>>> drivers/md/md.c | 1 +
>>>> drivers/md/md.h | 2 +-
>>>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>>> index ac85ec73a409..3ec4abf02fa0 100644
>>>> --- a/drivers/md/md.c
>>>> +++ b/drivers/md/md.c
>>>> @@ -1002,6 +1002,7 @@ static void super_written(struct bio *bio)
>>>> md_error(mddev, rdev);
>>>> if (!test_bit(Faulty, &rdev->flags)
>>>> && (bio->bi_opf & MD_FAILFAST)) {
>>>> + clear_bit(MD_BROKEN, &mddev->flags);
>
> And I feel a beeter way is to set MD_BROKEN only if the last rdev
> failed, set it in middle and clear it is werid.
Copy.
I'll modify logic and commit message, then send it out as v2.
Thanks,
Akagi
> Thanks,
> Kuai
>
>>>> set_bit(MD_SB_NEED_REWRITE, &mddev->sb_flags);
>>>> set_bit(LastDev, &rdev->flags);
>>>> }
>>>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>>>> index 51af29a03079..2f87bcc5d834 100644
>>>> --- a/drivers/md/md.h
>>>> +++ b/drivers/md/md.h
>>>> @@ -332,7 +332,7 @@ struct md_cluster_operations;
>>>> * resync lock, need to release the lock.
>>>> * @MD_FAILFAST_SUPPORTED: Using MD_FAILFAST on metadata writes is supported as
>>>> * calls to md_error() will never cause the array to
>>>> - * become failed.
>>>> + * become failed while fail_last_dev is not set.
>>>> * @MD_HAS_PPL: The raid array has PPL feature set.
>>>> * @MD_HAS_MULTIPLE_PPLS: The raid array has multiple PPLs feature set.
>>>> * @MD_NOT_READY: do_md_run() is active, so 'array_state', ust not report that
>>>>
>>>
>>>
>> .
>>
>
>
