On Tue, Aug 27, 2019 at 12:39 AM Guoqing Jiang
<guoqing.jiang@xxxxxxxxxxxxxxx> 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.
>
Yes, this looks good.
Thanks,
Song
