On Tue, Aug 27 2019, Guoqing Jiang wrote:
> Hi Song,
>
> Thanks a lot for your reply.
>
> On 8/26/19 10:07 PM, Song Liu wrote:
>> Hi Guoqing,
>>
>> Sorry for the delay.
>>
>> On Mon, Aug 26, 2019 at 6:46 AM Guoqing Jiang
>> <guoqing.jiang@xxxxxxxxxxxxxxx> wrote:
>> [...]
>>>> Maybe it makes sense to remove the checking of STRIPE_ACTIVE just like
>>>> commit 550da24f8d62f
>>>> ("md/raid5: preserve STRIPE_PREREAD_ACTIVE in break_stripe_batch_list").
>>>>
>>>> @@ -4606,8 +4607,7 @@ static void break_stripe_batch_list(struct
>>>> stripe_head *head_sh,
>>>>
>>>> list_del_init(&sh->batch_list);
>>>>
>>>> - WARN_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
>>>> - (1 << STRIPE_SYNCING) |
>>>> + WARN_ONCE(sh->state & ((1 << STRIPE_SYNCING) |
>>>> (1 << STRIPE_REPLACED) |
>>>> (1 << STRIPE_DELAYED) |
>>>> (1 << STRIPE_BIT_DELAY) |
>> This part looks good to me.
>>
>>>> @@ -4626,6 +4626,7 @@ static void break_stripe_batch_list(struct
>>>> stripe_head *head_sh,
>>>>
>>>> set_mask_bits(&sh->state, ~(STRIPE_EXPAND_SYNC_FLAGS |
>>>> (1 <<
>>>> STRIPE_PREREAD_ACTIVE) |
>>>> + (1 << STRIPE_ACTIVE) |
>>>> (1 << STRIPE_DEGRADED) |
>>>> (1 <<
>>>> STRIPE_ON_UNPLUG_LIST)),
>>>> head_sh->state & (1 << STRIPE_INSYNC));
>>>>
>> But I think we should not clear STRIPE_ACTIVE here. It should be
>> cleared at the end of handle_stripe().
>
> Yes, actually "clear_bit_unlock(STRIPE_ACTIVE, &sh->state)" is the last
> line in handle_stripe. So we only need to do one line change like.
>
>
> @@ -4606,8 +4607,7 @@ static void break_stripe_batch_list(struct
> stripe_head *head_sh,
>
> list_del_init(&sh->batch_list);
>
> - WARN_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
> - (1 << STRIPE_SYNCING) |
> + WARN_ONCE(sh->state & ((1 << STRIPE_SYNCING) |
> (1 << STRIPE_REPLACED) |
> (1 << STRIPE_DELAYED) |
> (1 << STRIPE_BIT_DELAY) |
>
> I will send formal patch if you are fine with above, thanks again.
This is probably OK.
I think that when the 'handle_flags' argument is zero, an error has
occurred and STRIPE_ACTIVE is expected to be set.
When it is non-zero, it shouldn't be set.
I'm not sure it is worth encoding that in the warning, but it probably
is worth making that clear in the commit message.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
