Re: [PATCH v6 3/3] md/raid10: free r10bio before ending master_bio in raid_end_bio_io() and raid_end_discard_bio()

[Chen Cheng]

在 2026/7/5 15:48, yu kuai 写道:
> Hi,
> 
> 在 2026/6/23 20:38, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@xxxxxxxxx>
>>
>> origin flow:
>>
>>         bio_endio(master_bio);   /* may drop active_io to zero */
>>         allow_barrier(conf);
>>         free_r10bio(r10_bio);    /* reads conf->geo, returns to pool */
>>
>> one scenario is:
>>
>>     CPU A (softirq, raid_end_bio_io)         CPU B (action_store) --> reshape
>>     ================================         ===============================
>>     bio_endio(master_bio)
>>       md_end_clone_io
>>         percpu_ref_put -> 0
>>                                              wait_event wakeup, and,
>>                                              	mddev_suspend return
>>                                              raid10_start_reshape:
>>                                                setup_geo(&conf->geo, new)
>>                                                ...
>>                                                mempool_destroy(old_pool)
>>                                                conf->r10bio_pool = new_pool
>>     allow_barrier(conf)
>>     free_r10bio(r10_bio)
>>       put_all_bios:
>>         for (i=0; i<conf->geo.raid_disks; i++)
>>             ==> old obj, new geo, OOB
>>       mempool_free(r10_bio, conf->r10bio_pool)
>>             ==> old-geometry obj freed into new pool
>>
>> so .. fix by reorder the flow:
>>
>> 	free_r10bio(r10_bio)
>> 	allow_barrier(conf)
>> 	bio_endio(master_io)
>>
>> raid_end_discard_bio() is exactly the same.
>>
>> Signed-off-by: Chen Cheng <chencheng@xxxxxxxxx>
>> ---
>>    drivers/md/raid10.c | 17 ++++++++++-------
>>    1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index d740744a9746..e44a9b6e95c7 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -330,24 +330,25 @@ static void reschedule_retry(struct r10bio *r10_bio)
>>     */
>>    static void raid_end_bio_io(struct r10bio *r10_bio)
>>    {
>>    	struct bio *bio = r10_bio->master_bio;
>>    	struct r10conf *conf = r10_bio->mddev->private;
>> +	unsigned long state = r10_bio->state;
>>    
>> -	if (!test_and_set_bit(R10BIO_Returned, &r10_bio->state)) {
>> -		if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
>> -			bio->bi_status = BLK_STS_IOERR;
>> -		bio_endio(bio);
>> -	}
>> +	free_r10bio(r10_bio);
>>    
>>    	/*
>>    	 * Wake up any possible resync thread that waits for the device
>>    	 * to go idle.
>>    	 */
>>    	allow_barrier(conf);
>>    
>> -	free_r10bio(r10_bio);
>> +	if (!test_and_set_bit(R10BIO_Returned, &state)) {
>> +		if (!test_bit(R10BIO_Uptodate, &state))
>> +			bio->bi_status = BLK_STS_IOERR;
>> +		bio_endio(bio);
>> +	}
> 
> Why is this moved to the end? I feel it's not necessary.
> 

yes, point is let endio after free r10bio.

free_r10bio(r10_bio) -> allow_barrier(conf) -> bio_endio(master_io) or

free_r10bio(r10_bio) -> bio_endio(master_io) -> allow_barrier(conf)

both order is correct to fix the bug.

>>    }
>>    
>>    /*
>>     * Update disk head position estimator based on IRQ completion info.
>>     */
>> @@ -1580,13 +1581,15 @@ static void raid_end_discard_bio(struct r10bio *r10bio)
>>    		if (!test_bit(R10BIO_Discard, &r10bio->state)) {
>>    			first_r10bio = (struct r10bio *)r10bio->master_bio;
>>    			free_r10bio(r10bio);
>>    			r10bio = first_r10bio;
>>    		} else {
>> +			struct bio *master_bio = r10bio->master_bio;
>> +
>>    			md_write_end(r10bio->mddev);
>> -			bio_endio(r10bio->master_bio);
>>    			free_r10bio(r10bio);
>> +			bio_endio(master_bio);
>>    			break;
>>    		}
>>    	}
>>    }
>>    
>