Re: [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/5] xfs: ensure log flush at the end of a synchronous fallocate call
Date: Tue, 1 Feb 2022 08:37:08 -0800	[thread overview]
Message-ID: <20220201163708.GI8313@magnolia> (raw)
In-Reply-To: <20220131064350.739863-6-david@fromorbit.com>

On Mon, Jan 31, 2022 at 05:43:50PM +1100, Dave Chinner wrote:
> From: "Darrick J. Wong" <djwong@kernel.org>
> 
> Since we've started treating fallocate more like a file write, we
> should flush the log to disk if the user has asked for synchronous
> writes either by setting it via fcntl flags, or inode flags, or with
> the sync mount option.  We've already got a helper for this, so use
> it.
> 
> [Slightly massaged by <dchinner@redhat.com> to fit this patchset]

I think you made more than 'slight massage' changes...

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_file.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ddc3336e8f84..209cba0f0ddc 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -861,6 +861,21 @@ xfs_break_layouts(
>  	return error;
>  }
>  
> +/* 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 (xfs_has_wsync(ip->i_mount))
> +		return true;
> +	if (filp->f_flags & (__O_SYNC | O_DSYNC))
> +		return true;
> +	if (IS_SYNC(file_inode(filp)))
> +		return true;
> +
> +	return false;
> +}
> +
>  #define	XFS_FALLOC_FL_SUPPORTED						\
>  		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
>  		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |	\
> @@ -1045,7 +1060,7 @@ xfs_file_fallocate(
>  	if (do_file_insert)
>  		error = xfs_insert_file_space(ip, offset, len);
>  
> -	if (file->f_flags & O_DSYNC)
> +	if (xfs_file_sync_writes(file))
>  		error = xfs_log_force_inode(ip);

...since the preceeding patches that you wrote enable simpler logic
here.  You've done all the (re)thinking necessary to get here, so I
think on those grounds:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  out_unlock:
> @@ -1078,21 +1093,6 @@ 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 (xfs_has_wsync(ip->i_mount))
> -		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,
> -- 
> 2.33.0
> 