Re: [PATCH 02/11] xfs: only grab shared inode locks for source file during reflink

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> >  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@redhat.com>
> 
> > -	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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html