diff mbox series

[3/3] xfs: refactor locking and unlocking two inodes against userspace IO

Message ID 159288490975.150128.5869655548489048713.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: reflink cleanups | expand

Commit Message

Darrick J. Wong June 23, 2020, 4:01 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the two functions that we use to lock and unlock two inodes to
block userspace from initiating IO against a file, whether via system
calls or mmap activity.  Move them to xfs_inode.c since this functionality
isn't specific to reflink anyway.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c    |    2 +
 fs/xfs/xfs_inode.c   |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_inode.h   |    3 ++
 fs/xfs/xfs_reflink.c |   85 +---------------------------------------------
 fs/xfs/xfs_reflink.h |    2 -
 5 files changed, 99 insertions(+), 86 deletions(-)

Comments

Brian Foster June 24, 2020, 12:39 p.m. UTC | #1 
On Mon, Jun 22, 2020 at 09:01:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the two functions that we use to lock and unlock two inodes to
> block userspace from initiating IO against a file, whether via system
> calls or mmap activity.  Move them to xfs_inode.c since this functionality
> isn't specific to reflink anyway.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I'd prefer to see refactoring patches separate from moving patches, but
looks Ok:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_file.c    |    2 +
>  fs/xfs/xfs_inode.c   |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h   |    3 ++
>  fs/xfs/xfs_reflink.c |   85 +---------------------------------------------
>  fs/xfs/xfs_reflink.h |    2 -
>  5 files changed, 99 insertions(+), 86 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index b375fae811f2..a32d1eeee0f7 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1065,7 +1065,7 @@ xfs_file_remap_range(
>  	if (mp->m_flags & XFS_MOUNT_WSYNC)
>  		xfs_log_force_inode(dest);
>  out_unlock:
> -	xfs_reflink_remap_unlock(file_in, file_out);
> +	xfs_iunlock_io_mmap(src, dest);
>  	if (ret)
>  		trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
>  	return remapped > 0 ? remapped : ret;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 9aea7d68d8ab..b9c6d1cc64a9 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3881,3 +3881,96 @@ xfs_log_force_inode(
>  		return 0;
>  	return xfs_log_force_lsn(ip->i_mount, lsn, XFS_LOG_SYNC, NULL);
>  }
> +
> +/*
> + * Grab the exclusive iolock for a data copy from src to dest, making sure to
> + * abide vfs locking order (lowest pointer value goes first) and breaking the
> + * layout leases before proceeding.  The loop is needed because we cannot call
> + * the blocking break_layout() with the iolocks held, and therefore have to
> + * back out both locks.
> + */
> +static int
> +xfs_iolock_two_inodes_and_break_layout(
> +	struct inode		*src,
> +	struct inode		*dest)
> +{
> +	int			error;
> +
> +	if (src > dest)
> +		swap(src, dest);
> +
> +retry:
> +	/* Wait to break both inodes' layouts before we start locking. */
> +	error = break_layout(src, true);
> +	if (error)
> +		return error;
> +	if (src != dest) {
> +		error = break_layout(dest, true);
> +		if (error)
> +			return error;
> +	}
> +
> +	/* Lock one inode and make sure nobody got in and leased it. */
> +	inode_lock(src);
> +	error = break_layout(src, false);
> +	if (error) {
> +		inode_unlock(src);
> +		if (error == -EWOULDBLOCK)
> +			goto retry;
> +		return error;
> +	}
> +
> +	if (src == dest)
> +		return 0;
> +
> +	/* Lock the other inode and make sure nobody got in and leased it. */
> +	inode_lock_nested(dest, I_MUTEX_NONDIR2);
> +	error = break_layout(dest, false);
> +	if (error) {
> +		inode_unlock(src);
> +		inode_unlock(dest);
> +		if (error == -EWOULDBLOCK)
> +			goto retry;
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Lock two files so that userspace cannot initiate I/O via file syscalls or
> + * mmap activity.
> + */
> +int
> +xfs_ilock_io_mmap(
> +	struct xfs_inode	*ip1,
> +	struct xfs_inode	*ip2)
> +{
> +	int			ret;
> +
> +	ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), VFS_I(ip2));
> +	if (ret)
> +		return ret;
> +	if (ip1 == ip2)
> +		xfs_ilock(ip1, XFS_MMAPLOCK_EXCL);
> +	else
> +		xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL,
> +				    ip2, XFS_MMAPLOCK_EXCL);
> +	return 0;
> +}
> +
> +/* Unlock both files to allow IO and mmap activity. */
> +void
> +xfs_iunlock_io_mmap(
> +	struct xfs_inode	*ip1,
> +	struct xfs_inode	*ip2)
> +{
> +	bool			same_inode = (ip1 == ip2);
> +
> +	xfs_iunlock(ip2, XFS_MMAPLOCK_EXCL);
> +	if (!same_inode)
> +		xfs_iunlock(ip1, XFS_MMAPLOCK_EXCL);
> +	inode_unlock(VFS_I(ip2));
> +	if (!same_inode)
> +		inode_unlock(VFS_I(ip1));
> +}
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 47d3b391030d..001529784f96 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -499,4 +499,7 @@ void xfs_iunlink_destroy(struct xfs_perag *pag);
>  
>  void xfs_end_io(struct work_struct *work);
>  
> +int xfs_ilock_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
> +void xfs_iunlock_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
> +
>  #endif	/* __XFS_INODE_H__ */
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index dd9ed7d5694d..130b6be8180e 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1203,81 +1203,6 @@ xfs_reflink_remap_blocks(
>  	return error;
>  }
>  
> -/*
> - * Grab the exclusive iolock for a data copy from src to dest, making sure to
> - * abide vfs locking order (lowest pointer value goes first) and breaking the
> - * layout leases before proceeding.  The loop is needed because we cannot call
> - * the blocking break_layout() with the iolocks held, and therefore have to
> - * back out both locks.
> - */
> -static int
> -xfs_iolock_two_inodes_and_break_layout(
> -	struct inode		*src,
> -	struct inode		*dest)
> -{
> -	int			error;
> -
> -	if (src > dest)
> -		swap(src, dest);
> -
> -retry:
> -	/* Wait to break both inodes' layouts before we start locking. */
> -	error = break_layout(src, true);
> -	if (error)
> -		return error;
> -	if (src != dest) {
> -		error = break_layout(dest, true);
> -		if (error)
> -			return error;
> -	}
> -
> -	/* Lock one inode and make sure nobody got in and leased it. */
> -	inode_lock(src);
> -	error = break_layout(src, false);
> -	if (error) {
> -		inode_unlock(src);
> -		if (error == -EWOULDBLOCK)
> -			goto retry;
> -		return error;
> -	}
> -
> -	if (src == dest)
> -		return 0;
> -
> -	/* Lock the other inode and make sure nobody got in and leased it. */
> -	inode_lock_nested(dest, I_MUTEX_NONDIR2);
> -	error = break_layout(dest, false);
> -	if (error) {
> -		inode_unlock(src);
> -		inode_unlock(dest);
> -		if (error == -EWOULDBLOCK)
> -			goto retry;
> -		return error;
> -	}
> -
> -	return 0;
> -}
> -
> -/* Unlock both inodes after they've been prepped for a range clone. */
> -void
> -xfs_reflink_remap_unlock(
> -	struct file		*file_in,
> -	struct file		*file_out)
> -{
> -	struct inode		*inode_in = file_inode(file_in);
> -	struct xfs_inode	*src = XFS_I(inode_in);
> -	struct inode		*inode_out = file_inode(file_out);
> -	struct xfs_inode	*dest = XFS_I(inode_out);
> -	bool			same_inode = (inode_in == inode_out);
> -
> -	xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
> -	if (!same_inode)
> -		xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> -	inode_unlock(inode_out);
> -	if (!same_inode)
> -		inode_unlock(inode_in);
> -}
> -
>  /*
>   * If we're reflinking to a point past the destination file's EOF, we must
>   * zero any speculative post-EOF preallocations that sit between the old EOF
> @@ -1340,18 +1265,12 @@ xfs_reflink_remap_prep(
>  	struct xfs_inode	*src = XFS_I(inode_in);
>  	struct inode		*inode_out = file_inode(file_out);
>  	struct xfs_inode	*dest = XFS_I(inode_out);
> -	bool			same_inode = (inode_in == inode_out);
>  	int			ret;
>  
>  	/* Lock both files against IO */
> -	ret = xfs_iolock_two_inodes_and_break_layout(inode_in, inode_out);
> +	ret = xfs_ilock_io_mmap(src, dest);
>  	if (ret)
>  		return ret;
> -	if (same_inode)
> -		xfs_ilock(src, XFS_MMAPLOCK_EXCL);
> -	else
> -		xfs_lock_two_inodes(src, XFS_MMAPLOCK_EXCL, dest,
> -				XFS_MMAPLOCK_EXCL);
>  
>  	/* Check file eligibility and prepare for block sharing. */
>  	ret = -EINVAL;
> @@ -1402,7 +1321,7 @@ xfs_reflink_remap_prep(
>  
>  	return 0;
>  out_unlock:
> -	xfs_reflink_remap_unlock(file_in, file_out);
> +	xfs_iunlock_io_mmap(src, dest);
>  	return ret;
>  }
>  
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 3e4fd46373ab..487b00434b96 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -56,7 +56,5 @@ extern int xfs_reflink_remap_blocks(struct xfs_inode *src, loff_t pos_in,
>  		loff_t *remapped);
>  extern int xfs_reflink_update_dest(struct xfs_inode *dest, xfs_off_t newlen,
>  		xfs_extlen_t cowextsize, unsigned int remap_flags);
> -extern void xfs_reflink_remap_unlock(struct file *file_in,
> -		struct file *file_out);
>  
>  #endif /* __XFS_REFLINK_H */
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b375fae811f2..a32d1eeee0f7 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1065,7 +1065,7 @@  xfs_file_remap_range(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_log_force_inode(dest);
 out_unlock:
-	xfs_reflink_remap_unlock(file_in, file_out);
+	xfs_iunlock_io_mmap(src, dest);
 	if (ret)
 		trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
 	return remapped > 0 ? remapped : ret;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 9aea7d68d8ab..b9c6d1cc64a9 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3881,3 +3881,96 @@  xfs_log_force_inode(
 		return 0;
 	return xfs_log_force_lsn(ip->i_mount, lsn, XFS_LOG_SYNC, NULL);
 }
+
+/*
+ * Grab the exclusive iolock for a data copy from src to dest, making sure to
+ * abide vfs locking order (lowest pointer value goes first) and breaking the
+ * layout leases before proceeding.  The loop is needed because we cannot call
+ * the blocking break_layout() with the iolocks held, and therefore have to
+ * back out both locks.
+ */
+static int
+xfs_iolock_two_inodes_and_break_layout(
+	struct inode		*src,
+	struct inode		*dest)
+{
+	int			error;
+
+	if (src > dest)
+		swap(src, dest);
+
+retry:
+	/* Wait to break both inodes' layouts before we start locking. */
+	error = break_layout(src, true);
+	if (error)
+		return error;
+	if (src != dest) {
+		error = break_layout(dest, true);
+		if (error)
+			return error;
+	}
+
+	/* Lock one inode and make sure nobody got in and leased it. */
+	inode_lock(src);
+	error = break_layout(src, false);
+	if (error) {
+		inode_unlock(src);
+		if (error == -EWOULDBLOCK)
+			goto retry;
+		return error;
+	}
+
+	if (src == dest)
+		return 0;
+
+	/* Lock the other inode and make sure nobody got in and leased it. */
+	inode_lock_nested(dest, I_MUTEX_NONDIR2);
+	error = break_layout(dest, false);
+	if (error) {
+		inode_unlock(src);
+		inode_unlock(dest);
+		if (error == -EWOULDBLOCK)
+			goto retry;
+		return error;
+	}
+
+	return 0;
+}
+
+/*
+ * Lock two files so that userspace cannot initiate I/O via file syscalls or
+ * mmap activity.
+ */
+int
+xfs_ilock_io_mmap(
+	struct xfs_inode	*ip1,
+	struct xfs_inode	*ip2)
+{
+	int			ret;
+
+	ret = xfs_iolock_two_inodes_and_break_layout(VFS_I(ip1), VFS_I(ip2));
+	if (ret)
+		return ret;
+	if (ip1 == ip2)
+		xfs_ilock(ip1, XFS_MMAPLOCK_EXCL);
+	else
+		xfs_lock_two_inodes(ip1, XFS_MMAPLOCK_EXCL,
+				    ip2, XFS_MMAPLOCK_EXCL);
+	return 0;
+}
+
+/* Unlock both files to allow IO and mmap activity. */
+void
+xfs_iunlock_io_mmap(
+	struct xfs_inode	*ip1,
+	struct xfs_inode	*ip2)
+{
+	bool			same_inode = (ip1 == ip2);
+
+	xfs_iunlock(ip2, XFS_MMAPLOCK_EXCL);
+	if (!same_inode)
+		xfs_iunlock(ip1, XFS_MMAPLOCK_EXCL);
+	inode_unlock(VFS_I(ip2));
+	if (!same_inode)
+		inode_unlock(VFS_I(ip1));
+}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 47d3b391030d..001529784f96 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -499,4 +499,7 @@  void xfs_iunlink_destroy(struct xfs_perag *pag);
 
 void xfs_end_io(struct work_struct *work);
 
+int xfs_ilock_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
+void xfs_iunlock_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
+
 #endif	/* __XFS_INODE_H__ */
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index dd9ed7d5694d..130b6be8180e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1203,81 +1203,6 @@  xfs_reflink_remap_blocks(
 	return error;
 }
 
-/*
- * Grab the exclusive iolock for a data copy from src to dest, making sure to
- * abide vfs locking order (lowest pointer value goes first) and breaking the
- * layout leases before proceeding.  The loop is needed because we cannot call
- * the blocking break_layout() with the iolocks held, and therefore have to
- * back out both locks.
- */
-static int
-xfs_iolock_two_inodes_and_break_layout(
-	struct inode		*src,
-	struct inode		*dest)
-{
-	int			error;
-
-	if (src > dest)
-		swap(src, dest);
-
-retry:
-	/* Wait to break both inodes' layouts before we start locking. */
-	error = break_layout(src, true);
-	if (error)
-		return error;
-	if (src != dest) {
-		error = break_layout(dest, true);
-		if (error)
-			return error;
-	}
-
-	/* Lock one inode and make sure nobody got in and leased it. */
-	inode_lock(src);
-	error = break_layout(src, false);
-	if (error) {
-		inode_unlock(src);
-		if (error == -EWOULDBLOCK)
-			goto retry;
-		return error;
-	}
-
-	if (src == dest)
-		return 0;
-
-	/* Lock the other inode and make sure nobody got in and leased it. */
-	inode_lock_nested(dest, I_MUTEX_NONDIR2);
-	error = break_layout(dest, false);
-	if (error) {
-		inode_unlock(src);
-		inode_unlock(dest);
-		if (error == -EWOULDBLOCK)
-			goto retry;
-		return error;
-	}
-
-	return 0;
-}
-
-/* Unlock both inodes after they've been prepped for a range clone. */
-void
-xfs_reflink_remap_unlock(
-	struct file		*file_in,
-	struct file		*file_out)
-{
-	struct inode		*inode_in = file_inode(file_in);
-	struct xfs_inode	*src = XFS_I(inode_in);
-	struct inode		*inode_out = file_inode(file_out);
-	struct xfs_inode	*dest = XFS_I(inode_out);
-	bool			same_inode = (inode_in == inode_out);
-
-	xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
-	if (!same_inode)
-		xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
-	inode_unlock(inode_out);
-	if (!same_inode)
-		inode_unlock(inode_in);
-}
-
 /*
  * If we're reflinking to a point past the destination file's EOF, we must
  * zero any speculative post-EOF preallocations that sit between the old EOF
@@ -1340,18 +1265,12 @@  xfs_reflink_remap_prep(
 	struct xfs_inode	*src = XFS_I(inode_in);
 	struct inode		*inode_out = file_inode(file_out);
 	struct xfs_inode	*dest = XFS_I(inode_out);
-	bool			same_inode = (inode_in == inode_out);
 	int			ret;
 
 	/* Lock both files against IO */
-	ret = xfs_iolock_two_inodes_and_break_layout(inode_in, inode_out);
+	ret = xfs_ilock_io_mmap(src, dest);
 	if (ret)
 		return ret;
-	if (same_inode)
-		xfs_ilock(src, XFS_MMAPLOCK_EXCL);
-	else
-		xfs_lock_two_inodes(src, XFS_MMAPLOCK_EXCL, dest,
-				XFS_MMAPLOCK_EXCL);
 
 	/* Check file eligibility and prepare for block sharing. */
 	ret = -EINVAL;
@@ -1402,7 +1321,7 @@  xfs_reflink_remap_prep(
 
 	return 0;
 out_unlock:
-	xfs_reflink_remap_unlock(file_in, file_out);
+	xfs_iunlock_io_mmap(src, dest);
 	return ret;
 }
 
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 3e4fd46373ab..487b00434b96 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -56,7 +56,5 @@  extern int xfs_reflink_remap_blocks(struct xfs_inode *src, loff_t pos_in,
 		loff_t *remapped);
 extern int xfs_reflink_update_dest(struct xfs_inode *dest, xfs_off_t newlen,
 		xfs_extlen_t cowextsize, unsigned int remap_flags);
-extern void xfs_reflink_remap_unlock(struct file *file_in,
-		struct file *file_out);
 
 #endif /* __XFS_REFLINK_H */