Re: [PATCH v6 2/3] md/raid10: resize r10bio_pool for reshape

在 2026/6/23 21:00, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly allocated `new_pool` is leaked on the `reshape_backwards` error path.
> --
> 
> commit 2142096170ffdd8a1caabfde6f2461f6e7d64f61
> Author: Chen Cheng <[email protected]>
> 
> md/raid10: resize r10bio_pool for reshape
>      
> When reshape grows raid_disks, the pool must also switch to new geometry
> object size, and allocate a new geometry size pool and replace the old.
> 
> But not for shrinking reshape, because regular I/O can still use the
> prev geo for sectors that have not crossed reshape_progress yet.
> 
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index cee5a253a281d..d740744a97461 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
> [ ... ]
>> @@ -4411,6 +4422,11 @@ static int raid10_start_reshape(struct mddev *mddev)
>>   		return -EINVAL;
>>   
>>   	conf->offset_diff = min_offset_diff;
>> +	if (mddev->delta_disks > 0) {
>> +		new_pool = create_r10bio_pool(new.raid_disks);
>> +		if (!new_pool)
>> +			return -ENOMEM;
>> +	}
>>   	spin_lock_irq(&conf->device_lock);
>>   	if (conf->mirrors_new) {
>>   		memcpy(conf->mirrors_new, conf->mirrors,
> 
> [Severity: Medium]
> Will this newly allocated new_pool be leaked if the reshape_backwards
> error path is triggered?
> 
> If an array is configured for a reshape that expands the number of disks
> (delta_disks > 0), the new_pool is allocated here. However, if the
> component device sizes are also reduced such that the array is set to
> reshape backwards, it can hit a pre-existing error check a few lines
> down:

It's impossible use backward reshape while delta disk > 0 or change 
component size.

> 
> 	if (mddev->reshape_backwards) {
> 		sector_t size = raid10_size(mddev, 0, 0);
> 		if (size < mddev->array_sectors) {
> 			spin_unlock_irq(&conf->device_lock);
> 			pr_warn("md/raid10:%s: array size must be reduce before number of disks\n",
> 				mdname(mddev));
> 			return -EINVAL;
> 		}
> 
> This direct return skips the standard cleanup path at the abort label,
> causing the mempool_t structure and its preallocated elements to leak.
> Should we add a mempool_destroy(new_pool) here, or change the return to
> a goto abort?
>