diff mbox

[3/4] xfs: move inode locking from xfs_reflink_remap_range to xfs_file_share_range

Message ID 1476705920-32493-4-git-send-email-hch@lst.de (mailing list archive)
State Accepted
Headers show

Commit Message

Christoph Hellwig Oct. 17, 2016, 12:05 p.m. UTC 
We need the iolock protection to stabilizie the IS_SWAPFILE and IS_IMMUTABLE
values, as well as preventing new buffered writers re-dirtying the file data
that we just wrote out.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c    | 62 ++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_reflink.c | 15 -------------
 2 files changed, 41 insertions(+), 36 deletions(-)

Comments

Darrick J. Wong Oct. 17, 2016, 9:16 p.m. UTC | #1 
On Mon, Oct 17, 2016 at 02:05:19PM +0200, Christoph Hellwig wrote:
> We need the iolock protection to stabilizie the IS_SWAPFILE and IS_IMMUTABLE
> values, as well as preventing new buffered writers re-dirtying the file data
> that we just wrote out.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/xfs_file.c    | 62 ++++++++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_reflink.c | 15 -------------
>  2 files changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 012a960..0960264 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -996,38 +996,54 @@ xfs_file_share_range(
>  	inode_out = file_inode(file_out);
>  	bs = inode_out->i_sb->s_blocksize;
>  
> +	/* Lock both files against IO */
> +	same_inode = (inode_in == inode_out);
> +	if (same_inode) {
> +		xfs_ilock(XFS_I(inode_in), XFS_IOLOCK_EXCL);
> +		xfs_ilock(XFS_I(inode_in), XFS_MMAPLOCK_EXCL);
> +	} else {
> +		xfs_lock_two_inodes(XFS_I(inode_in), XFS_I(inode_out),
> +				XFS_IOLOCK_EXCL);
> +		xfs_lock_two_inodes(XFS_I(inode_in), XFS_I(inode_out),
> +				XFS_MMAPLOCK_EXCL);
> +	}
> +
>  	/* Don't touch certain kinds of inodes */
> +	ret = -EPERM;
>  	if (IS_IMMUTABLE(inode_out))
> -		return -EPERM;
> -	if (IS_SWAPFILE(inode_in) ||
> -	    IS_SWAPFILE(inode_out))
> -		return -ETXTBSY;
> -
> -	same_inode = (inode_in == inode_out);
> +		goto out_unlock;
> +	ret = -ETXTBSY;
> +	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
> +		goto out_unlock;
>  
>  	/* Don't reflink dirs, pipes, sockets... */
> +	ret = -EISDIR;
>  	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
> -		return -EISDIR;
> +		goto out_unlock;
> +	ret = -EINVAL;
>  	if (S_ISFIFO(inode_in->i_mode) || S_ISFIFO(inode_out->i_mode))
> -		return -EINVAL;
> +		goto out_unlock;
>  	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
> -		return -EINVAL;
> +		goto out_unlock;
>  
>  	/* Don't share DAX file data for now. */
>  	if (IS_DAX(inode_in) || IS_DAX(inode_out))
> -		return -EINVAL;
> +		goto out_unlock;
>  
>  	/* Are we going all the way to the end? */
>  	isize = i_size_read(inode_in);
> -	if (isize == 0)
> -		return 0;
> +	if (isize == 0) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> +
>  	if (len == 0)
>  		len = isize - pos_in;
>  
>  	/* Ensure offsets don't wrap and the input is inside i_size */
>  	if (pos_in + len < pos_in || pos_out + len < pos_out ||
>  	    pos_in + len > isize)
> -		return -EINVAL;
> +		goto out_unlock;
>  
>  	/* Don't allow dedupe past EOF in the dest file */
>  	if (is_dedupe) {
> @@ -1035,7 +1051,7 @@ xfs_file_share_range(
>  
>  		disize = i_size_read(inode_out);
>  		if (pos_out >= disize || pos_out + len > disize)
> -			return -EINVAL;
> +			goto out_unlock;
>  	}
>  
>  	/* If we're linking to EOF, continue to the block boundary. */
> @@ -1047,28 +1063,32 @@ xfs_file_share_range(
>  	/* Only reflink if we're aligned to block boundaries */
>  	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_in + blen, bs) ||
>  	    !IS_ALIGNED(pos_out, bs) || !IS_ALIGNED(pos_out + blen, bs))
> -		return -EINVAL;
> +		goto out_unlock;
>  
>  	/* Don't allow overlapped reflink within the same file */
>  	if (same_inode && pos_out + blen > pos_in && pos_out < pos_in + blen)
> -		return -EINVAL;
> +		goto out_unlock;
>  
>  	/* Wait for the completion of any pending IOs on srcfile */
>  	ret = xfs_file_wait_for_io(inode_in, pos_in, len);
>  	if (ret)
> -		goto out;
> +		goto out_unlock;
>  	ret = xfs_file_wait_for_io(inode_out, pos_out, len);
>  	if (ret)
> -		goto out;
> +		goto out_unlock;
>  
>  	if (is_dedupe)
>  		flags |= XFS_REFLINK_DEDUPE;
>  	ret = xfs_reflink_remap_range(XFS_I(inode_in), pos_in, XFS_I(inode_out),
>  			pos_out, len, flags);
> -	if (ret < 0)
> -		goto out;
>  
> -out:
> +out_unlock:
> +	xfs_iunlock(XFS_I(inode_in), XFS_MMAPLOCK_EXCL);
> +	xfs_iunlock(XFS_I(inode_in), XFS_IOLOCK_EXCL);
> +	if (!same_inode) {
> +		xfs_iunlock(XFS_I(inode_out), XFS_MMAPLOCK_EXCL);
> +		xfs_iunlock(XFS_I(inode_out), XFS_IOLOCK_EXCL);
> +	}
>  	return ret;
>  }
>  
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 5965e94..d012746 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1341,15 +1341,6 @@ xfs_reflink_remap_range(
>  
>  	trace_xfs_reflink_remap_range(src, srcoff, len, dest, destoff);
>  
> -	/* Lock both files against IO */
> -	if (src->i_ino == dest->i_ino) {
> -		xfs_ilock(src, XFS_IOLOCK_EXCL);
> -		xfs_ilock(src, XFS_MMAPLOCK_EXCL);
> -	} else {
> -		xfs_lock_two_inodes(src, dest, XFS_IOLOCK_EXCL);
> -		xfs_lock_two_inodes(src, dest, XFS_MMAPLOCK_EXCL);
> -	}
> -
>  	/*
>  	 * Check that the extents are the same.
>  	 */
> @@ -1401,12 +1392,6 @@ xfs_reflink_remap_range(
>  		goto out_error;
>  
>  out_error:
> -	xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> -	xfs_iunlock(src, XFS_IOLOCK_EXCL);
> -	if (src->i_ino != dest->i_ino) {
> -		xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> -		xfs_iunlock(dest, XFS_IOLOCK_EXCL);
> -	}
>  	if (error)
>  		trace_xfs_reflink_remap_range_error(dest, error, _RET_IP_);
>  	return error;
> -- 
> 2.1.4
> 
--
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
diff mbox

Patch

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 012a960..0960264 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -996,38 +996,54 @@  xfs_file_share_range(
 	inode_out = file_inode(file_out);
 	bs = inode_out->i_sb->s_blocksize;
 
+	/* Lock both files against IO */
+	same_inode = (inode_in == inode_out);
+	if (same_inode) {
+		xfs_ilock(XFS_I(inode_in), XFS_IOLOCK_EXCL);
+		xfs_ilock(XFS_I(inode_in), XFS_MMAPLOCK_EXCL);
+	} else {
+		xfs_lock_two_inodes(XFS_I(inode_in), XFS_I(inode_out),
+				XFS_IOLOCK_EXCL);
+		xfs_lock_two_inodes(XFS_I(inode_in), XFS_I(inode_out),
+				XFS_MMAPLOCK_EXCL);
+	}
+
 	/* Don't touch certain kinds of inodes */
+	ret = -EPERM;
 	if (IS_IMMUTABLE(inode_out))
-		return -EPERM;
-	if (IS_SWAPFILE(inode_in) ||
-	    IS_SWAPFILE(inode_out))
-		return -ETXTBSY;
-
-	same_inode = (inode_in == inode_out);
+		goto out_unlock;
+	ret = -ETXTBSY;
+	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
+		goto out_unlock;
 
 	/* Don't reflink dirs, pipes, sockets... */
+	ret = -EISDIR;
 	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
-		return -EISDIR;
+		goto out_unlock;
+	ret = -EINVAL;
 	if (S_ISFIFO(inode_in->i_mode) || S_ISFIFO(inode_out->i_mode))
-		return -EINVAL;
+		goto out_unlock;
 	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
-		return -EINVAL;
+		goto out_unlock;
 
 	/* Don't share DAX file data for now. */
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
-		return -EINVAL;
+		goto out_unlock;
 
 	/* Are we going all the way to the end? */
 	isize = i_size_read(inode_in);
-	if (isize == 0)
-		return 0;
+	if (isize == 0) {
+		ret = 0;
+		goto out_unlock;
+	}
+
 	if (len == 0)
 		len = isize - pos_in;
 
 	/* Ensure offsets don't wrap and the input is inside i_size */
 	if (pos_in + len < pos_in || pos_out + len < pos_out ||
 	    pos_in + len > isize)
-		return -EINVAL;
+		goto out_unlock;
 
 	/* Don't allow dedupe past EOF in the dest file */
 	if (is_dedupe) {
@@ -1035,7 +1051,7 @@  xfs_file_share_range(
 
 		disize = i_size_read(inode_out);
 		if (pos_out >= disize || pos_out + len > disize)
-			return -EINVAL;
+			goto out_unlock;
 	}
 
 	/* If we're linking to EOF, continue to the block boundary. */
@@ -1047,28 +1063,32 @@  xfs_file_share_range(
 	/* Only reflink if we're aligned to block boundaries */
 	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_in + blen, bs) ||
 	    !IS_ALIGNED(pos_out, bs) || !IS_ALIGNED(pos_out + blen, bs))
-		return -EINVAL;
+		goto out_unlock;
 
 	/* Don't allow overlapped reflink within the same file */
 	if (same_inode && pos_out + blen > pos_in && pos_out < pos_in + blen)
-		return -EINVAL;
+		goto out_unlock;
 
 	/* Wait for the completion of any pending IOs on srcfile */
 	ret = xfs_file_wait_for_io(inode_in, pos_in, len);
 	if (ret)
-		goto out;
+		goto out_unlock;
 	ret = xfs_file_wait_for_io(inode_out, pos_out, len);
 	if (ret)
-		goto out;
+		goto out_unlock;
 
 	if (is_dedupe)
 		flags |= XFS_REFLINK_DEDUPE;
 	ret = xfs_reflink_remap_range(XFS_I(inode_in), pos_in, XFS_I(inode_out),
 			pos_out, len, flags);
-	if (ret < 0)
-		goto out;
 
-out:
+out_unlock:
+	xfs_iunlock(XFS_I(inode_in), XFS_MMAPLOCK_EXCL);
+	xfs_iunlock(XFS_I(inode_in), XFS_IOLOCK_EXCL);
+	if (!same_inode) {
+		xfs_iunlock(XFS_I(inode_out), XFS_MMAPLOCK_EXCL);
+		xfs_iunlock(XFS_I(inode_out), XFS_IOLOCK_EXCL);
+	}
 	return ret;
 }
 
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5965e94..d012746 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1341,15 +1341,6 @@  xfs_reflink_remap_range(
 
 	trace_xfs_reflink_remap_range(src, srcoff, len, dest, destoff);
 
-	/* Lock both files against IO */
-	if (src->i_ino == dest->i_ino) {
-		xfs_ilock(src, XFS_IOLOCK_EXCL);
-		xfs_ilock(src, XFS_MMAPLOCK_EXCL);
-	} else {
-		xfs_lock_two_inodes(src, dest, XFS_IOLOCK_EXCL);
-		xfs_lock_two_inodes(src, dest, XFS_MMAPLOCK_EXCL);
-	}
-
 	/*
 	 * Check that the extents are the same.
 	 */
@@ -1401,12 +1392,6 @@  xfs_reflink_remap_range(
 		goto out_error;
 
 out_error:
-	xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
-	xfs_iunlock(src, XFS_IOLOCK_EXCL);
-	if (src->i_ino != dest->i_ino) {
-		xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
-		xfs_iunlock(dest, XFS_IOLOCK_EXCL);
-	}
 	if (error)
 		trace_xfs_reflink_remap_range_error(dest, error, _RET_IP_);
 	return error;