Re: [PATCH v3 04/13] md/raid1,raid10: set Uptodate and clear badblocks if narrow_write_error success

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

Hi,

在 2025/12/15 11:04, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
>
> narrow_write_error() returns true when all sectors are rewritten
> successfully. In this case, set R1BIO_Uptodate to return success
> to upper layers and clear correspondinga badblocks.
>
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   drivers/md/raid1.c  | 24 ++++++++++++++++++++----
>   drivers/md/raid10.c | 17 +++++++++++++++--
>   2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 9ffa3ab0fdcc..bd63cf039381 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2587,9 +2587,10 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>   	int m, idx;
>   	bool fail = false;
>   
> -	for (m = 0; m < conf->raid_disks * 2 ; m++)
> +	for (m = 0; m < conf->raid_disks * 2 ; m++) {
> +		struct md_rdev *rdev = conf->mirrors[m].rdev;
> +
>   		if (r1_bio->bios[m] == IO_MADE_GOOD) {
> -			struct md_rdev *rdev = conf->mirrors[m].rdev;
>   			rdev_clear_badblocks(rdev,
>   					     r1_bio->sector,
>   					     r1_bio->sectors, 0);
> @@ -2599,11 +2600,26 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>   			 * narrow down and record precise write
>   			 * errors.
>   			 */
> -			fail = true;
> -			narrow_write_error(r1_bio, m);
> +			if (narrow_write_error(r1_bio, m)) {
> +				/* re-write success */
> +				if (rdev_has_badblock(rdev,
> +						      r1_bio->sector,
> +						      r1_bio->sectors))
> +					rdev_clear_badblocks(rdev,
> +							     r1_bio->sector,
> +							     r1_bio->sectors, 0);
> +				if (test_bit(In_sync, &rdev->flags) &&
> +				    !test_bit(Faulty, &rdev->flags))
> +					set_bit(R1BIO_Uptodate,
> +						&r1_bio->state);

Clear badblocks is fine, although I can't think of any case write io can be
issued to rdev with badblocks existing. And I think it's fine to move above
codes into narrow_write_error() and keep declaring it as void.

> +			} else {
> +				fail = true;

Why change the fail here, you also change the logic to return this r1bio early,
before this change, r1bio is returned from raid1d() after updating sb. And you
don't explain why this is safe.

                 /*
                  * In case freeze_array() is waiting for condition
                  * get_unqueued_pending() == extra to be true.
                  */

> +			}
> +
>   			rdev_dec_pending(conf->mirrors[m].rdev,
>   					 conf->mddev);
>   		}
> +	}
>   	if (fail) {
>   		spin_lock_irq(&conf->device_lock);
>   		list_add(&r1_bio->retry_list, &conf->bio_end_io_list);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 21a347c4829b..db6fbd423726 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2939,8 +2939,21 @@ static void handle_write_completed(struct r10conf *conf, struct r10bio *r10_bio)
>   					r10_bio->sectors, 0);
>   				rdev_dec_pending(rdev, conf->mddev);
>   			} else if (bio != NULL && bio->bi_status) {
> -				fail = true;
> -				narrow_write_error(r10_bio, m);
> +				if (narrow_write_error(r10_bio, m)) {
> +					/* re-write success */
> +					if (rdev_has_badblock(rdev,
> +							r10_bio->devs[m].addr,
> +							r10_bio->sectors))
> +						rdev_clear_badblocks(
> +							rdev,
> +							r10_bio->devs[m].addr,
> +							r10_bio->sectors, 0);
> +					if (test_bit(In_sync, &rdev->flags) &&
> +					    !test_bit(Faulty, &rdev->flags))
> +						set_bit(R10BIO_Uptodate, &r10_bio->state);
> +				} else {
> +					fail = true;
> +				}
>   				rdev_dec_pending(rdev, conf->mddev);
>   			}
>   			bio = r10_bio->devs[m].repl_bio;

-- 
Thansk,
Kuai