Re: [PATCH v3 21/21] md/raid5: split reshape bios before bitmap accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Returning `false` from `raid5_make_request()` after a split bio encounters `STRIPE_WAIT_RESHAPE` causes double-submission of the remainder bio, leaks the split bio, and hangs IO indefinitely.
--

--- Patch [21]: [PATCH v3 21/21] md/raid5: split reshape bios before bitmap accounting ---
commit a1d64516f61f7855ab4b16be870b2ea6fb0d09a0
Author: Yu Kuai <yukuai@fygo.io>

md/raid5: split reshape bios before bitmap accounting

This patch updates raid5_make_request() to use the shared
mddev_bio_split_at_reshape_offset() helper. This ensures RAID5 submits
only one-side bios to llbitmap during reshape.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index f70b2f6119801..30e18ce7c0e3d 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6184,6 +6184,14 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
>  		return true;
>  	}
>  
> +	bi = mddev_bio_split_at_reshape_offset(mddev, bi, NULL,
> +					       &conf->bio_split);
> +	if (!bi) {
> +		if (rw == WRITE)
> +			md_write_end(mddev);
> +		return true;
> +	}
> +

[Severity: Critical]
If this bio is split, `bi` is updated to the first half, while the original
bio (the remainder) is submitted asynchronously.

Later in raid5_make_request(), if make_stripe_request() returns
STRIPE_WAIT_RESHAPE, it sleeps and returns false:

	if (res == STRIPE_WAIT_RESHAPE) {
		DECLARE_COMPLETION_ONSTACK(done);
		WRITE_ONCE(bi->bi_private, &done);

		bio_endio(bi);

		wait_for_completion(&done);
		return false;
	}

When raid5_make_request() returns false, md_handle_request() retries the
request using its original `bio` pointer:

drivers/md/md.c:md_handle_request() {
	...
	if (!mddev->pers->make_request(mddev, bio)) {
		percpu_ref_put(&mddev->active_io);
		if (mddev_is_dm(mddev) && mddev->pers->prepare_suspend)
			return false;
		goto check_suspended;
	}
	...
}

Because the original `bio` is now the remainder that was already submitted
asynchronously, won't returning false here cause a double-submission
of the remainder bio?

Additionally, if the completion handler intercepts the completion of the first
half to wake the sleeper but skips calling bio_endio() on the split bio, won't
this leak the split bio? Because the bio was split and chained, does skipping
the completion prevent the parent bio's reference count from dropping to zero,
causing IO to hang indefinitely?

>  	logical_sector = bi->bi_iter.bi_sector & ~((sector_t)RAID5_STRIPE_SECTORS(conf)-1);
>  	bi->bi_next = NULL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785206690.git.yukuai@fygo.io?part=21