Replace STRIPE_SYNCING with STRIPE_ACTIVE in the subject ...
On 8/30/19 4:07 PM, Guoqing Jiang wrote:
On 8/30/19 1:26 AM, Song Liu wrote:
On Aug 29, 2019, at 8:00 AM, Guoqing Jiang
<guoqing.jiang@xxxxxxxxxxxxxxx> wrote:
Hi Song,
On 8/29/19 7:42 AM, Song Liu wrote:
I read the code again, and now I am not sure whether we are fixing
the issue.
This WARN_ONCE() does not run for head_sh, which should have
STRIPE_ACTIVE.
It only runs on other stripes in the batch, which should not have
STRIPE_ACTIVE.
From the original commit which introduced batch write, it has the
description
which I think is align with your above sentence.
"With below patch, we will automatically batch adjacent full stripe
write
together. Such stripes will be added to the batch list. Only the
first stripe
of the list will be put to handle_list and so run handle_stripe().".
Could you point me related code which achieve the above purpose?
Thanks.
Do you mean which code makes sure the batched stripe will not be
handled?
This is done via properly maintain STRIPE_HANDLE bit.
The raid5_make_request always set STRIPE_HANDLE for stripe which is
returned from
raid5_get_active_stripe, so seems the batched stripe could also set with
STRIPE_HANDLE
too, am I miss something?
I checked the STRIPE_IO_STARTED flag, it is set in the path:
handle_stripe -> ops_run_io.
Since both STRIPE_IO_STARTED and STRIPE_ACTIVE are valid, which means
the stripe
should be handling in the middle of handle_stripe.
Maybe the scenario is that raid5_make_request get a lone stripe, then it
is added to handle_list
since STRIPE_HANDLE is valid, then handle_stripe sets STRIPE_ACTIVE flag
to the stripe.
Meantime, another full write writes to the same stripe, and added stripe
to batch_list by:
raid5_make_request -> add_stripe_bio -> stripe_add_to_batch_list.
Then the warning is triggered when err happens in the batch head stripe.
There are two
ways to address the warning I think.
1. remove the checking of STRIPE_ACTIVE flag since it is possible that a
batched stripe
could have the flag.
2. if STRIPE_ACTIVE flag is valid, then don't add stripe to batch list.
Also, there is short window between set STRIPE_ACTIVE and clear the flag
in handle_stripe,
does it make sense to make rough change like this?
static void handle_stripe(struct stripe_head *sh)
{
struct stripe_head_state s;
@@ -4675,7 +4682,7 @@ static void handle_stripe(struct stripe_head *sh)
struct r5dev *pdev, *qdev;
clear_bit(STRIPE_HANDLE, &sh->state);
- if (test_and_set_bit_lock(STRIPE_ACTIVE, &sh->state)) {
+ if (test_bit(STRIPE_ACTIVE, &sh->state)) {
/* already being handled, ensure it gets handled
* again when current action finishes */
set_bit(STRIPE_HANDLE, &sh->state);
@@ -4683,9 +4690,9 @@ static void handle_stripe(struct stripe_head *sh)
}
if (clear_batch_ready(sh) ) {
- clear_bit_unlock(STRIPE_ACTIVE, &sh->state);
return;
}
+ set_bit(STRIPE_ACTIVE, &sh->state);
Regards,
Guoqing
