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

[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.

>   }
>   
>   /*
>    * 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;
>   		}
>   	}
>   }
>   

-- 
Thanks,
Kuai