> 2025年5月6日 09:50，Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> 写道：
>
> 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 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?
>
> 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.
Yes, the logic is correct, but the code in stripe_ahead_of_reshape() indeed checks whether
this stripe head is NOT ahead of reshape, see the code below which I copy & paste from
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;
It returns ‘true’ when the sh range is in reshaped or reshaping range, not ahead of reshape range.
This is the expected logic, but the function name is misleading. That why I though the code logic
was incorrect at the first glance.
>
>
>>>
>>> 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.
>
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 }
For the above code, I don’t see how only unlikely(previous) help on branch prediction and prefetch,
IMHO the following form may help to achieve expected unlikely() result,
if (unlikely(previous && !stripe_ahead_of_reshape(mddev, conf, sh))) {
Thanks.
Coly Li
>
>>>
>> 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
