Re: [PATCH 1/2] xfs: check return codes when flushing block devices

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 04, 2022 at 11:06:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If a blkdev_issue_flush fails, fsync needs to report that to upper
> levels.  Modify xfs_file_fsync to capture the errors, while trying to
> flush as much data and log updates to disk as possible.
> 
> If log writes cannot flush the data device, we need to shut down the log
> immediately because we've violated a log invariant.  Modify this code to
> check the return value of blkdev_issue_flush as well.
> 
> This behavior seems to go back to about 2.6.15 or so, which makes this
> fixes tag a bit misleading.
> 
> Link: https://elixir.bootlin.com/linux/v2.6.15/source/fs/xfs/xfs_vnodeops.c#L1187
> Fixes: b5071ada510a ("xfs: remove xfs_blkdev_issue_flush")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_file.c |   22 ++++++++++++++--------
>  fs/xfs/xfs_log.c  |   11 +++++++++--
>  2 files changed, 23 insertions(+), 10 deletions(-)

Looks good, couple of minor nits you can take or leave.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 5a171c0b244b..a02000be931b 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -142,7 +142,7 @@ xfs_file_fsync(
>  {
>  	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
>  	struct xfs_mount	*mp = ip->i_mount;
> -	int			error = 0;
> +	int			error, err2;
>  	int			log_flushed = 0;
>  
>  	trace_xfs_file_fsync(ip);
> @@ -163,18 +163,21 @@ xfs_file_fsync(
>  	 * inode size in case of an extending write.
>  	 */
>  	if (XFS_IS_REALTIME_INODE(ip))
> -		blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
> +		error = blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
>  	else if (mp->m_logdev_targp != mp->m_ddev_targp)
> -		blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
> +		error = blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
>  
>  	/*
>  	 * Any inode that has dirty modifications in the log is pinned.  The
> -	 * racy check here for a pinned inode while not catch modifications
> +	 * racy check here for a pinned inode will not catch modifications
>  	 * that happen concurrently to the fsync call, but fsync semantics
>  	 * only require to sync previously completed I/O.
>  	 */
> -	if (xfs_ipincount(ip))
> -		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> +	if (xfs_ipincount(ip)) {
> +		err2 = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> +		if (!error && err2)
> +			error = err2;

This is better done as

		if (err2 && !error)
			.....

Because we only care about the value of error if err2 is non zero.
Hence for normal operation where there are no errors, checking err2
first is less code to execute as error never needs to be checked...

> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4b1c0a9c6368..15d7cdc7a632 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1925,9 +1925,16 @@ xlog_write_iclog(
>  		 * device cache first to ensure all metadata writeback covered
>  		 * by the LSN in this iclog is on stable storage. This is slow,
>  		 * but it *must* complete before we issue the external log IO.
> +		 *
> +		 * If the flush fails, we cannot conclude that past metadata
> +		 * writeback from the log succeeded, which is effectively a

		 * writeback from the log succeeded, and repeating
		 * the flush from iclog IO is not possible. Hence we have to
		 * shut down with log IO error to avoid shutdown
		 * re-entering this path and erroring out here again.
		 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com