Re: [patch]raid5: make release_stripe lockless

[NeilBrown <neilb@suse.de>]

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

On Thu, 28 Mar 2013 10:00:04 +0800 Shaohua Li <shli@kernel.org> wrote:

> On Thu, Mar 28, 2013 at 11:45:46AM +1100, NeilBrown wrote:
> > On Fri, 22 Mar 2013 14:36:17 +0800 Shaohua Li <shli@kernel.org> wrote:
> > 
> > > 
> > > Subject: raid5: make release_stripe lockless
> > > 
> > > release_stripe still has big lock contention. We just add the stripe to a llist
> > > without taking device_lock. We let the raid5d thread to do the real stripe
> > > release, which must hold device_lock anyway. In this way, release_stripe
> > > doesn't hold any locks.
> > > 
> > > The side effect is the released stripes order is changed. But sounds not a big
> > > deal, stripes are never handled in order. And I thought block layer can already
> > > do nice request merge, which means order isn't that important.
> > > 
> > > I kept the unplug release batch, which is unnecessary with this patch from lock
> > > contention avoid point of view, and actually if we delete it, the stripe_head
> > > release_list and lru can share storage. But the unplug release batch is also
> > > helpful for request merge. We probably can delay wakeup raid5d till unplug, but
> > > I'm still afraid of the case which raid5d is running.
> > 
> > Looks good, thanks.
> > 
> > One comment:
> > 
> > 
> > > +/* should hold conf->device_lock already */
> > > +static int release_stripe_list(struct r5conf *conf)
> > > +{
> > > +	struct stripe_head *sh;
> > > +	struct llist_node *node;
> > > +	int count = 0;
> > > +
> > > +	while (1) {
> > > +		node = llist_del_first(&conf->released_stripes);
> > > +		if (!node)
> > > +			break;
> > 
> > Why not:
> >  llist_for_each_entry(sh, llist_delete_all(&conf->released_stripes), release_list) {
> >       clear_bit()
> >       __release_stripe(conf, sh);
> >       count++;
> >  }
> 
> This absolutly is ok too. I didn't clearly remember why I do it in my way,
> maybe because new entry can be added. If you prefer llist_for_each_entry(), I
> can change it.

I already changed it :-)
http://git.neil.brown.name/git?p=md.git;a=commitdiff;h=023a4ef1fc49d060ab6f5e69146e56ef885375dc

Thanks,
NeilBrown

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