On Tue, Jan 23, 2018 at 06:18:09PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Reflink and dedupe operations remap blocks from a source file into a
> destination file. The destination file needs exclusive locks on all
> levels because we're updating its block map, but the source file isn't
> undergoing any block map changes so we can use a shared lock.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_inode.c | 49 ++++++++++++++++++++++++++++++++-----------------
> fs/xfs/xfs_inode.h | 12 +++++++++++-
> fs/xfs/xfs_reflink.c | 26 ++++++++++++++++----------
> 3 files changed, 59 insertions(+), 28 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c9e40d4..4a38cfc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -545,24 +545,37 @@ xfs_lock_inodes(
> }
>
> /*
> - * xfs_lock_two_inodes() can only be used to lock one type of lock at a time -
> - * the iolock, the mmaplock or the ilock, but not more than one at a time. If we
> - * lock more than one at a time, lockdep will report false positives saying we
> - * have violated locking orders.
> + * xfs_lock_two_inodes_separately() can only be used to lock one type of lock
> + * at a time - the mmaplock or the ilock, but not more than one type at a
> + * time. If we lock more than one at a time, lockdep will report false
> + * positives saying we have violated locking orders. The iolock must be
> + * double-locked separately since we use i_rwsem for that. We now support
> + * taking one lock EXCL and the other SHARED.
> */
> void
> -xfs_lock_two_inodes(
> - xfs_inode_t *ip0,
> - xfs_inode_t *ip1,
> - uint lock_mode)
> +xfs_lock_two_inodes_separately(
> + struct xfs_inode *ip0,
> + uint ip0_mode,
> + struct xfs_inode *ip1,
> + uint ip1_mode)
> {
Nit.. but "separately" doesn't really convey meaning to me. I guess
something like xfs_lock_two_inodes_mode() is more clear to me, even
though the original version still accepted a mode. Eh, I guess even
__xfs_lock_two_inodes() might be fine (and perhaps more common
practice). Code looks fine either way:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> - xfs_inode_t *temp;
> + struct xfs_inode *temp;
> + uint mode_temp;
> int attempts = 0;
> xfs_log_item_t *lp;
>
> - ASSERT(!(lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)));
> - if (lock_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL))
> - ASSERT(!(lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
> + ASSERT(hweight32(ip0_mode) == 1);
> + ASSERT(hweight32(ip1_mode) == 1);
> + ASSERT(!(ip0_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)));
> + ASSERT(!(ip1_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)));
> + ASSERT(!(ip0_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
> + !(ip0_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
> + ASSERT(!(ip1_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
> + !(ip1_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
> + ASSERT(!(ip1_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
> + !(ip0_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
> + ASSERT(!(ip0_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
> + !(ip1_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
>
> ASSERT(ip0->i_ino != ip1->i_ino);
>
> @@ -570,10 +583,13 @@ xfs_lock_two_inodes(
> temp = ip0;
> ip0 = ip1;
> ip1 = temp;
> + mode_temp = ip0_mode;
> + ip0_mode = ip1_mode;
> + ip1_mode = mode_temp;
> }
>
> again:
> - xfs_ilock(ip0, xfs_lock_inumorder(lock_mode, 0));
> + xfs_ilock(ip0, xfs_lock_inumorder(ip0_mode, 0));
>
> /*
> * If the first lock we have locked is in the AIL, we must TRY to get
> @@ -582,18 +598,17 @@ xfs_lock_two_inodes(
> */
> 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));
> }
> }
>
> -
> void
> __xfs_iflock(
> struct xfs_inode *ip)
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 386b0bb..ff56486 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -423,7 +423,17 @@ void xfs_iunpin_wait(xfs_inode_t *);
> #define xfs_ipincount(ip) ((unsigned int) atomic_read(&ip->i_pincount))
>
> int xfs_iflush(struct xfs_inode *, struct xfs_buf **);
> -void xfs_lock_two_inodes(xfs_inode_t *, xfs_inode_t *, uint);
> +void xfs_lock_two_inodes_separately(struct xfs_inode *ip0,
> + uint ip0_mode, struct xfs_inode *ip1,
> + uint ip1_mode);
> +static inline void
> +xfs_lock_two_inodes(
> + struct xfs_inode *ip0,
> + struct xfs_inode *ip1,
> + uint lock_mode)
> +{
> + xfs_lock_two_inodes_separately(ip0, lock_mode, ip1, lock_mode);
> +}
>
> xfs_extlen_t xfs_get_extsz_hint(struct xfs_inode *ip);
> xfs_extlen_t xfs_get_cowextsz_hint(struct xfs_inode *ip);
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index f89a725..f5a43b2 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1202,13 +1202,16 @@ xfs_reflink_remap_blocks(
>
> /* drange = (destoff, destoff + len); srange = (srcoff, srcoff + len) */
> while (len) {
> + uint lock_mode;
> +
> trace_xfs_reflink_remap_blocks_loop(src, srcoff, len,
> dest, destoff);
> +
> /* Read extent from the source file */
> nimaps = 1;
> - xfs_ilock(src, XFS_ILOCK_EXCL);
> + lock_mode = xfs_ilock_data_map_shared(src);
> error = xfs_bmapi_read(src, srcoff, len, &imap, &nimaps, 0);
> - xfs_iunlock(src, XFS_ILOCK_EXCL);
> + xfs_iunlock(src, lock_mode);
> if (error)
> goto err;
> ASSERT(nimaps == 1);
> @@ -1262,7 +1265,7 @@ xfs_iolock_two_inodes_and_break_layout(
>
> retry:
> if (src_first) {
> - inode_lock(src);
> + inode_lock_shared(src);
> inode_lock_nested(dest, I_MUTEX_NONDIR2);
> } else {
> inode_lock(dest);
> @@ -1272,7 +1275,7 @@ xfs_iolock_two_inodes_and_break_layout(
> if (error == -EWOULDBLOCK) {
> inode_unlock(dest);
> if (src_first)
> - inode_unlock(src);
> + inode_unlock_shared(src);
> error = break_layout(dest, true);
> if (error)
> return error;
> @@ -1280,11 +1283,11 @@ xfs_iolock_two_inodes_and_break_layout(
> } else if (error) {
> inode_unlock(dest);
> if (src_first)
> - inode_unlock(src);
> + inode_unlock_shared(src);
> return error;
> }
> if (src_last)
> - inode_lock_nested(src, I_MUTEX_NONDIR2);
> + down_read_nested(&src->i_rwsem, I_MUTEX_NONDIR2);
> return 0;
> }
>
> @@ -1324,7 +1327,8 @@ xfs_reflink_remap_range(
> if (same_inode)
> xfs_ilock(src, XFS_MMAPLOCK_EXCL);
> else
> - xfs_lock_two_inodes(src, dest, XFS_MMAPLOCK_EXCL);
> + xfs_lock_two_inodes_separately(src, XFS_MMAPLOCK_SHARED,
> + dest, XFS_MMAPLOCK_EXCL);
>
> /* Check file eligibility and prepare for block sharing. */
> ret = -EINVAL;
> @@ -1387,10 +1391,12 @@ xfs_reflink_remap_range(
> is_dedupe);
>
> out_unlock:
> - xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> + xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> + if (!same_inode)
> + xfs_iunlock(src, XFS_MMAPLOCK_SHARED);
> + inode_unlock(inode_out);
> if (!same_inode)
> - xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> - unlock_two_nondirectories(inode_in, inode_out);
> + inode_unlock_shared(inode_in);
> if (ret)
> trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
> return ret;
>
> --
> 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
--
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
