Re: [PATCH] xfs: force the log after remapping a synchronous-writes file

From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] xfs: force the log after remapping a synchronous-writes file
Date: Fri, 4 Sep 2020 07:24:51 -0400	[thread overview]
Message-ID: <20200904112451.GA529978@bfoster> (raw)
In-Reply-To: <20200904031100.GZ6096@magnolia>

On Thu, Sep 03, 2020 at 08:11:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Commit 5833112df7e9 tried to make it so that a remap operation would
> force the log out to disk if the filesystem is mounted with mandatory
> synchronous writes.  Unfortunately, that commit failed to handle the
> case where the inode or the file descriptor require mandatory
> synchronous writes.
> 
> Refactor the check into into a helper that will look for all three
> conditions, and now we can treat reflink just like any other synchronous
> write.
> 
> Fixes: 5833112df7e9 ("xfs: reflink should force the log out if mounted with wsync")

More of a process thought than an issue with this particular patch, but
I feel like the Fixes tag thing gets more watered down as we attempt to
apply it to more patches. Is it really necessary here? If so, what's the
reasoning? I thought it was more of a "this previous patch has a bug,"
but that link seems a bit tenuous here given the original patch refers
specifically to wsync. Sure, a stable kernel probably wants both
patches, but is that really the primary purpose of "Fixes?"

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

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

>  fs/xfs/xfs_file.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c31cd3be9fb2..ee43f137830c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1008,6 +1008,21 @@ xfs_file_fadvise(
>  	return ret;
>  }
>  
> +/* Does this file, inode, or mount want synchronous writes? */
> +static inline bool xfs_file_sync_writes(struct file *filp)
> +{
> +	struct xfs_inode	*ip = XFS_I(file_inode(filp));
> +
> +	if (ip->i_mount->m_flags & XFS_MOUNT_WSYNC)
> +		return true;
> +	if (filp->f_flags & (__O_SYNC | O_DSYNC))
> +		return true;
> +	if (IS_SYNC(file_inode(filp)))
> +		return true;
> +
> +	return false;
> +}
> +
>  STATIC loff_t
>  xfs_file_remap_range(
>  	struct file		*file_in,
> @@ -1065,7 +1080,7 @@ xfs_file_remap_range(
>  	if (ret)
>  		goto out_unlock;
>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> +	if (xfs_file_sync_writes(file_in) || xfs_file_sync_writes(file_out))
>  		xfs_log_force_inode(dest);
>  out_unlock:
>  	xfs_iunlock2_io_mmap(src, dest);
> 