Re: [PATCH] xfs: reflink should force the log out if mounted with wsync

[Dave Chinner <david@fromorbit.com>]

On Wed, Apr 01, 2020 at 09:17:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>
> ---
>  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@fromorbit.com