On Mon, Jun 22, 2020 at 09:01:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
I'd prefer to see refactoring patches separate from moving patches, but
looks Ok:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> 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 */
>
