Re: [PATCH v7 0/4] xfs: Remove wrappers for some semaphores

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 23, 2020 at 09:33:42AM -0700, Darrick J. Wong wrote:
> On Mon, Mar 23, 2020 at 10:22:02AM +0100, Pavel Reichl wrote:
> > Oh, thanks for the heads up...I'll try to investigate.
> 
> Ahah, I figured it out.  It took me a while to pin down a solid reproducer,
> but here's a stack trace that I see most often:
....
> lockdep tracks the rwsem's lock state /and/ which process actually
> holds the rwsem.  This ownership doesn't transfer from 28177 to 27081,
> so when the kworker asks lockdep if it holds ILOCK_EXCL, lockdep says
> no, because 27081 doesn't own the lock, 28177 does.  Kaboom.

Yeah, linux has this weird idea that semaphores can only be accessed
from a single process context, like a mutex. We've beaten our head
against this many times, and it's the only reason struct semaphore
still exists and everything isn't a mutex.

rwsems now do crazy stuff like track the owner for spinning
optimisations, despite the fact it's a semaphore and, by definition,
can be released by non-owner contexts....

> The old mrlock_t had that 'int mr_writer' field which didn't care about
> lock ownership and so isilocked would return true and so the assert was
> happy.
> 
> So now comes the fun part -- the old isilocked code had a glaring hole
> in which it would return true if *anyone* held the lock, even if the
> owner is some other unrelated thread.  That's probably good enough for
> most of the fstests because we generally only run one thread at a time,
> and developers will probably notice. :)

No, that's not a bug, that's how semaphores work - semaphores
protect the object, not the process context that took the lock.

i.e. the difference between a mutex and a semaphore is that the
mutex protects a code path from running concurrently with other
processes, while a semaphore protects an object from other accesses,
even when they don't have a process context associated with them
(e.g. while they are under IO).

> However, with your series applied, the isilocked function becomes much
> more powerful when lockdep is active because now we can test that the
> lock is held *by the caller*, which closes that hole.

Not really, it's just another "lockdep behaviour is wrong" issue we
have to work around. When we switch to a worker, we need to release
and acquire the lockdep context so that it thinks the current
process working on the object owns the lock.

> Unfortunately, it also trips over this bmap split case, so if there's a
> way to solve that problem we'd better find it quickly.  Unfortunately, I
> don't know of a way to gift a lock to another thread temporarily...

rwsem_release() when the work is queued, rwsem_acquire() when the
work is run. And vice versa when the work is done.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com