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

[Shaohua Li <shli@kernel.org>]

On Wed, Aug 28, 2013 at 02:32:52PM +1000, NeilBrown wrote:
> On Tue, 27 Aug 2013 16:53:30 +0800 Shaohua Li <shli@kernel.org> wrote:
> 
> > On Tue, Aug 27, 2013 at 01:17:52PM +1000, NeilBrown wrote:
> 
> > 
> > > 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.
> > 
> > did you mean get_active_stripe() doesn't need device_lock for any code path?
> > How could it be safe? device_lock still protects something like handle_list,
> > delayed_list, which release_stripe() will use while a get_active_stripe can run
> > concurrently.
> 
> Yes you will still need device_lock to protect list_del_init(&sh->lru),
> as well as the hash lock.
> Do you need device_lock anywhere else in there?

That's what I mean. So I need get both device_lock and hash_lock. To not
deadlock, I need release hash_lock and relock device_lock/hash_lock. Since I
release lock, I need recheck if I can find the stripe in hash again. So the
seqcount locking doesn't simplify things here. I thought the seqlock only fixes
one race. Did I miss anything?

I saw your tree only has seqcount_write lock in one place, but there are still
other places which changing quiesce, degraded. I thought we still need lock all
locks like what I did.

Thanks,
Shaohua