Re: [PATCH v2 20/20] md/raid5: split reshape bios before bitmap accounting

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Splitting a bio in `raid5_make_request()` breaks the `STRIPE_WAIT_RESHAPE` retry logic, causing a double-submission of the remainder and a permanent leak of the split front.

Pre-existing issues:
- [High] `make_discard_request()` leaks the `REQ_OP_DISCARD` bio without completion if an array is reshaping or `REQ_NOWAIT` is set.
--

commit fd0cb884709b54fc9c1fe1f70aa2016108b2a998
Author: Yu Kuai <[email protected]>

md/raid5: split reshape bios before bitmap accounting

The commit uses mddev_bio_split_at_reshape_offset() in
raid5_make_request() to split bios at the reshape offset. This ensures
that RAID5 submits only one-side bios to llbitmap during reshape.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index ac7ea483502fd..fb346a3c4aa3b 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6181,6 +6181,14 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)

[ ... ]

>  		return true;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but is it possible for the discard bio to be
leaked if the array is reshaping or REQ_NOWAIT is set?

In make_discard_request() which is called just above this context, it returns
early without completing the bio:

    if (WARN_ON_ONCE(bi->bi_opf & REQ_NOWAIT))
        return;

    if (mddev->reshape_position != MaxSector)
        return;

Since raid5_make_request() calls make_discard_request() and then unconditionally
returns true, does this strand the bio and cause indefinite hangs?

> +	bi = mddev_bio_split_at_reshape_offset(mddev, bi, NULL,
> +					       &conf->bio_split);

[Severity: Critical]
Does splitting the bio here break the STRIPE_WAIT_RESHAPE retry logic and cause
a double-submission and memory leak?

When mddev_bio_split_at_reshape_offset() splits the bio, it submits the
remainder via bio_submit_split_bioset() and returns the front half as the
new bi.

Later in raid5_make_request(), if make_stripe_request() hits the reshape window
and returns STRIPE_WAIT_RESHAPE, it handles the retry:

    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 false is returned, md_handle_request() loops and retries the original
bio pointer.

However, the original bio was already submitted as the remainder by the split
operation. Will this result in concurrently submitting the remainder twice?

Additionally, when completing the cloned front half (bi) with the completion
flag, md_end_clone_io() does not complete the original split bio, which
permanently leaks the front half.

> +	if (!bi) {
> +		if (rw == WRITE)
> +			md_write_end(mddev);
> +		return true;
> +	}
> +
>  	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/[email protected]?part=20