Re: [PATCH 2/6] xfs: don't continue on error in xfs_fsync

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Sep 02, 2026 at 08:49:16AM +0300, Christoph Hellwig wrote:
> As soon as we get an error from cache flushing or log forcing, there
> is no point in continuing as the data integrity is already impacted.
> Return the error instead of continuing to do more work.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_file.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 426a67b813a7..0d31fea67a2c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -130,8 +130,8 @@ xfs_file_fsync(
>  {
>  	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
>  	struct xfs_mount	*mp = ip->i_mount;
> -	int			error, err2;
>  	int			log_flushed = 0;
> +	int			error;
>  
>  	trace_xfs_file_fsync(ip);
>  
> @@ -154,15 +154,17 @@ xfs_file_fsync(
>  		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
>  	else if (mp->m_logdev_targp != mp->m_ddev_targp)
>  		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
> +	if (error)
> +		return error;

Hmm so at this point dirty writeback was pushed out and failed, so now
already we've lost data.  No point in continuing, agreed...

>  
>  	/*
>  	 * If the inode has a inode log item attached, it may need the journal
>  	 * flushed to persist any changes the log item might be tracking.
>  	 */
>  	if (ip->i_itemp) {
> -		err2 = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> -		if (err2 && !error)
> -			error = err2;
> +		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> +		if (error)
> +			return error;

...so at this point we failed to push the log to disk and possibly
lost file metadata.  Agreed that we might as well give up...

>  	}
>  
>  	/*
> @@ -178,14 +180,11 @@ xfs_file_fsync(
>  	if (!log_flushed) {
>  		struct xfs_buftarg *file_targp = xfs_inode_buftarg(ip);
>  
> -		if (mp->m_logdev_targp == file_targp) {
> -			err2 = blkdev_issue_flush(file_targp->bt_bdev);
> -			if (err2 && !error)
> -				error = err2;
> -		}
> +		if (mp->m_logdev_targp == file_targp)
> +			return blkdev_issue_flush(file_targp->bt_bdev);

...and same here if the bdev flush fails.

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

--D

>  	}
>  
> -	return error;
> +	return 0;
>  }
>  
>  static int
> -- 
> 2.53.0
> 
>