| 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: <dcb72e23-d0ea-c8b3-24db-5dd515f619a8@huaweicloud.com>
Date: Fri, 29 Aug 2025 10:54:52 +0800
From: Li Nan <linan666@...weicloud.com>
To: Kenta Akagi <k@...l.me>, 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 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.
> @@ -1995,8 +1996,12 @@ static int enough(struct r10conf *conf, int ignore)
> * - 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 raid10_error(struct mddev *mddev, struct md_rdev *rdev)
> {
> @@ -2006,6 +2011,13 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
> spin_lock_irqsave(&conf->device_lock, flags);
>
> if (test_bit(In_sync, &rdev->flags) && !enough(conf, rdev->raid_disk)) {
> + if (test_and_clear_bit(FailfastIOFailure, &rdev->flags)) {
> + spin_unlock_irqrestore(&conf->device_lock, flags);
> + pr_warn_ratelimited("md/raid10:%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) {
> @@ -2413,6 +2425,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
> continue;
> } else if (test_bit(FailFast, &rdev->flags)) {
> /* Just give up on this device */
> + set_bit(FailfastIOFailure, &rdev->flags);
> md_error(rdev->mddev, rdev);
> continue;
> }
> @@ -2865,8 +2878,10 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
> freeze_array(conf, 1);
> fix_read_error(conf, mddev, r10_bio);
> unfreeze_array(conf);
> - } else
> + } else {
> + set_bit(FailfastIOFailure, &rdev->flags);
> md_error(mddev, rdev);
> + }
>
> rdev_dec_pending(rdev, mddev);
> r10_bio->state = 0;
--
Thanks,
Nan
Powered by blists - more mailing lists