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

[NeilBrown <neilb@suse.de>]

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

On Mon, 12 Aug 2013 10:24:37 +0800 Shaohua Li <shli@kernel.org> wrote:

> get_active_stripe() is the last place we have lock contention. It has two
> paths. One is stripe isn't found and new stripe is allocated, the other is
> stripe is found.

Hi Shaohua Li,
 thanks for the patch.  I think it is a good idea but it needs more work.
But first we will need to fix some bugs ... in md.c and in your patch.

> 
> The first path basically calls __find_stripe and init_stripe. It accesses
> conf->generation, conf->previous_raid_disks, conf->raid_disks,
> conf->prev_chunk_sectors, conf->chunk_sectors, conf->max_degraded,
> conf->prev_algo, conf->algorithm, the stripe_hashtbl and inactive_list. Except
> stripe_hashtbl and inactive_list, other fields are changed very rarely.

Yes, those fields don't change very often, but our current locking doesn't
properly protect against them changing.
In particular in "make_request()", if  raid5_start_reshape() changes these
fields between the point where reshape_progress is seen to be MaxSector, and
where get_active_stripe() is called, get_active_stripe will return the wrong
stripe.

I think we should probably introduce a seqlock to protect these fields.
It is very cheap to get a read-lock on a seqlock so we can do that every time
we enter make_request.

Then get_active_stripe wouldn't need to worry about device_lock at all and
would only need to get the hash lock for the particular sector.  That should
make it a lot simpler.

Also your new shrink_stripes() and similar code in resize_stripes is wrong.
It seems to assume that the stripe_heads will be evenly distributed over all
hash values, which isn't the case.

In particular, shrink_stripes() will stop calling drop_one_stripe() as soon
as any inactive_list is empty, but it must continue until all inactive lists
are empty.

I'll add the seqlock and push that out to my for-next branch, and then you
can rebase this patch on top of that.

Thanks,
NeilBrown


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