Re: [PATCH 4/7] libxfs: flush all dirty buffers and report errors when unmounting filesystem

[Brian Foster <bfoster@redhat.com>]

On Mon, Feb 24, 2020 at 04:10:53PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach libxfs_umount to flush all dirty buffers when unmounting the
> filesystem, to log write verifier errors and IO errors, and to return an
> error code when things go wrong.  Subsequent patches will teach critical
> utilities to exit with EXIT_FAILURE when this happens.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Looks reasonable to me:

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

>  include/xfs_mount.h |    2 +
>  libxfs/init.c       |   96 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  libxfs/libxfs_io.h  |    7 ++++
>  libxfs/rdwr.c       |   38 ++++++++++++++++++--
>  4 files changed, 135 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> index 29b3cc1b..7bd23fbb 100644
> --- a/include/xfs_mount.h
> +++ b/include/xfs_mount.h
> @@ -184,7 +184,7 @@ xfs_perag_resv(
>  
>  extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
>  				dev_t, dev_t, dev_t, int);
> -extern void	libxfs_umount (xfs_mount_t *);
> +int		libxfs_umount(struct xfs_mount *mp);
>  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
>  
>  #endif	/* __XFS_MOUNT_H__ */
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a0d4b7f4..d4804ead 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
>  	}
>  	btp->bt_mount = mp;
>  	btp->dev = dev;
> +	btp->flags = 0;
> +
>  	return btp;
>  }
>  
> @@ -791,17 +793,104 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
>  	mp->m_rsumip = mp->m_rbmip = NULL;
>  }
>  
> +/* Flush a device and report on writes that didn't make it to stable storage. */
> +static inline int
> +libxfs_flush_buftarg(
> +	struct xfs_buftarg	*btp,
> +	const char		*buftarg_descr)
> +{
> +	int			error = 0;
> +	int			err2;
> +
> +	/*
> +	 * Write verifier failures are evidence of a buggy program.  Make sure
> +	 * that this state is always reported to the caller.
> +	 */
> +	if (btp->flags & XFS_BUFTARG_CORRUPT_WRITE) {
> +		fprintf(stderr,
> +_("%s: Refusing to write a corrupt buffer to the %s!\n"),
> +				progname, buftarg_descr);
> +		error = -EFSCORRUPTED;
> +	}
> +
> +	if (btp->flags & XFS_BUFTARG_LOST_WRITE) {
> +		fprintf(stderr,
> +_("%s: Lost a write to the %s!\n"),
> +				progname, buftarg_descr);
> +		if (!error)
> +			error = -EIO;
> +	}
> +
> +	err2 = libxfs_blkdev_issue_flush(btp);
> +	if (err2) {
> +		fprintf(stderr,
> +_("%s: Flushing the %s failed, err=%d!\n"),
> +				progname, buftarg_descr, -err2);
> +	}
> +	if (!error)
> +		error = err2;
> +
> +	return error;
> +}
> +
> +/*
> + * Flush all dirty buffers to stable storage and report on writes that didn't
> + * make it to stable storage.
> + */
> +static int
> +libxfs_flush_mount(
> +	struct xfs_mount	*mp)
> +{
> +	int			error = 0;
> +	int			err2;
> +
> +	/*
> +	 * Purge the buffer cache to write all dirty buffers to disk and free
> +	 * all incore buffers.  Buffers that fail write verification will cause
> +	 * the CORRUPT_WRITE flag to be set in the buftarg.  Buffers that
> +	 * cannot be written will cause the LOST_WRITE flag to be set in the
> +	 * buftarg.
> +	 */
> +	libxfs_bcache_purge();
> +
> +	/* Flush all kernel and disk write caches, and report failures. */
> +	if (mp->m_ddev_targp) {
> +		err2 = libxfs_flush_buftarg(mp->m_ddev_targp, _("data device"));
> +		if (!error)
> +			error = err2;
> +	}
> +
> +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> +		err2 = libxfs_flush_buftarg(mp->m_logdev_targp,
> +				_("log device"));
> +		if (!error)
> +			error = err2;
> +	}
> +
> +	if (mp->m_rtdev_targp) {
> +		err2 = libxfs_flush_buftarg(mp->m_rtdev_targp,
> +				_("realtime device"));
> +		if (!error)
> +			error = err2;
> +	}
> +
> +	return error;
> +}
> +
>  /*
>   * Release any resource obtained during a mount.
>   */
> -void
> -libxfs_umount(xfs_mount_t *mp)
> +int
> +libxfs_umount(
> +	struct xfs_mount	*mp)
>  {
>  	struct xfs_perag	*pag;
>  	int			agno;
> +	int			error;
>  
>  	libxfs_rtmount_destroy(mp);
> -	libxfs_bcache_purge();
> +
> +	error = libxfs_flush_mount(mp);
>  
>  	for (agno = 0; agno < mp->m_maxagi; agno++) {
>  		pag = radix_tree_delete(&mp->m_perag_tree, agno);
> @@ -816,6 +905,7 @@ libxfs_umount(xfs_mount_t *mp)
>  		kmem_free(mp->m_logdev_targp);
>  	kmem_free(mp->m_ddev_targp);
>  
> +	return error;
>  }
>  
>  /*
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 579df52b..6bb75a67 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -23,10 +23,17 @@ struct xfs_perag;
>  struct xfs_buftarg {
>  	struct xfs_mount	*bt_mount;
>  	dev_t			dev;
> +	unsigned int		flags;
>  };
>  
> +/* We purged a dirty buffer and lost a write. */
> +#define XFS_BUFTARG_LOST_WRITE		(1 << 0)
> +/* A dirty buffer failed the write verifier. */
> +#define XFS_BUFTARG_CORRUPT_WRITE	(1 << 1)
> +
>  extern void	libxfs_buftarg_init(struct xfs_mount *mp, dev_t ddev,
>  				    dev_t logdev, dev_t rtdev);
> +int libxfs_blkdev_issue_flush(struct xfs_buftarg *btp);
>  
>  #define LIBXFS_BBTOOFF64(bbs)	(((xfs_off_t)(bbs)) << BBSHIFT)
>  
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 8b47d438..4253b890 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -17,6 +17,7 @@
>  #include "xfs_inode_fork.h"
>  #include "xfs_inode.h"
>  #include "xfs_trans.h"
> +#include "libfrog/platform.h"
>  
>  #include "libxfs.h"		/* for LIBXFS_EXIT_ON_FAILURE */
>  
> @@ -1219,6 +1220,19 @@ libxfs_iomove(xfs_buf_t *bp, uint boff, int len, void *data, int flags)
>  	}
>  }
>  
> +/* Complain about (and remember) dropping dirty buffers. */
> +static void
> +libxfs_whine_dirty_buf(
> +	struct xfs_buf		*bp)
> +{
> +	fprintf(stderr, _("%s: Releasing dirty buffer to free list!\n"),
> +			progname);
> +
> +	if (bp->b_error == -EFSCORRUPTED)
> +		bp->b_target->flags |= XFS_BUFTARG_CORRUPT_WRITE;
> +	bp->b_target->flags |= XFS_BUFTARG_LOST_WRITE;
> +}
> +
>  static void
>  libxfs_brelse(
>  	struct cache_node	*node)
> @@ -1228,8 +1242,7 @@ libxfs_brelse(
>  	if (!bp)
>  		return;
>  	if (bp->b_flags & LIBXFS_B_DIRTY)
> -		fprintf(stderr,
> -			"releasing dirty buffer to free list!\n");
> +		libxfs_whine_dirty_buf(bp);
>  
>  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
>  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> @@ -1249,8 +1262,7 @@ libxfs_bulkrelse(
>  
>  	list_for_each_entry(bp, list, b_node.cn_mru) {
>  		if (bp->b_flags & LIBXFS_B_DIRTY)
> -			fprintf(stderr,
> -				"releasing dirty buffer (bulk) to free list!\n");
> +			libxfs_whine_dirty_buf(bp);
>  		count++;
>  	}
>  
> @@ -1479,6 +1491,24 @@ libxfs_irele(
>  	kmem_cache_free(xfs_inode_zone, ip);
>  }
>  
> +/*
> + * Flush everything dirty in the kernel and disk write caches to stable media.
> + * Returns 0 for success or a negative error code.
> + */
> +int
> +libxfs_blkdev_issue_flush(
> +	struct xfs_buftarg	*btp)
> +{
> +	int			fd, ret;
> +
> +	if (btp->dev == 0)
> +		return 0;
> +
> +	fd = libxfs_device_to_fd(btp->dev);
> +	ret = platform_flush_device(fd, btp->dev);
> +	return ret ? -errno : 0;
> +}
> +
>  /*
>   * Write out a buffer list synchronously.
>   *
>