Re: [PATCH] xfs: Fix false positive lockdep warning with sb_internal & fs_reclaim

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 07, 2020 at 10:40:12AM -0500, Waiman Long wrote:
> On 1/6/20 4:01 PM, Dave Chinner wrote:
> > On Mon, Jan 06, 2020 at 11:12:32AM -0500, Waiman Long wrote:
> >> On 1/3/20 9:36 PM, Dave Chinner wrote:
> >>> On Thu, Jan 02, 2020 at 10:52:08AM -0500, Waiman Long wrote:
> >>> IOWs, "fix" it by stating that "lockdep can't track freeze
> >>> dependencies correctly"?
> >> The lockdep code has a singular focus on spotting possible deadlock
> >> scenarios from a locking point of view.
> > IMO, lockdep only works for very simplistic locking strategies.
> > Anything complex requires more work to get lockdep annotations
> > "correct enough" to prevent false positives than it does to actually
> > read the code and very the locking is correct.
> >
> > Have you noticed we do runs of nested trylock-and-back-out-on-
> > failure because we lock objects in an order that might deadlock
> > because of cross-object state dependencies that can't be covered by
> > lockdep?  e.g. xfs_lock_inodes() which nests up to 5 inodes deep,
> > can nest 3 separate locks per inode and has to take into account
> > journal flushing depenedencies when locking multiple inodes?
> >
> > Lockdep is very simplisitic and the complex annotations we need to
> > handle situations like the above are very difficult to design,
> > use and maintainr. It's so much simpler just to use big hammers like
> > GFP_NOFS to shut up all the different types of false positives
> > lockdep throws up for reclaim context false positives because after
> > all these years there still isn't a viable mechanism to easily
> > express this sort of complex dependency chain.
>
> Regarding the trylock-and-back-out-on_failure code, do you think adding
> new APIs with timeout for mutex and rwsem and may be spin count for

Timeouts have no place in generic locking APIs. Indeed, in these
cases, timeouts do nothing to avoid the issues that require us to
use trylock-and-back-out algorithms, so timeouts do nothing but
hold off racing inode locks for unnecessarily long time periods
while we wait for something to happen somewhere else that we have no
direct control over....

> spinlock will help to at least reduce the number of failures that can
> happen in the code. RT mutex does have a rt_mutex_timed_lock(), but
> there is no equivalent for mutex and rwsem.

Realtime has all sorts of problems with blocking where normal
kernels don't (e.g. by turning spinlocks into mutexes) and so it
forces rt code to jump through all sorts of crazy hoops to prevent
priority inversions and deadlocks. If lock timeouts are necessary
to avoid deadlocks and/or priority inversions in your code, then
that indicates that there are locking algorithm problems that need
fixing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com