On Thu, Feb 05, 2015 at 08:04:19AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Extent swap operations are another extent manipulation operation
> that we need to ensure does not race against mmap page faults. The
> current code returns if the file is mapped prior to the swap being
> done, but it could potentially race against new page faults while
> the swap is in progress. Hence we should use the XFS_MMAPLOCK_EXCL
> for this operation, too.
>
> While there, fix the error path handling that can result in double
> unlocks of the inodes when cancelling the swapext transaction.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> fs/xfs/xfs_bmap_util.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 22a5dcb..7efa23e 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1599,13 +1599,6 @@ xfs_swap_extent_flush(
> /* Verify O_DIRECT for ftmp */
> if (VFS_I(ip)->i_mapping->nrpages)
> return -EINVAL;
> -
> - /*
> - * Don't try to swap extents on mmap()d files because we can't lock
> - * out races against page faults safely.
> - */
> - if (mapping_mapped(VFS_I(ip)->i_mapping))
> - return -EBUSY;
> return 0;
> }
>
> @@ -1633,13 +1626,14 @@ xfs_swap_extents(
> }
>
> /*
> - * 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 the inodes against other IO, page faults 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;
> + lock_flags = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
> xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL);
> + xfs_lock_two_inodes(ip, tip, XFS_MMAPLOCK_EXCL);
>
> /* Verify that both files have the same format */
> if ((ip->i_d.di_mode & S_IFMT) != (tip->i_d.di_mode & S_IFMT)) {
> @@ -1666,8 +1660,16 @@ xfs_swap_extents(
> xfs_trans_cancel(tp, 0);
> goto out_unlock;
> }
> +
> + /*
> + * Lock and join the inodes to the tansaction so that transaction commit
> + * or cancel will unlock the inodes from this point onwards.
> + */
> xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL);
> lock_flags |= XFS_ILOCK_EXCL;
> + xfs_trans_ijoin(tp, ip, lock_flags);
> + xfs_trans_ijoin(tp, tip, lock_flags);
> +
>
> /* Verify all data are being swapped */
> if (sxp->sx_offset != 0 ||
> @@ -1720,9 +1722,6 @@ xfs_swap_extents(
> goto out_trans_cancel;
> }
>
> - 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
> * appropriately. We have to do this as we are demand paging the btree
> @@ -1856,5 +1855,5 @@ out_unlock:
>
> out_trans_cancel:
> xfs_trans_cancel(tp, 0);
> - goto out_unlock;
> + goto out;
> }
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
