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

[NeilBrown <neilb@suse.de>]

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

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.

Thanks,
NeilBrown



> ---
>  drivers/md/raid5.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/md/raid5.c
> ===================================================================
> --- linux.orig/drivers/md/raid5.c	2012-06-01 14:13:46.846909398 +0800
> +++ linux/drivers/md/raid5.c	2012-06-01 14:22:07.944611949 +0800
> @@ -4023,6 +4023,38 @@ static struct stripe_head *__get_priorit
>  	return sh;
>  }
>  
> +struct raid5_plug {
> +	struct blk_plug_cb cb;
> +	struct stripe_head_batch batch;
> +};
> +static DEFINE_PER_CPU(struct raid5_plug, raid5_plugs);
> +
> +static void raid5_do_plug(struct blk_plug_cb *cb)
> +{
> +	struct raid5_plug *plug = container_of(cb, struct raid5_plug, cb);
> +
> +	release_stripe_flush_batch(&plug->batch);
> +	INIT_LIST_HEAD(&plug->cb.list);
> +}
> +
> +static void release_stripe_plug(struct stripe_head *sh)
> +{
> +	struct blk_plug *plug = current->plug;
> +	struct raid5_plug *raid5_plug;
> +
> +	if (!plug) {
> +		release_stripe(sh);
> +		return;
> +	}
> +	preempt_disable();
> +	raid5_plug = &__raw_get_cpu_var(raid5_plugs);
> +	release_stripe_add_batch(&raid5_plug->batch, sh);
> +
> +	if (list_empty(&raid5_plug->cb.list))
> +		list_add(&raid5_plug->cb.list, &plug->cb_list);
> +	preempt_enable();
> +}
> +
>  static void make_request(struct mddev *mddev, struct bio * bi)
>  {
>  	struct r5conf *conf = mddev->private;
> @@ -4153,7 +4185,7 @@ static void make_request(struct mddev *m
>  			if ((bi->bi_rw & REQ_SYNC) &&
>  			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>  				atomic_inc(&conf->preread_active_stripes);
> -			release_stripe(sh);
> +			release_stripe_plug(sh);
>  		} else {
>  			/* cannot get stripe for read-ahead, just give-up */
>  			clear_bit(BIO_UPTODATE, &bi->bi_flags);
> @@ -6170,6 +6202,14 @@ static struct md_personality raid4_perso
>  
>  static int __init raid5_init(void)
>  {
> +	int i;
> +
> +	for_each_present_cpu(i) {
> +		struct raid5_plug *plug = &per_cpu(raid5_plugs, i);
> +		plug->batch.count = 0;
> +		INIT_LIST_HEAD(&plug->cb.list);
> +		plug->cb.callback = raid5_do_plug;
> +	}
>  	register_md_personality(&raid6_personality);
>  	register_md_personality(&raid5_personality);
>  	register_md_personality(&raid4_personality);


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