Re: [PATCH v5 05/16] md/raid1: implement pers->should_error()

["Yu Kuai" <yukuai@fnnas.com>]

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,