linux-kernel - Re: [PATCH v4 5/9] md/raid1,raid10: Set R{1,10}BIO_Uptodate when successful retry of a failed bio

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: <010601995d7867c6-b8e08c65-5db3-4616-abd3-ec03a92ec8bc-000000@ap-northeast-1.amazonses.com>
Date: Thu, 18 Sep 2025 15:36:36 +0000
From: Kenta Akagi <k@...l.me>
To: linan666@...weicloud.com, song@...nel.org, yukuai3@...wei.com, 
	mtkaczyk@...nel.org, shli@...com, jgq516@...il.com
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org, k@...l.me
Subject: Re: [PATCH v4 5/9] md/raid1,raid10: Set R{1,10}BIO_Uptodate when
 successful retry of a failed bio



On 2025/09/18 15:39, Li Nan wrote:
> 
> 
> 在 2025/9/17 21:20, Kenta Akagi 写道:
> 
>>>>                if (!narrow_write_error(r1_bio, m))
>>>> -                md_error(conf->mddev,
>>>> -                     conf->mirrors[m].rdev);
>>>> +                md_error(conf->mddev, rdev);
>>>>                    /* an I/O failed, we can't clear the bitmap */
>>>> -            rdev_dec_pending(conf->mirrors[m].rdev,
>>>> -                     conf->mddev);
>>>> +            else if (test_bit(In_sync, &rdev->flags) &&
>>>> +                 !test_bit(Faulty, &rdev->flags) &&
>>>> +                 rdev_has_badblock(rdev,
>>>> +                           r1_bio->sector,
>>>> +                           r1_bio->sectors) == 0)
>>>
>>> Clear badblock and set R10BIO_Uptodate if rdev has badblock.
>>
>> narrow_write_error returns true when the write succeeds, or when the write
>> fails but rdev_set_badblocks succeeds. Here, it determines that the re-write
>> succeeded if there is no badblock in the sector to be written by r1_bio.
>> So we should not call rdev_clear_badblocks here.
>>
> 
> I am trying to cleanup narrow_write_error():
> 
> https://lore.kernel.org/linux-raid/20250917093508.456790-3-linan666@huaweicloud.com/T/#u
> 
> It may be clearer if narrow_write_error() returns true when all fix IO
> succeeds.
> 
> ```
> @@ -2553,11 +2551,17 @@ static bool narrow_write_error(struct r1bio *r1_bio, int i)
>                 bio_trim(wbio, sector - r1_bio->sector, sectors);
>                 wbio->bi_iter.bi_sector += rdev->data_offset;
> 
> -               if (submit_bio_wait(wbio) < 0)
> -                       /* failure! */
> -                       ok = rdev_set_badblocks(rdev, sector,
> -                                               sectors, 0)
> -                               && ok;
> +               if (submit_bio_wait(wbio) < 0) {
> +                       ok = false;
> +                       if (rdev_set_badblocks(rdev, sector, sectors, 0)) {
> +                               /*
> +                                * Badblocks set failed, disk marked Faulty.
> +                                * No further operations needed.
> +                                */
> +                               bio_put(wbio);
> +                               break;
> +                       }
> +               }
> 
>                 bio_put(wbio);
>                 sect_to_write -= sectors;
> ```
> 
> We can clear badblocks and set R10BIO_Uptodate after it. What do you think?

Thanks for the detailed explanation.
narrow_write_error now returns whether the retry write succeeded.
If rdev_set_badblock fails, narrow_write_error calls md_error. This looks good to me.
And after narrow_write_error cleanup, rdev_clear_badblock should be called.

BTW, I'm not familiar with the patch workflow. Should I submit my
handle_write_finished patch separately after your cleanup patch merged?

Thanks,
Akagi

> -- 
> Thanks,
> Nan
> 
>