On Wed, Apr 01, 2020 at 09:17:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Reflink should force the log out to disk if the filesystem was mounted
> with wsync, the same as most other operations in xfs.
>
> Fixes: 3fc9f5e409319 ("xfs: remove xfs_reflink_remap_range")
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_file.c | 21 +++++++++++++++++++--
> fs/xfs/xfs_reflink.c | 15 ++++++++++++++-
> fs/xfs/xfs_reflink.h | 3 ++-
> 3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index b8a4a3f29b36..17bdc5bbc2ae 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1029,8 +1029,10 @@ xfs_file_remap_range(
> struct inode *inode_out = file_inode(file_out);
> struct xfs_inode *dest = XFS_I(inode_out);
> struct xfs_mount *mp = src->i_mount;
> + xfs_lsn_t sync_lsn = 0;
> loff_t remapped = 0;
> xfs_extlen_t cowextsize;
> + bool need_sync;
> int ret;
>
> if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> @@ -1068,13 +1070,28 @@ xfs_file_remap_range(
> cowextsize = src->i_d.di_cowextsize;
>
> ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize,
> - remap_flags);
> + remap_flags, &need_sync);
> + if (ret)
> + goto out_unlock;
> +
> + /*
> + * If this is a synchronous mount and xfs_reflink_update_dest didn't
> + * already take care of this, make sure that the transaction goes to
> + * disk before returning to the user.
> + */
> + if (need_sync && xfs_ipincount(dest))
> + sync_lsn = dest->i_itemp->ili_last_lsn;
>
> out_unlock:
> xfs_reflink_remap_unlock(file_in, file_out);
> if (ret)
> trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
> - return remapped > 0 ? remapped : ret;
> + if (remapped > 0) {
> + if (sync_lsn)
> + xfs_log_force_lsn(mp, sync_lsn, XFS_LOG_SYNC, NULL);
> + return remapped;
> + }
> + return ret;
This seems pretty fragile compared to all the other WSYNC cases
which just set the last transaction as a sync transaction.
Why can't we just set the transaction sync at the appropriate time,
and if we have to do two sync commits for a reflink then we just
suck it up for now?
As it is, wsync is really only used for active/passive HA configs
these days, which means we've already given up on performance
because data integrity is an essential requirement in those
configs...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
