lkml.org 
[lkml]   [2016]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem
On Tue, Sep 06, 2016 at 03:58:00AM +0200, Andreas Mohr wrote:
> Hi,
>
> [no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]
>
> https://lkml.org/lkml/2016/9/5/832

Subscribe to lkml already.. also lkml.org is near useless these days,
please use any other archive.

> Two thoughts:
>
> ***multiple locks
>
> Don't have much insight into this
> (didn't spend much thinking on this),
> but of course it's unfortunate
> that two lock types need to be serviced
> rather than having the atomic handling exchange area/section
> be sufficiently guarded by one lock only
> (the question here could possibly be:
> what kind of currently existing
> structural disadvantage/layer distribution
> prevents us from being able to
> have things simply serviced
> within one granular lock area only?)

percpu rwsem is a sleeping lock, flc_flock is not. flc_flock also nests
under other non-sleeping locks, and therefore cannot (trivially) be made
a sleeping lock.

This is in the Changelog.

Furthermore, in the fast path of percpu_{down,up}_read() are exactly 0
atomic/serializing instructions.

> ***lock handling
>
> > + percpu_down_read(&file_rwsem);
> > spin_lock(&ctx->flc_lock);
>
>
> > spin_unlock(&ctx->flc_lock);
> > + percpu_up_read(&file_rwsem);
>
>
> These are repeated multiple times in this commit, thus error-prone.
>
> A possibly good way to commit-micro-manage this would be:
> 1. commit shoves things into a newly created encapsulation/wrapper helper
> stuff_lock(&flc_lock); /* <---- naming surely can be improved here */

No, because not all instances of flc_flock need the percpu-rwsem held,
creating such a wrapper could mistakenly create the impression it
should.

> That way it is pretty much guaranteed that:
> a) neither one nor the other lock type
> will get forgotten later, at a specific use site

That's what we have lockdep_assert_held() for. Note how this patch also
introduces percpu_rwsem_assert_held() usage, this insures we shall never
'forget' the appropriate locking.

> b) lock order will be correctly maintained at all times
> (AB-BA deadlock......)

lockdep is rather good at finding those, also might_sleep() debugging
would trivially catch those since percpu-rwsem is a sleeping lock while
fcl_flock is not.

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.043 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site