On 3/18/20 12:13 PM, Pavel Reichl wrote: > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote: ... >> So, this function's call signature should change so that callers can >> communicate both _SHARED and _EXCL; and then you can pick the correct > > Thanks for the suggestion...but that's how v5 signature looked like > before Christoph and Eric requested change...on the grounds that > there're: > * confusion over a (true, true) set of args > * confusion of what happens if we pass (false, false). > >> "r" parameter value for the lockdep_is_held_type() call. Then all of >> this becomes: >> >> if !debug_locks: >> return rwsem_is_locked(rwsem) >> >> if shared and excl: >> r = -1 >> elif shared: >> r = 1 >> else: >> r = 0 >> return lockdep_is_held_type(rwsem, r) > > I tried to create a table for this code as well: <adding back the table headers> > (nolockdep corresponds to debug_locks == 0) > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > SHARED EXCL SHARED | EXCL > readlocked y n y > writelocked *n* y y > unlocked n n n > nolockdep readlocked y y y > nolockdep writelocked y y y > nolockdep unlocked n n n > > I think that when we query writelocked lock for being shared having > 'no' for an answer may not be expected...or at least this is how I > read the code. This might be ok, because a) it is technically correct (is it shared? /no/ it is exclusive), and b) in the XFS code today we never call: xfs_isilocked(ip, XFS_ILOCK_SHARED); it's always: xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); So I think that if we document the behavior clearly, the truth table above would be ok. Thoughts? -Eric
