Re: [patch 6/8] raid5: make_request use batch stripe release

[NeilBrown <neilb@suse.de>]

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

On Thu, 7 Jun 2012 14:33:58 +0800 Shaohua Li <shli@kernel.org> wrote:

> On Thu, Jun 07, 2012 at 11:23:45AM +1000, NeilBrown wrote:
> > On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <shli@kernel.org> wrote:
> > 
> > > make_request() does stripe release for every stripe and the stripe usually has
> > > count 1, which makes previous release_stripe() optimization not work. In my
> > > test, this release_stripe() becomes the heaviest pleace to take
> > > conf->device_lock after previous patches applied.
> > > 
> > > Below patch makes stripe release batch. When maxium strips of a batch reach,
> > > the batch will be flushed out. Another way to do the flush is when unplug is
> > > called.
> > > 
> > > Signed-off-by: Shaohua Li <shli@fusionio.com>
> > 
> > I like the idea of a batched release.
> > I don't like the per-cpu variables... and I don't think it is safe to only
> > allocate them for_each_present_cpu without support cpu-hot-plug.
> > 
> > I would much rather keep a list of stripes (linked on ->lru) in struct
> > md_plug_cb (or maybe in some structure which contains that) and release them
> > all on unplug - and only on unplug.
> > 
> > Maybe pass a size to mddev_check_unplugged, and it allocates that much more
> > space.  Get mddev_check_unplugged to return the md_plug_cb structure.
> > If the new space is NULL, then list_head_init it, and change the cb.callback
> > to a raid5 specific function.
> > Then add any stripe to the md_plug_cb, and in the unplug function, release
> > them all.
> > 
> > Does that make sense?
> > 
> > Also I would rather the batched stripe release code were defined in the same
> > patch that used it.  It isn't big enough to justify a separate patch.
> 
> The stripe->lru need protection of device_lock, so I can't use a list. An array
> is preferred. I really didn't like the idea to allocate memory especially when
> allocating an array. I'll fix the code for cpuhotplug.

You don't need device_lock to use ->lru.
Currently the lru is not used when sh->count is not-zero unless
STRIPE_EXPANDING is set - and we never attach IO requests if STRIPE_EXPANDING
is set.
So when make_request wants to release a stripe_head, ->lru is currently
unused.
So we can use it to put the stripe on a per-thread list without locking.

We need another stripe_head flag to say "is on a per-thread unplug list" to
avoid racing between processes, but we don't need a spinlock for that.
ie.
  if (!test_and_set(STRIPE_ON_UNPLUG_LIST, &sh->state))
           list_add(&plug->list, &sh->lru);

or similar.

Please do waste time on cpuhotplug - it isn't the right solution.

NeilBrown

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