On Fri, Jan 26, 2018 at 04:07:41AM -0800, Christoph Hellwig wrote:
> > +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?
>
> > 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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
