On Mon, May 05, 2025 at 11:28:31PM +0800, colyli@xxxxxxxxxx wrote:
> From: Coly Li <colyli@xxxxxxxxxx>
>
> Commit f4aec6a09738 ("md/raid5: Factor out helper from
> raid5_make_request() loop") added the following code block to check
After read historical commits, I realize the change was from
commit 486f60558607 ("md/raid5: Check all disks in a stripe_head for
reshape progress").
> whether the reshape range passed the stripe head range during thetime to
> wait for a valid struct stripe_head object,
>
> 5971 if (unlikely(previous) &&
> 5972 stripe_ahead_of_reshape(mddev, conf, sh)) {
> 5973 /*
> 5974 * Expansion moved on while waiting for a stripe.
> 5975 * Expansion could still move past after this
> 5976 * test, but as we are holding a reference to
> 5977 * 'sh', we know that if that happens,
> 5978 * STRIPE_EXPANDING will get set and the expansion
> 5979 * won't proceed until we finish with the stripe.
> 5980 */
> 5981 ret = STRIPE_SCHEDULE_AND_RETRY;
> 5982 goto out_release;
> 5983 }
>
> But from the code comments and context, the if statement should check
> whether stripe_ahead_of_reshape() returns false, then the code logic can
> match the context that reshape range went accross the sh range during
> raid5_get_active_stripe().
And the code logic might be correct, because inside
stripe_ahead_of_reshape(),
5788 if (!range_ahead_of_reshape(mddev, min_sector, max_sector,
5789 conf->reshape_progress))
5790 /* mismatch, need to try again */
5791 ret = true;
true is returned when the sh range is NOT ahead of reshape position.
Then the logic makes sense, but the function name stripe_ahead_of_reshape()
is really misleading IMHO. Maybe it should be named something like
stripe_behind_reshape()?
Should we change the name? Or I missed something important and still don't
understand the code correctly?
>
> And unlikely(previous) seems useless inside the if statement, and the
> unlikely() should include all checking statemetns.
>
This part still valid IMHO.
Thanks for comments.
> This patch has both of the above changes, hope it can make the code be
> more comfortable.
>
> Fixes: f4aec6a09738 ("md/raid5: Factor out helper from raid5_make_request() loop")
> Signed-off-by: Coly Li <colyli@xxxxxxxxxx>
> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Cc: Yu Kuai <yukuai3@xxxxxxxxxx>
> Cc: Xiao Ni <xni@xxxxxxxxxx>
> Cc: Song Liu <song@xxxxxxxxxx>
> ---
> drivers/md/raid5.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 39e7596e78c0..030e4672ab18 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5969,8 +5969,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
> return STRIPE_FAIL;
> }
>
> - if (unlikely(previous) &&
> - stripe_ahead_of_reshape(mddev, conf, sh)) {
> + if (unlikely(previous && !stripe_ahead_of_reshape(mddev, conf, sh))) {
> /*
> * Expansion moved on while waiting for a stripe.
> * Expansion could still move past after this
> --
> 2.39.5
>
--
Coly Li
