Re: [PATCH] md/raid1,raid10: fix IO handle for REQ_NOWAIT

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Zheng,


Thank you for the patch.

Am 12.06.25 um 15:21 schrieb Zheng Qixing:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> IO with REQ_NOWAIT should not set R1BIO_Uptodate when it fails,
> and bad blocks should also be cleared when REQ_NOWAIT IO succeeds.

It’d be great if you could add an explanation for the *should*. Why 
should it not be done?

Do you have a reproducer for this?

> Fixes: 9f346f7d4ea7 ("md/raid1,raid10: don't handle IO error for REQ_RAHEAD and REQ_NOWAIT")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   drivers/md/raid1.c  | 11 ++++++-----
>   drivers/md/raid10.c |  9 +++++----
>   2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 19c5a0ce5a40..a1cddd24b178 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -455,13 +455,13 @@ static void raid1_end_write_request(struct bio *bio)
>   	struct md_rdev *rdev = conf->mirrors[mirror].rdev;
>   	sector_t lo = r1_bio->sector;
>   	sector_t hi = r1_bio->sector + r1_bio->sectors;
> -	bool ignore_error = !raid1_should_handle_error(bio) ||
> -		(bio->bi_status && bio_op(bio) == REQ_OP_DISCARD);
> +	bool discard_error = bio->bi_status && bio_op(bio) == REQ_OP_DISCARD;

Excuse my ignorance. What is the difference between ignore and discard?

>   	/*
>   	 * 'one mirror IO has finished' event handler:
>   	 */
> -	if (bio->bi_status && !ignore_error) {
> +	if (bio->bi_status && !discard_error &&
> +	    raid1_should_handle_error(bio)) {
>   		set_bit(WriteErrorSeen,	&rdev->flags);
>   		if (!test_and_set_bit(WantReplacement, &rdev->flags))
>   			set_bit(MD_RECOVERY_NEEDED, &
> @@ -507,12 +507,13 @@ static void raid1_end_write_request(struct bio *bio)
>   		 * check this here.
>   		 */
>   		if (test_bit(In_sync, &rdev->flags) &&
> -		    !test_bit(Faulty, &rdev->flags))
> +		    !test_bit(Faulty, &rdev->flags) &&
> +		    (!bio->bi_status || discard_error))
>   			set_bit(R1BIO_Uptodate, &r1_bio->state);
>   
>   		/* Maybe we can clear some bad blocks. */
>   		if (rdev_has_badblock(rdev, r1_bio->sector, r1_bio->sectors) &&
> -		    !ignore_error) {
> +		    !bio->bi_status) {
>   			r1_bio->bios[mirror] = IO_MADE_GOOD;
>   			set_bit(R1BIO_MadeGood, &r1_bio->state);
>   		}
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index b74780af4c22..1848947b0a6d 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -458,8 +458,8 @@ static void raid10_end_write_request(struct bio *bio)
>   	int slot, repl;
>   	struct md_rdev *rdev = NULL;
>   	struct bio *to_put = NULL;
> -	bool ignore_error = !raid1_should_handle_error(bio) ||
> -		(bio->bi_status && bio_op(bio) == REQ_OP_DISCARD);
> +	bool discard_error = bio->bi_status && bio_op(bio) == REQ_OP_DISCARD;
> +	bool ignore_error = !raid1_should_handle_error(bio) || discard_error;
>   
>   	dev = find_bio_disk(conf, r10_bio, bio, &slot, &repl);
>   
> @@ -522,13 +522,14 @@ static void raid10_end_write_request(struct bio *bio)
>   		 * check this here.
>   		 */
>   		if (test_bit(In_sync, &rdev->flags) &&
> -		    !test_bit(Faulty, &rdev->flags))
> +		    !test_bit(Faulty, &rdev->flags) &&
> +		    (!bio->bi_status || discard_error))
>   			set_bit(R10BIO_Uptodate, &r10_bio->state);
>   
>   		/* Maybe we can clear some bad blocks. */
>   		if (rdev_has_badblock(rdev, r10_bio->devs[slot].addr,
>   				      r10_bio->sectors) &&
> -		    !ignore_error) {
> +		    !bio->bi_status) {
>   			bio_put(bio);
>   			if (repl)
>   				r10_bio->devs[slot].repl_bio = IO_MADE_GOOD;


Kind regards,

Paul