linux-kernel - Re: [PATCH] md/raid1,raid10: fix IO handle for REQ

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <34f22e12-62e7-4e0c-9fd2-b4bacd95a4fc@huawei.com>
Date: Sat, 14 Jun 2025 14:50:41 +0800
From: Zheng Qixing <zhengqixing@...wei.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, Zheng Qixing
	<zhengqixing@...weicloud.com>
CC: <song@...nel.org>, <yukuai3@...wei.com>, <linux-raid@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>, <yangerkun@...wei.com>
Subject: Re: [PATCH] md/raid1,raid10: fix IO handle for REQ_NOWAIT

Hello Paul,


Please disregard the previous reply email, as it contained garbled text.


在 2025/6/13 16:02, Paul Menzel 写道:
> Dear Zheng,
>
>
> Thank you for the patch.
>
> Am 12.06.25 um 15:21 schrieb Zheng Qixing:
>> From: Zheng Qixing <zhengqixing@...wei.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?
>

If we set R1BIO_Uptodate when IO with REQ_NOWAIT fails, the request will
return a success.

But actually it should return BLK_STS_IOERR or BLK_STS_AGAIN, right?


>> Fixes: 9f346f7d4ea7 ("md/raid1,raid10: don't handle IO error for 
>> REQ_RAHEAD and REQ_NOWAIT")
>> Signed-off-by: Zheng Qixing <zhengqixing@...wei.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?


REQ_OP_DISCARD is a operation type while REQ_NOWAIT is just a request flag.

These two can be combined together. IO with REQ_NOWAIT can fail early, even

though the storage medium is fine. So, we better handle this type of
error specially.


I hope this clarifies your doubts.


>
>>       /*
>>        * '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


Regards,

Zheng