Hi,
在 2025/10/27 23:04, Kenta Akagi 写道:
> The failfast feature in RAID1 and RAID10 assumes that when md_error() is
> called, the array remains functional because the last rdev neither fails
> nor sets MD_BROKEN.
>
> However, the current implementation can cause the array to lose
> its last in-sync device or be marked as MD_BROKEN, which breaks the
> assumption and can lead to array failure.
>
> To address this issue, introduce a new handler, md_cond_error(), to
> ensure that failfast I/O does not mark the array as broken.
>
> md_cond_error() checks whether a device should be faulted based on
> pers->should_error(). This commit implements should_error() callback
> for raid1 personality, which returns true if faulting the specified rdev
> would cause the mddev to become non-functional.
>
> Signed-off-by: Kenta Akagi <k@mgml.me>
> ---
> drivers/md/raid1.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 202e510f73a4..69b7730f3875 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1732,6 +1732,40 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
> seq_printf(seq, "]");
> }
>
> +/**
> + * raid1_should_error() - Determine if this rdev should be failed
> + * @mddev: affected md device
> + * @rdev: member device to check
> + * @bio: the bio that caused the failure
> + *
> + * When failfast bios failure, rdev can fail, but the mddev must not fail.
> + * This function tells md_cond_error() not to fail rdev if bio is failfast
> + * and last rdev.
> + *
> + * Returns: %false if bio is failfast and rdev is the last in-sync device.
> + * Otherwise %true - should fail this rdev.
> + */
> +static bool raid1_should_error(struct mddev *mddev, struct md_rdev *rdev, struct bio *bio)
> +{
> + int i;
> + struct r1conf *conf = mddev->private;
> +
> + if (!(bio->bi_opf & MD_FAILFAST) ||
> + !test_bit(FailFast, &rdev->flags) ||
> + test_bit(Faulty, &rdev->flags))
> + return true;
The above checking is the same for raid1 and raid10, so I think it should go to
the caller, and you don't need to pass in bio.
> +
> + for (i = 0; i < conf->raid_disks; i++) {
> + struct md_rdev *rdev2 = conf->mirrors[i].rdev;
> +
> + if (rdev2 && rdev2 != rdev &&
> + test_bit(In_sync, &rdev2->flags) &&
> + !test_bit(Faulty, &rdev2->flags))
> + return true;
> + }
Why not use the same checking from raid1_error()? You can factor out a
helper for this. And BTW, now I feel it's better to name the new method
like rdev_last_in_sync().
Thanks,
Kuai
> + return false;
> +}
> +
> /**
> * raid1_error() - RAID1 error handler.
> * @mddev: affected md device.
> @@ -3486,6 +3520,7 @@ static struct md_personality raid1_personality =
> .free = raid1_free,
> .status = raid1_status,
> .error_handler = raid1_error,
> + .should_error = raid1_should_error,
> .hot_add_disk = raid1_add_disk,
> .hot_remove_disk= raid1_remove_disk,
> .spare_active = raid1_spare_active,