Hi, 在 2025/05/05 23:47, Coly Li 写道:
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 checkAfter 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?
The logic is correct. What I understand is that the *ahead of* means reshape is not performed yet in this area, not that the location is ahead of reshape position. And 'reshape_backwards' can make location comparison different completely. Noted there is a local variable 'previous' with the same logical. Instead of 'behind', I'll perfer another name to reflect this.
And unlikely(previous) seems useless inside the if statement, and the unlikely() should include all checking statemetns.
And I think the unlikely is fine, this is called from IO path, and the 'previous' is unlikely to set. Thanks, Kuai
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
