Re: [PATCH 2/8] md/raid5: Ensure a batch member is not handled prematurely.

[Shaohua Li <shli@kernel.org>]

On Wed, May 27, 2015 at 09:34:51AM +1000, NeilBrown wrote:
> On Wed, 27 May 2015 08:35:32 +1000 NeilBrown <neilb@suse.de> wrote:
> 
> > Could you please review and possibly test the patch below?
> > 
> 
> well... that patch had a fairly obvious double-lock bug.
> Try this one.
> (oh, just saw your email that you spotted the lock bug :-)
> 
> 
> NeilBrown
> 
> From: NeilBrown <neilb@suse.de>
> Date: Wed, 27 May 2015 08:43:45 +1000
> Subject: [PATCH] md/raid5: close race between STRIPE_BIT_DELAY and batching.
> 
> The first time a write is added to a stripe, we need to set the
> bitmap bits (if a bitmap is active).
> While doing that the stripe is not locked and other writes could
> be added and then the stripe could be added to a batch.
> Once it has entered the batch it is too large to set STRIPE_BIT_DELAY
> as the batch head has taken over when the stripe will be written.
> 
> We cannot hold the spinlock while adding the bitmap bit,
> so introduce a new stripe_head flag 'STRIPE_BITMAP_PENDING' which
> indicates that adding to the bitmap is pending.  This prevents
> the stripe from being added to a batch.
> 
> Only the first thread to add a write to a stripe can set this bit,
> so it is safe for it to clear it again when it is done.
> 
> Reported-by: Shaohua Li <shli@kernel.org>
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 73b5376dad3b..dae587ecdf71 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -760,6 +760,7 @@ static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
>  static bool stripe_can_batch(struct stripe_head *sh)
>  {
>  	return test_bit(STRIPE_BATCH_READY, &sh->state) &&
> +		!test_bit(STRIPE_BITMAP_PENDING, &sh->state) &&
>  		is_full_stripe_write(sh);
>  }
>  
> @@ -3007,14 +3008,27 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
>  	pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",
>  		(unsigned long long)(*bip)->bi_iter.bi_sector,
>  		(unsigned long long)sh->sector, dd_idx);
> -	spin_unlock_irq(&sh->stripe_lock);
>  
>  	if (conf->mddev->bitmap && firstwrite) {
> +		/* Cannot hold spinlock over bitmap_startwrite,
> +		 * but must ensure this isn't added to a batch until
> +		 * we have added to the bitmap and set bm_seq.
> +		 * So set STRIPE_BITMAP_PENDING to prevent
> +		 * batching.
> +		 * Only the first thread to add a write to a stripe
> +		 * can set this bit, so we "own" it.
> +		 */
> +		WARN_ON(test_bit(STRIPE_BITMAP_PENDING, &sh->state));
I keep hitting this. the firstwrite is set for every device.

Thanks,
Shaohua