On Wed 26-05-21 08:32:51, Darrick J. Wong wrote:

On Wed, May 26, 2021 at 12:18:40PM +0200, Jan Kara wrote:

quoted

On Tue 25-05-21 14:37:29, Darrick J. Wong wrote:

quoted

On Tue, May 25, 2021 at 03:50:44PM +0200, Jan Kara wrote:

quoted

Use invalidate_lock instead of XFS internal i_mmap_lock. The intended
purpose of invalidate_lock is exactly the same. Note that the locking in
__xfs_filemap_fault() slightly changes as filemap_fault() already takes
invalidate_lock.

Reviewed-by: Christoph Hellwig <hch@lst.de>
CC: <redacted>
CC: "Darrick J. Wong" <redacted>

It's djwong@kernel.org now.

OK, updated.

quoted

quoted

@@ -355,8 +358,11 @@ xfs_isilocked(
 
 	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
 		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
-			return !!ip->i_mmaplock.mr_writer;
-		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
+			return !debug_locks ||
+				lockdep_is_held_type(
+					&VFS_I(ip)->i_mapping->invalidate_lock,
+					0);
+		return rwsem_is_locked(&VFS_I(ip)->i_mapping->invalidate_lock);

This doesn't look right...

If lockdep is disabled, we always return true for
xfs_isilocked(ip, XFS_MMAPLOCK_EXCL) even if nobody holds the lock?

Granted, you probably just copy-pasted from the IOLOCK_SHARED clause
beneath it.  Er... oh right, preichl was messing with all that...

https://lore.kernel.org/linux-xfs/20201016021005.548850-2-preichl@redhat.com/ (local)

Indeed copy-paste programming ;) It certainly makes the assertions happy
but useless. Should I pull the patch you reference into the series? It
seems to have been uncontroversial and reviewed. Or will you pull the
series to xfs tree so I can just rebase on top?

The full conversion series introduced assertion failures because lockdep
can't handle some of the ILOCK usage patterns, specifically the fact
that a thread sometimes takes the ILOCK but then hands the inode to a
workqueue to avoid overflowing the first thread's stack.  That's why it
never got merged into the xfs tree.

I see. Yeah, we do "interesting" dances around lockdep fs-freezing
annotations for AIO as well where the freeze protection is inherited from
submission to completion context (we effectively generate false release
event for lockdep when exiting submit context and false acquire event in
the completion context). It can be done but it's ugly and error prone.

However, that kind of switcheroo isn't done with the
MMAPLOCK/invalidate_lock, so you could simply pull the patch I linked
above into your series.

OK, will do!

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR