| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3ea67e48-ce8a-9d70-a128-edf5eddf15f0@huaweicloud.com>
Date: Sat, 30 Aug 2025 16:48:07 +0800
From: Li Nan <linan666@...weicloud.com>
To: Kenta Akagi <k@...l.me>, 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
Subject: Re: [PATCH v3 1/3] md/raid1,raid10: Do not set MD_BROKEN on failfast
io failure
在 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.
So, the FailFast for normal read/write is not triggered in the scenario you
described in cover-letter.
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.
--
Thanks,
Nan
Powered by blists - more mailing lists