Re: [PATCH] raid1: fix nr_pending leak in REQ_ATOMIC bad-block error path

[Abd-Alrhman Masalkhi]

hi,

Thank you for the feedback.

On Mon, Jun 01, 2026 at 09:43 +0100, John Garry wrote:
> On 30/05/2026 16:14, Abd-Alrhman Masalkhi wrote:
>> In raid1_write_request(), each per-mirror loop iteration begins by
>> incrementing rdev->nr_pending. If a REQ_ATOMIC write encounters a
>> badblock within the requested range, the code jumps to err_handle
>> without dropping the reference taken for the current mirror.
>> 
>> err_handle's cleanup loop will only decrements for k < i and
>> r1_bio->bios[k] is non-NULL. The current slot is therefore skipped,
>> leaving its nr_pending reference leaked permanently. The reference
>> prevents the rdev from ever being removed, since raid1_remove_conf()
>> refuses to remove an rdev with nr_pending > 0.
>> 
>> Fix this by calling rdev_dec_pending() before jumping to err_handle.
>> 
>> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@xxxxxxxxx>
>
> FWIW,
>
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>
>> ---
>>   drivers/md/raid1.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 181400e147c0..0084bbc24076 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1580,8 +1580,10 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>>   				 * complexity of supporting that is not worth
>>   				 * the benefit.
>>   				 */
>> -				if (bio->bi_opf & REQ_ATOMIC)
>> +				if (bio->bi_opf & REQ_ATOMIC) {
>> +					rdev_dec_pending(rdev, mddev);
>
> It's not so nice that we have 2x locations that does the 
> rdev_dec_pending work
>

Are you suggesting deferring atomic_inc(&rdev->nr_pending) until after
the if (test_bit(WriteErrorSeen, &rdev->flags)) {..} block? The patch
is already in md-7.2; should I send a separate cleanup patch?

>>   					goto err_handle;
>> +				}
>>   
>>   				good_sectors = first_bad - r1_bio->sector;
>>   				if (good_sectors < max_sectors)
>

-- 
Best Regards,
Abd-Alrhman