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

Hi,

在 2026/7/11 18:03, Chen Cheng 写道:
> From: Chen Cheng <[email protected]>
>
> 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_bio)
>
> raid_end_discard_bio() is exactly the same.
>
> Signed-off-by: Chen Cheng <[email protected]>
> ---
>   drivers/md/raid10.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index db68fcc9e9be..d77f60db7660 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -329,24 +329,27 @@ 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;
> +	bool returned;
>   
> -	if (!test_and_set_bit(R10BIO_Returned, &r10_bio->state)) {
> -		if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
> -			bio->bi_status = BLK_STS_IOERR;
> +	returned = test_and_set_bit(R10BIO_Returned, &state);
> +	if (!returned && !test_bit(R10BIO_Uptodate, &state))
> +		bio->bi_status = BLK_STS_IOERR;
> +
> +	free_r10bio(r10_bio);
> +
> +	if (!returned)
>   		bio_endio(bio);
> -	}

Apparently this change is not enough, the R10BIO_Returned case still return
the master_bio before freeing r10_bio.

I think you should get an active_io reference while allocating r10_bio to
guarantee that no r10_bio is active while array is suspended.

>   
>   	/*
>   	 * Wake up any possible resync thread that waits for the device
>   	 * to go idle.
>   	 */
>   	allow_barrier(conf);
> -
> -	free_r10bio(r10_bio);
>   }
>   
>   /*
>    * Update disk head position estimator based on IRQ completion info.
>    */
> @@ -1579,13 +1582,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