On Wed, Jan 24, 2018 at 09:18:21AM -0500, Brian Foster wrote:
> 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:
TBH, "_mode" was my close second choice, so I'll just change the name
before I commit it for reals. Thanks for the review!
--D
>
> 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
--
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
