On Fri, Jan 26, 2018 at 04:07:41AM -0800, Christoph Hellwig wrote:

quoted

+xfs_lock_two_inodes_separately(
+	struct xfs_inode	*ip0,
+	uint			ip0_mode,
+	struct xfs_inode	*ip1,
+	uint			ip1_mode)

We only have 6 calls to xfs_lock_two_inodes in total, so just update
the signature to take two modes and be done with it.

Also how about mode1 and mode2 for the argument names?

quoted

 	lp = (xfs_log_item_t *)ip0->i_itemp;
 	if (lp && (lp->li_flags & XFS_LI_IN_AIL)) {
-		if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(lock_mode, 1))) {
-			xfs_iunlock(ip0, lock_mode);
+		if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(ip1_mode, 1))) {
+			xfs_iunlock(ip0, ip0_mode);
 			if ((++attempts % 5) == 0)
 				delay(1); /* Don't just spin the CPU */
 			goto again;
 		}
 	} else {
-		xfs_ilock(ip1, xfs_lock_inumorder(lock_mode, 1));
+		xfs_ilock(ip1, xfs_lock_inumorder(ip1_mode, 1));
 	}
 }

Not directly related to your patch, but the the nowait + retry
mess must go away.

I think we need to move to the VFS locking conventions, that is
based on ancestors for directories (see lock_rename) and otherwise
based on the struct inode address as in lock_two_nondirectories.

I'm pretty sure this has nothing to do with directory lock order.
It's preventing deadlocks with xfs_iflush_cluster() where we lock
other inodes in the cluster and flush them to the backing buffer
while we still hold the ILOCK for the original inode we are pushing.

That's why this is all conditional on the XFS_LI_IN_AIL flag - the
inode writeback code won't be holding or attmpeting to lock the
inode if it is not in the AIL (i.e. it is clean).

Hence I don't think this trylock+backoff can ever go away here,
because there's no guaranteed lock ordering relationship between
directory structure and location in the inode cluster....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com