xfs: fix swapext ilock deadlock
xfs_swap_extents() holds the ilock over a call to filemap_write_and_wait(), which can then try to write data and take the ilock. That causes a self-deadlock. Fix the deadlock and clean up the code by separating the locking appropriately. Add a lockflags variable to track what locks we are holding as we gain and drop them and cleanup the error handling to always use "out_unlock" with the lockflags variable. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
This commit is contained in:
parent
b92cc59f69
commit
8121768321
|
@ -1686,6 +1686,7 @@ xfs_swap_extents(
|
|||
int aforkblks = 0;
|
||||
int taforkblks = 0;
|
||||
__uint64_t tmp;
|
||||
int lock_flags;
|
||||
|
||||
tempifp = kmem_alloc(sizeof(xfs_ifork_t), KM_MAYFAIL);
|
||||
if (!tempifp) {
|
||||
|
@ -1694,13 +1695,13 @@ xfs_swap_extents(
|
|||
}
|
||||
|
||||
/*
|
||||
* we have to do two separate lock calls here to keep lockdep
|
||||
* happy. If we try to get all the locks in one call, lock will
|
||||
* report false positives when we drop the ILOCK and regain them
|
||||
* below.
|
||||
* Lock up the inodes against other IO and truncate to begin with.
|
||||
* Then we can ensure the inodes are flushed and have no page cache
|
||||
* safely. Once we have done this we can take the ilocks and do the rest
|
||||
* of the checks.
|
||||
*/
|
||||
lock_flags = XFS_IOLOCK_EXCL;
|
||||
xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
|
||||
xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
|
||||
|
||||
/* Verify that both files have the same format */
|
||||
if ((ip->i_d.di_mode & S_IFMT) != (tip->i_d.di_mode & S_IFMT)) {
|
||||
|
@ -1719,6 +1720,9 @@ xfs_swap_extents(
|
|||
goto out_unlock;
|
||||
truncate_pagecache_range(VFS_I(tip), 0, -1);
|
||||
|
||||
xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
|
||||
lock_flags |= XFS_ILOCK_EXCL;
|
||||
|
||||
/* Verify O_DIRECT for ftmp */
|
||||
if (VFS_I(tip)->i_mapping->nrpages) {
|
||||
error = -EINVAL;
|
||||
|
@ -1773,6 +1777,7 @@ xfs_swap_extents(
|
|||
|
||||
xfs_iunlock(ip, XFS_ILOCK_EXCL);
|
||||
xfs_iunlock(tip, XFS_ILOCK_EXCL);
|
||||
lock_flags &= ~XFS_ILOCK_EXCL;
|
||||
|
||||
/*
|
||||
* There is a race condition here since we gave up the
|
||||
|
@ -1785,13 +1790,11 @@ xfs_swap_extents(
|
|||
|
||||
tp = xfs_trans_alloc(mp, XFS_TRANS_SWAPEXT);
|
||||
error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ichange, 0, 0);
|
||||
if (error) {
|
||||
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
|
||||
xfs_iunlock(tip, XFS_IOLOCK_EXCL);
|
||||
xfs_trans_cancel(tp, 0);
|
||||
goto out;
|
||||
}
|
||||
if (error)
|
||||
goto out_trans_cancel;
|
||||
|
||||
xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
|
||||
lock_flags |= XFS_ILOCK_EXCL;
|
||||
|
||||
/*
|
||||
* Count the number of extended attribute blocks
|
||||
|
@ -1810,8 +1813,8 @@ xfs_swap_extents(
|
|||
goto out_trans_cancel;
|
||||
}
|
||||
|
||||
xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
|
||||
xfs_trans_ijoin(tp, tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
|
||||
xfs_trans_ijoin(tp, ip, lock_flags);
|
||||
xfs_trans_ijoin(tp, tip, lock_flags);
|
||||
|
||||
/*
|
||||
* Before we've swapped the forks, lets set the owners of the forks
|
||||
|
@ -1940,8 +1943,8 @@ out:
|
|||
return error;
|
||||
|
||||
out_unlock:
|
||||
xfs_iunlock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
|
||||
xfs_iunlock(tip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
|
||||
xfs_iunlock(ip, lock_flags);
|
||||
xfs_iunlock(tip, lock_flags);
|
||||
goto out;
|
||||
|
||||
out_trans_cancel:
|
||||
|
|
Loading…
Reference in New Issue