Hi Neil,
Thanks for your comment!
On 8/30/19 10:43 AM, NeilBrown wrote:
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.
Yes, indeed. When error (STRIPE_BATCH_ERR or lost more than max_degraded
devices)
happens, handle_stripe calls break_stripe_batch_list with set
'handle_flags' to zero.
I'm not sure it is worth encoding that in the warning, but it probably
is worth making that clear in the commit message.
Maybe something like this.
@@ -4607,24 +4609,25 @@ static void break_stripe_batch_list(struct
stripe_head *head_sh,
struct stripe_head *sh, *next;
int i;
int do_wakeup = 0;
-
+ unsigned long warn_states = (1 << STRIPE_SYNCING) |
+ (1 << STRIPE_REPLACED) |
+ (1 << STRIPE_BIT_DELAY) |
+ (1 << STRIPE_FULL_WRITE) |
+ (1 << STRIPE_BIOFILL_RUN) |
+ (1 << STRIPE_COMPUTE_RUN) |
+ (1 << STRIPE_OPS_REQ_PENDING) |
+ (1 << STRIPE_DISCARD) |
+ (1 << STRIPE_BATCH_READY) |
+ (1 << STRIPE_BATCH_ERR) |
+ (1 << STRIPE_BITMAP_PENDING);
+
+
+ if (handle_flags)
+ warn_states = warn_states | (1 << STRIPE_ACTIVE);
list_for_each_entry_safe(sh, next, &head_sh->batch_list,
batch_list) {
list_del_init(&sh->batch_list);
-
- WARN_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
- (1 << STRIPE_SYNCING) |
- (1 << STRIPE_REPLACED) |
- (1 << STRIPE_DELAYED) |
- (1 << STRIPE_BIT_DELAY) |
- (1 << STRIPE_FULL_WRITE) |
- (1 << STRIPE_BIOFILL_RUN) |
- (1 << STRIPE_COMPUTE_RUN) |
- (1 << STRIPE_OPS_REQ_PENDING) |
- (1 << STRIPE_DISCARD) |
- (1 << STRIPE_BATCH_READY) |
- (1 << STRIPE_BATCH_ERR) |
- (1 << STRIPE_BITMAP_PENDING)),
+ WARN_ONCE(sh->state & warn_states,
"stripe state: %lx\n", sh->state);
Best Regards,
Guoqing
