Re: [patch 3/3] raid5: relieve lock contention in get_active_stripe()

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Tue, 10 Sep 2013 10:35:55 +0800 Shaohua Li <shli@kernel.org> wrote:

> On Tue, Sep 10, 2013 at 11:13:18AM +1000, NeilBrown wrote:
> > On Mon, 9 Sep 2013 12:33:18 +0800 Shaohua Li <shli@kernel.org> wrote:
> > >  		} else {
> > > +			spin_lock(&conf->device_lock);
> > > +
> > >  			if (atomic_read(&sh->count)) {
> > >  				BUG_ON(!list_empty(&sh->lru)
> > >  				    && !test_bit(STRIPE_EXPANDING, &sh->state)
> > > @@ -611,13 +725,14 @@ get_active_stripe(struct r5conf *conf, s
> > >  					sh->group = NULL;
> > >  				}
> > >  			}
> > > +			spin_unlock(&conf->device_lock);
> > 
> > The device_lock is only really needed in the 'else' branch of the if
> > statement.  So can we have it only there.  i.e. don't take the lock if
> > sh->count is non-zero.
> 
> This is correct, I assume this isn't worthy optimizing before. Will fix soon.

It isn't really about optimising performance.  It is about making the code
easier to understand.  If we keep the region covered by the lock as small as
reasonably possible, it makes it more obvious to the reader which values are
being protected.

 
> > > -	spin_lock_irqsave(&conf->device_lock, flags);
> > > +	lock_all_device_hash_locks_irqsave(conf, &flags);
> > >  	clear_bit(In_sync, &rdev->flags);
> > >  	mddev->degraded = calc_degraded(conf);
> > > -	spin_unlock_irqrestore(&conf->device_lock, flags);
> > > +	unlock_all_device_hash_locks_irqrestore(conf, &flags);
> > >  	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> > 
> > Why do you think you need to take all the hash locks here and elsewhere when
> > ->degraded is set?
> > The lock is only need to ensure that the 'In_sync' flags are consistent with
> > the 'degraded' count.
> > ->degraded isn't used in get_active_stripe so I cannot see how it is relevant
> > to the hash locks.
> > 
> > We need to lock everything in raid5_quiesce().  I don't think we need to
> > anywhere else.
> 
> init_stripe() accesses some filelds, don't need to protect?

What fields?  Not ->degraded.

I think the fields that it accesses are effectively protected by the new
seqlock.
If you don't think so, please be explicit.


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]