[PATCH v2 0/8] xfsprogs: actually check that writes succeeded

[PATCH v2 0/8] xfsprogs: actually check that writes succeeded

[Darrick J. Wong]

Hi all,

A code audit demonstrated that many xfsprogs utilities do not check that
the buffers they write actually make it to disk.  While the userspace
buffer cache has a means to check that a buffer was written (which is to
reread the buffer after the write), most utilities mark a buffer dirty
and release it to the MRU and do not re-check the buffer.

Worse yet, the MRU will retain the buffers for all failed writes until
the buffer cache is torn down, but it in turn has no way to communicate
that writes were lost due to IO errors.  libxfs will flush the device
when it is unmounted, but as there is no return value, we again fail to
notice that writes have been lost.  Most likely this leads to a corrupt
filesystem, which makes it all the more surprising that xfs_repair can
lose writes yet still return 0!

Fix all this by making delwri_submit a synchronous write operation like
its kernel counterpart; teaching the buffer cache to mark the buftarg
when it knows it's losing writes; teaching the device flush functions to
return error codes; and adding a new "flush filesystem" API that user
programs can call to check for lost writes or IO errors.  Then teach all
the userspace programs to flush the fs at exit and report errors.

In v2 we split up some of the patches and make sure we always fsync when
flushing a block device.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=buffer-write-fixes

[PATCH 1/8] libxfs: libxfs_buf_delwri_submit should write buffers immediately

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The whole point of libxfs_buf_delwri_submit is to submit a bunch of
buffers for write and wait for the response.  Unfortunately, while it
does mark the buffers dirty, it doesn't actually flush them and lets the
cache mru flusher do it.  This is inconsistent with the kernel API,
which actually writes the buffers and returns any IO errors.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/rdwr.c   |    3 ++-
 mkfs/xfs_mkfs.c |   16 ++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 0d9d7202..2e9f66cc 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1491,9 +1491,10 @@ xfs_buf_delwri_submit(
 
 	list_for_each_entry_safe(bp, n, buffer_list, b_list) {
 		list_del_init(&bp->b_list);
-		error2 = libxfs_writebuf(bp, 0);
+		error2 = libxfs_writebufr(bp);
 		if (!error)
 			error = error2;
+		libxfs_putbuf(bp);
 	}
 
 	return error;
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 5a042917..1f5d2105 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3685,6 +3685,7 @@ main(
 	};
 
 	struct list_head	buffer_list;
+	int			error;
 
 	platform_uuid_generate(&cli.uuid);
 	progname = basename(argv[0]);
@@ -3885,16 +3886,19 @@ main(
 		if (agno % 16)
 			continue;
 
-		if (libxfs_buf_delwri_submit(&buffer_list)) {
-			fprintf(stderr, _("%s: writing AG headers failed\n"),
-					progname);
+		error = -libxfs_buf_delwri_submit(&buffer_list);
+		if (error) {
+			fprintf(stderr,
+	_("%s: writing AG headers failed, err=%d\n"),
+					progname, error);
 			exit(1);
 		}
 	}
 
-	if (libxfs_buf_delwri_submit(&buffer_list)) {
-		fprintf(stderr, _("%s: writing AG headers failed\n"),
-				progname);
+	error = -libxfs_buf_delwri_submit(&buffer_list);
+	if (error) {
+		fprintf(stderr, _("%s: writing AG headers failed, err=%d\n"),
+				progname, error);
 		exit(1);
 	}
 

[PATCH 2/8] libxfs: complain when write IOs fail

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Complain whenever a metadata write fails.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/rdwr.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 2e9f66cc..8b47d438 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1149,7 +1149,12 @@ libxfs_writebufr(xfs_buf_t *bp)
 			(long long)LIBXFS_BBTOOFF64(bp->b_bn),
 			(long long)bp->b_bn, bp, bp->b_error);
 #endif
-	if (!bp->b_error) {
+	if (bp->b_error) {
+		fprintf(stderr,
+	_("%s: write failed on %s bno 0x%llx/0x%x, err=%d\n"),
+			__func__, bp->b_ops->name,
+			(long long)bp->b_bn, bp->b_bcount, -bp->b_error);
+	} else {
 		bp->b_flags |= LIBXFS_B_UPTODATE;
 		bp->b_flags &= ~(LIBXFS_B_DIRTY | LIBXFS_B_EXIT |
 				 LIBXFS_B_UNCHECKED);

[PATCH 3/8] libxfs: return flush failures

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Modify platform_flush_device so that we can return error status when
device flushes fail.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libfrog/linux.c    |   25 +++++++++++++++++--------
 libfrog/platform.h |    2 +-
 2 files changed, 18 insertions(+), 9 deletions(-)


diff --git a/libfrog/linux.c b/libfrog/linux.c
index 41a168b4..60bc1dc4 100644
--- a/libfrog/linux.c
+++ b/libfrog/linux.c
@@ -140,20 +140,29 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
 	return error;
 }
 
-void
-platform_flush_device(int fd, dev_t device)
+/*
+ * Flush dirty pagecache and disk write cache to stable media.  Returns 0 for
+ * success or -1 (with errno set) for failure.
+ */
+int
+platform_flush_device(
+	int		fd,
+	dev_t		device)
 {
 	struct stat	st;
+	int		ret;
+
 	if (major(device) == RAMDISK_MAJOR)
-		return;
+		return 0;
 
-	if (fstat(fd, &st) < 0)
-		return;
+	ret = fstat(fd, &st);
+	if (ret)
+		return ret;
 
 	if (S_ISREG(st.st_mode))
-		fsync(fd);
-	else
-		ioctl(fd, BLKFLSBUF, 0);
+		return fsync(fd);
+
+	return ioctl(fd, BLKFLSBUF, 0);
 }
 
 void
diff --git a/libfrog/platform.h b/libfrog/platform.h
index 76887e5e..0aef318a 100644
--- a/libfrog/platform.h
+++ b/libfrog/platform.h
@@ -12,7 +12,7 @@ int platform_check_ismounted(char *path, char *block, struct stat *sptr,
 int platform_check_iswritable(char *path, char *block, struct stat *sptr);
 int platform_set_blocksize(int fd, char *path, dev_t device, int bsz,
 		int fatal);
-void platform_flush_device(int fd, dev_t device);
+int platform_flush_device(int fd, dev_t device);
 char *platform_findrawpath(char *path);
 char *platform_findblockpath(char *path);
 int platform_direct_blockdev(void);

[PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a new function that will ensure that everything we changed has
landed on stable media, and report the results.  Subsequent commits will
teach the individual programs to report when things go wrong.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/xfs_mount.h |    3 +++
 libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
 libxfs/libxfs_io.h  |    2 ++
 libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
 4 files changed, 73 insertions(+), 2 deletions(-)


diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 29b3cc1b..c80aaf69 100644
--- a/include/xfs_mount.h
+++ b/include/xfs_mount.h
@@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
 extern void	libxfs_umount (xfs_mount_t *);
 extern void	libxfs_rtmount_destroy (xfs_mount_t *);
 
+void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
+		int *rtdev);
+
 #endif	/* __XFS_MOUNT_H__ */
diff --git a/libxfs/init.c b/libxfs/init.c
index a0d4b7f4..d1d3f4df 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
 	}
 	btp->bt_mount = mp;
 	btp->dev = dev;
+	btp->lost_writes = false;
+
 	return btp;
 }
 
@@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
 	mp->m_rsumip = mp->m_rbmip = NULL;
 }
 
+static inline int
+libxfs_flush_buftarg(
+	struct xfs_buftarg	*btp)
+{
+	if (btp->lost_writes)
+		return -ENOTRECOVERABLE;
+
+	return libxfs_blkdev_issue_flush(btp);
+}
+
+/*
+ * Purge the buffer cache to write all dirty buffers to disk and free all
+ * incore buffers.  Buffers that cannot be written will cause the lost_writes
+ * flag to be set in the buftarg.  If there were no lost writes, flush the
+ * device to make sure the writes made it to stable storage.
+ *
+ * For each device, the return code will be set to -ENOTRECOVERABLE if we
+ * couldn't write something to disk; or the results of the block device flush
+ * operation.
+ */
+void
+libxfs_flush_devices(
+	struct xfs_mount	*mp,
+	int			*datadev,
+	int			*logdev,
+	int			*rtdev)
+{
+	*datadev = *logdev = *rtdev = 0;
+
+	libxfs_bcache_purge();
+
+	if (mp->m_ddev_targp)
+		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
+
+	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
+		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
+
+	if (mp->m_rtdev_targp)
+		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
+}
+
 /*
  * Release any resource obtained during a mount.
  */
diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 579df52b..fc0fd060 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -23,10 +23,12 @@ struct xfs_perag;
 struct xfs_buftarg {
 	struct xfs_mount	*bt_mount;
 	dev_t			dev;
+	bool			lost_writes;
 };
 
 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..92e497f9 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 */
 
@@ -1227,9 +1228,11 @@ libxfs_brelse(
 
 	if (!bp)
 		return;
-	if (bp->b_flags & LIBXFS_B_DIRTY)
+	if (bp->b_flags & LIBXFS_B_DIRTY) {
 		fprintf(stderr,
 			"releasing dirty buffer to free list!\n");
+		bp->b_target->lost_writes = true;
+	}
 
 	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
 	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
@@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
 		return 0 ;
 
 	list_for_each_entry(bp, list, b_node.cn_mru) {
-		if (bp->b_flags & LIBXFS_B_DIRTY)
+		if (bp->b_flags & LIBXFS_B_DIRTY) {
 			fprintf(stderr,
 				"releasing dirty buffer (bulk) to free list!\n");
+			bp->b_target->lost_writes = true;
+		}
 		count++;
 	}
 
@@ -1479,6 +1484,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.
  *

[PATCH 5/8] xfs_db: check that metadata updates have been committed

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a new function that will ensure that everything we scribbled on has
landed on stable media, and report the results.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/init.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)


diff --git a/db/init.c b/db/init.c
index 0ac37368..e92de232 100644
--- a/db/init.c
+++ b/db/init.c
@@ -184,6 +184,7 @@ main(
 	char	*input;
 	char	**v;
 	int	start_iocur_sp;
+	int	d, l, r;
 
 	init(argc, argv);
 	start_iocur_sp = iocur_sp;
@@ -216,6 +217,19 @@ main(
 	 */
 	while (iocur_sp > start_iocur_sp)
 		pop_cur();
+
+	libxfs_flush_devices(mp, &d, &l, &r);
+	if (d)
+		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
+				progname, d);
+	if (l)
+		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
+				progname, l);
+	if (r)
+		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
+				progname, r);
+
+
 	libxfs_umount(mp);
 	if (x.ddev)
 		libxfs_device_close(x.ddev);

[PATCH 6/8] mkfs: check that metadata updates have been committed

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a new function that will ensure that everything we formatted has
landed on stable media, and report the results.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/xfs_mkfs.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1f5d2105..6b182264 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3644,6 +3644,7 @@ main(
 	char			*protofile = NULL;
 	char			*protostring = NULL;
 	int			worst_freelist = 0;
+	int			d, l, r;
 
 	struct libxfs_xinit	xi = {
 		.isdirect = LIBXFS_DIRECT,
@@ -3940,6 +3941,20 @@ main(
 	(XFS_BUF_TO_SBP(buf))->sb_inprogress = 0;
 	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
 
+	/* Make sure our new fs made it to stable storage. */
+	libxfs_flush_devices(mp, &d, &l, &r);
+	if (d)
+		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
+				progname, d);
+	if (l)
+		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
+				progname, l);
+	if (r)
+		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
+				progname, r);
+	if (d || l || r)
+		return 1;
+
 	libxfs_umount(mp);
 	if (xi.rtdev)
 		libxfs_device_close(xi.rtdev);

[PATCH 7/8] xfs_repair: check that metadata updates have been committed

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a new function that will ensure that everything we changed has
landed on stable media, and report the results.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/xfs_repair.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index eb1ce546..c0a77cad 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -690,6 +690,36 @@ check_fs_vs_host_sectsize(
 	}
 }
 
+/* Flush the devices and complain if anything bad happened. */
+static bool
+check_write_failed(
+	struct xfs_mount	*mp)
+{
+	int			d, l, r;
+
+	libxfs_flush_devices(mp, &d, &l, &r);
+
+	if (d == -ENOTRECOVERABLE)
+		do_warn(_("Lost writes to data device, please re-run.\n"));
+	else if (d)
+		do_warn(_("Error %d flushing data device, please re-run.\n"),
+				-d);
+
+	if (l == -ENOTRECOVERABLE)
+		do_warn(_("Lost writes to log device, please re-run.\n"));
+	else if (l)
+		do_warn(_("Error %d flushing log device, please re-run.\n"),
+				-l);
+
+	if (r == -ENOTRECOVERABLE)
+		do_warn(_("Lost writes to realtime device, please re-run.\n"));
+	else if (r)
+		do_warn(_("Error %d flushing realtime device, please re-run.\n"),
+				-r);
+
+	return d || l || r;
+}
+
 int
 main(int argc, char **argv)
 {
@@ -703,6 +733,7 @@ main(int argc, char **argv)
 	struct xfs_sb	psb;
 	int		rval;
 	struct xfs_ino_geometry	*igeo;
+	bool		writes_failed;
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");
@@ -1106,6 +1137,8 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 	format_log_max_lsn(mp);
 	libxfs_umount(mp);
 
+	writes_failed = check_write_failed(mp);
+
 	if (x.rtdev)
 		libxfs_device_close(x.rtdev);
 	if (x.logdev && x.logdev != x.ddev)
@@ -1125,6 +1158,8 @@ _("Repair of readonly mount complete.  Immediate reboot encouraged.\n"));
 
 	free(msgbuf);
 
+	if (writes_failed)
+		return 1;
 	if (fs_is_dirty && report_corrected)
 		return (4);
 	return (0);

[PATCH 8/8] libfrog: always fsync when flushing a device

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Always call fsync() when we're flushing a device, even if it is a block
device.  It's probably redundant to call fsync /and/ BLKFLSBUF, but the
latter has odd behavior so we want to make sure the standard flush
methods have a chance to run first.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libfrog/linux.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)


diff --git a/libfrog/linux.c b/libfrog/linux.c
index 60bc1dc4..40a839d1 100644
--- a/libfrog/linux.c
+++ b/libfrog/linux.c
@@ -155,14 +155,18 @@ platform_flush_device(
 	if (major(device) == RAMDISK_MAJOR)
 		return 0;
 
+	ret = fsync(fd);
+	if (ret)
+		return ret;
+
 	ret = fstat(fd, &st);
 	if (ret)
 		return ret;
 
-	if (S_ISREG(st.st_mode))
-		return fsync(fd);
+	if (S_ISBLK(st.st_mode))
+		return ioctl(fd, BLKFLSBUF, 0);
 
-	return ioctl(fd, BLKFLSBUF, 0);
+	return 0;
 }
 
 void

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Brian Foster]

On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a new function that will ensure that everything we changed has
> landed on stable media, and report the results.  Subsequent commits will
> teach the individual programs to report when things go wrong.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  include/xfs_mount.h |    3 +++
>  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
>  libxfs/libxfs_io.h  |    2 ++
>  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
>  4 files changed, 73 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> index 29b3cc1b..c80aaf69 100644
> --- a/include/xfs_mount.h
> +++ b/include/xfs_mount.h
> @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
>  extern void	libxfs_umount (xfs_mount_t *);
>  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
>  
> +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> +		int *rtdev);
> +
>  #endif	/* __XFS_MOUNT_H__ */
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a0d4b7f4..d1d3f4df 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
>  	}
>  	btp->bt_mount = mp;
>  	btp->dev = dev;
> +	btp->lost_writes = false;
> +
>  	return btp;
>  }
>  
> @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
>  	mp->m_rsumip = mp->m_rbmip = NULL;
>  }
>  
> +static inline int
> +libxfs_flush_buftarg(
> +	struct xfs_buftarg	*btp)
> +{
> +	if (btp->lost_writes)
> +		return -ENOTRECOVERABLE;

I'm curious why we'd want to skip the flush just because some writes
happened to fail..? I suppose the fs might be borked, but it seems a
little strange to at least not try the flush, particularly since we
might still flush any of the other two possible devices.

> +
> +	return libxfs_blkdev_issue_flush(btp);
> +}
> +
> +/*
> + * Purge the buffer cache to write all dirty buffers to disk and free all
> + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> + * flag to be set in the buftarg.  If there were no lost writes, flush the
> + * device to make sure the writes made it to stable storage.
> + *
> + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> + * couldn't write something to disk; or the results of the block device flush
> + * operation.

Why not -EIO?

Brian

> + */
> +void
> +libxfs_flush_devices(
> +	struct xfs_mount	*mp,
> +	int			*datadev,
> +	int			*logdev,
> +	int			*rtdev)
> +{
> +	*datadev = *logdev = *rtdev = 0;
> +
> +	libxfs_bcache_purge();
> +
> +	if (mp->m_ddev_targp)
> +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> +
> +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> +
> +	if (mp->m_rtdev_targp)
> +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> +}
> +
>  /*
>   * Release any resource obtained during a mount.
>   */
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 579df52b..fc0fd060 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -23,10 +23,12 @@ struct xfs_perag;
>  struct xfs_buftarg {
>  	struct xfs_mount	*bt_mount;
>  	dev_t			dev;
> +	bool			lost_writes;
>  };
>  
>  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..92e497f9 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 */
>  
> @@ -1227,9 +1228,11 @@ libxfs_brelse(
>  
>  	if (!bp)
>  		return;
> -	if (bp->b_flags & LIBXFS_B_DIRTY)
> +	if (bp->b_flags & LIBXFS_B_DIRTY) {
>  		fprintf(stderr,
>  			"releasing dirty buffer to free list!\n");
> +		bp->b_target->lost_writes = true;
> +	}
>  
>  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
>  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
>  		return 0 ;
>  
>  	list_for_each_entry(bp, list, b_node.cn_mru) {
> -		if (bp->b_flags & LIBXFS_B_DIRTY)
> +		if (bp->b_flags & LIBXFS_B_DIRTY) {
>  			fprintf(stderr,
>  				"releasing dirty buffer (bulk) to free list!\n");
> +			bp->b_target->lost_writes = true;
> +		}
>  		count++;
>  	}
>  
> @@ -1479,6 +1484,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.
>   *
> 

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Brian Foster]

On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a new function that will ensure that everything we scribbled on has
> landed on stable media, and report the results.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/init.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> 
> diff --git a/db/init.c b/db/init.c
> index 0ac37368..e92de232 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -184,6 +184,7 @@ main(
>  	char	*input;
>  	char	**v;
>  	int	start_iocur_sp;
> +	int	d, l, r;
>  
>  	init(argc, argv);
>  	start_iocur_sp = iocur_sp;
> @@ -216,6 +217,19 @@ main(
>  	 */
>  	while (iocur_sp > start_iocur_sp)
>  		pop_cur();
> +
> +	libxfs_flush_devices(mp, &d, &l, &r);
> +	if (d)
> +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> +				progname, d);
> +	if (l)
> +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> +				progname, l);
> +	if (r)
> +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> +				progname, r);
> +
> +

Seems like we could reduce some boilerplate by passing progname into
libxfs_flush_devices() and letting it dump out of the error messages,
unless there's some future code that cares about individual device error
state.

That said, it also seems the semantics of libxfs_flush_devices() are a
bit different from convention. Just below we invoke
libxfs_device_close() for each device (rather than for all three), and
device_close() also happens to call fsync() and platform_flush_device()
itself...

Brian

>  	libxfs_umount(mp);
>  	if (x.ddev)
>  		libxfs_device_close(x.ddev);
> 

Re: [PATCH 8/8] libfrog: always fsync when flushing a device

[Brian Foster]

On Wed, Feb 19, 2020 at 05:42:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Always call fsync() when we're flushing a device, even if it is a block
> device.  It's probably redundant to call fsync /and/ BLKFLSBUF, but the
> latter has odd behavior so we want to make sure the standard flush
> methods have a chance to run first.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  libfrog/linux.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/libfrog/linux.c b/libfrog/linux.c
> index 60bc1dc4..40a839d1 100644
> --- a/libfrog/linux.c
> +++ b/libfrog/linux.c
> @@ -155,14 +155,18 @@ platform_flush_device(
>  	if (major(device) == RAMDISK_MAJOR)
>  		return 0;
>  
> +	ret = fsync(fd);
> +	if (ret)
> +		return ret;
> +
>  	ret = fstat(fd, &st);
>  	if (ret)
>  		return ret;
>  
> -	if (S_ISREG(st.st_mode))
> -		return fsync(fd);
> +	if (S_ISBLK(st.st_mode))
> +		return ioctl(fd, BLKFLSBUF, 0);
>  
> -	return ioctl(fd, BLKFLSBUF, 0);
> +	return 0;
>  }
>  
>  void
> 

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 09:06:12AM -0500, Brian Foster wrote:
> On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a new function that will ensure that everything we changed has
> > landed on stable media, and report the results.  Subsequent commits will
> > teach the individual programs to report when things go wrong.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/xfs_mount.h |    3 +++
> >  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  libxfs/libxfs_io.h  |    2 ++
> >  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
> >  4 files changed, 73 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > index 29b3cc1b..c80aaf69 100644
> > --- a/include/xfs_mount.h
> > +++ b/include/xfs_mount.h
> > @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> >  extern void	libxfs_umount (xfs_mount_t *);
> >  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
> >  
> > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > +		int *rtdev);
> > +
> >  #endif	/* __XFS_MOUNT_H__ */
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index a0d4b7f4..d1d3f4df 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> >  	}
> >  	btp->bt_mount = mp;
> >  	btp->dev = dev;
> > +	btp->lost_writes = false;
> > +
> >  	return btp;
> >  }
> >  
> > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> >  	mp->m_rsumip = mp->m_rbmip = NULL;
> >  }
> >  
> > +static inline int
> > +libxfs_flush_buftarg(
> > +	struct xfs_buftarg	*btp)
> > +{
> > +	if (btp->lost_writes)
> > +		return -ENOTRECOVERABLE;
> 
> I'm curious why we'd want to skip the flush just because some writes
> happened to fail..? I suppose the fs might be borked, but it seems a
> little strange to at least not try the flush, particularly since we
> might still flush any of the other two possible devices.

My thinking here was that if the write verifiers (or the pwrite() calls
themselves) failed then there's no point in telling the disk to flush
its write cache since we already know it's not in sync with the buffer
cache.

> > +
> > +	return libxfs_blkdev_issue_flush(btp);
> > +}
> > +
> > +/*
> > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> > + * flag to be set in the buftarg.  If there were no lost writes, flush the
> > + * device to make sure the writes made it to stable storage.
> > + *
> > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > + * couldn't write something to disk; or the results of the block device flush
> > + * operation.
> 
> Why not -EIO?

Originally I thought it might be useful to be able to distinguish
between "dirty buffers never even made it out of the buffer cache" vs.
"dirty buffers were sent to the disk but the disk sent back media
errors", though in the end the userspace tools don't make any
distinction.

That said, looking at this again, maybe I should track write verifier
failure separately so that we can return EFSCORRUPTED for that?

--D

> 
> Brian
> 
> > + */
> > +void
> > +libxfs_flush_devices(
> > +	struct xfs_mount	*mp,
> > +	int			*datadev,
> > +	int			*logdev,
> > +	int			*rtdev)
> > +{
> > +	*datadev = *logdev = *rtdev = 0;
> > +
> > +	libxfs_bcache_purge();
> > +
> > +	if (mp->m_ddev_targp)
> > +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > +
> > +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > +
> > +	if (mp->m_rtdev_targp)
> > +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > +}
> > +
> >  /*
> >   * Release any resource obtained during a mount.
> >   */
> > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > index 579df52b..fc0fd060 100644
> > --- a/libxfs/libxfs_io.h
> > +++ b/libxfs/libxfs_io.h
> > @@ -23,10 +23,12 @@ struct xfs_perag;
> >  struct xfs_buftarg {
> >  	struct xfs_mount	*bt_mount;
> >  	dev_t			dev;
> > +	bool			lost_writes;
> >  };
> >  
> >  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..92e497f9 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 */
> >  
> > @@ -1227,9 +1228,11 @@ libxfs_brelse(
> >  
> >  	if (!bp)
> >  		return;
> > -	if (bp->b_flags & LIBXFS_B_DIRTY)
> > +	if (bp->b_flags & LIBXFS_B_DIRTY) {
> >  		fprintf(stderr,
> >  			"releasing dirty buffer to free list!\n");
> > +		bp->b_target->lost_writes = true;
> > +	}
> >  
> >  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
> >  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> > @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
> >  		return 0 ;
> >  
> >  	list_for_each_entry(bp, list, b_node.cn_mru) {
> > -		if (bp->b_flags & LIBXFS_B_DIRTY)
> > +		if (bp->b_flags & LIBXFS_B_DIRTY) {
> >  			fprintf(stderr,
> >  				"releasing dirty buffer (bulk) to free list!\n");
> > +			bp->b_target->lost_writes = true;
> > +		}
> >  		count++;
> >  	}
> >  
> > @@ -1479,6 +1484,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.
> >   *
> > 
> 

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 09:06:23AM -0500, Brian Foster wrote:
> On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a new function that will ensure that everything we scribbled on has
> > landed on stable media, and report the results.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/init.c |   14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > 
> > diff --git a/db/init.c b/db/init.c
> > index 0ac37368..e92de232 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -184,6 +184,7 @@ main(
> >  	char	*input;
> >  	char	**v;
> >  	int	start_iocur_sp;
> > +	int	d, l, r;
> >  
> >  	init(argc, argv);
> >  	start_iocur_sp = iocur_sp;
> > @@ -216,6 +217,19 @@ main(
> >  	 */
> >  	while (iocur_sp > start_iocur_sp)
> >  		pop_cur();
> > +
> > +	libxfs_flush_devices(mp, &d, &l, &r);
> > +	if (d)
> > +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> > +				progname, d);
> > +	if (l)
> > +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> > +				progname, l);
> > +	if (r)
> > +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> > +				progname, r);
> > +
> > +
> 
> Seems like we could reduce some boilerplate by passing progname into
> libxfs_flush_devices() and letting it dump out of the error messages,
> unless there's some future code that cares about individual device error
> state.

Such a program could call libxfs_flush_devices directly, as we do here.

Also, progname is defined in libxfs so we don't even need to pass it as
an argument.

I had originally thought that we should try not to add fprintf calls to
libxfs because libraries aren't really supposed to be doing things like
that, but perhaps you're right that all of this should be melded into
something else.

> That said, it also seems the semantics of libxfs_flush_devices() are a
> bit different from convention. Just below we invoke
> libxfs_device_close() for each device (rather than for all three), and
> device_close() also happens to call fsync() and platform_flush_device()
> itself...

Yeah, the division of responsibilities is a little hazy here -- I would
think that unmounting a filesystem should flush all the memory caches
and then the disk cache, but OTOH it's the utility that opens the
devices and should therefore flush and close them.

I dunno.  My current thinking is that libxfs_umount should call
libxfs_flush_devices() and print error messages as necessary, and return
error codes as appropriate.  xfs_repair can then check the umount return
value and translate that into exit(1) as required.  The device_close
functions will fsync a second time, but that shouldn't be a big deal
because we haven't dirtied anything in the meantime.

Thoughts?

--D

> Brian
> 
> >  	libxfs_umount(mp);
> >  	if (x.ddev)
> >  		libxfs_device_close(x.ddev);
> > 
> 

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Brian Foster]

On Thu, Feb 20, 2020 at 08:46:38AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 09:06:12AM -0500, Brian Foster wrote:
> > On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Add a new function that will ensure that everything we changed has
> > > landed on stable media, and report the results.  Subsequent commits will
> > > teach the individual programs to report when things go wrong.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  include/xfs_mount.h |    3 +++
> > >  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
> > >  libxfs/libxfs_io.h  |    2 ++
> > >  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
> > >  4 files changed, 73 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > > index 29b3cc1b..c80aaf69 100644
> > > --- a/include/xfs_mount.h
> > > +++ b/include/xfs_mount.h
> > > @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> > >  extern void	libxfs_umount (xfs_mount_t *);
> > >  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
> > >  
> > > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > > +		int *rtdev);
> > > +
> > >  #endif	/* __XFS_MOUNT_H__ */
> > > diff --git a/libxfs/init.c b/libxfs/init.c
> > > index a0d4b7f4..d1d3f4df 100644
> > > --- a/libxfs/init.c
> > > +++ b/libxfs/init.c
> > > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> > >  	}
> > >  	btp->bt_mount = mp;
> > >  	btp->dev = dev;
> > > +	btp->lost_writes = false;
> > > +
> > >  	return btp;
> > >  }
> > >  
> > > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> > >  	mp->m_rsumip = mp->m_rbmip = NULL;
> > >  }
> > >  
> > > +static inline int
> > > +libxfs_flush_buftarg(
> > > +	struct xfs_buftarg	*btp)
> > > +{
> > > +	if (btp->lost_writes)
> > > +		return -ENOTRECOVERABLE;
> > 
> > I'm curious why we'd want to skip the flush just because some writes
> > happened to fail..? I suppose the fs might be borked, but it seems a
> > little strange to at least not try the flush, particularly since we
> > might still flush any of the other two possible devices.
> 
> My thinking here was that if the write verifiers (or the pwrite() calls
> themselves) failed then there's no point in telling the disk to flush
> its write cache since we already know it's not in sync with the buffer
> cache.
> 

I suppose, but it seems there is some value in flushing what we did
write.. That's effectively historical behavior (since we ignored
errors), right?

> > > +
> > > +	return libxfs_blkdev_issue_flush(btp);
> > > +}
> > > +
> > > +/*
> > > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > > + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> > > + * flag to be set in the buftarg.  If there were no lost writes, flush the
> > > + * device to make sure the writes made it to stable storage.
> > > + *
> > > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > > + * couldn't write something to disk; or the results of the block device flush
> > > + * operation.
> > 
> > Why not -EIO?
> 
> Originally I thought it might be useful to be able to distinguish
> between "dirty buffers never even made it out of the buffer cache" vs.
> "dirty buffers were sent to the disk but the disk sent back media
> errors", though in the end the userspace tools don't make any
> distinction.
> 
> That said, looking at this again, maybe I should track write verifier
> failure separately so that we can return EFSCORRUPTED for that?
> 

It's not clear to me that anything application level would care much
about verifier failure vs. I/O failure, but I've no objection to doing
something like that either.

Brian

> --D
> 
> > 
> > Brian
> > 
> > > + */
> > > +void
> > > +libxfs_flush_devices(
> > > +	struct xfs_mount	*mp,
> > > +	int			*datadev,
> > > +	int			*logdev,
> > > +	int			*rtdev)
> > > +{
> > > +	*datadev = *logdev = *rtdev = 0;
> > > +
> > > +	libxfs_bcache_purge();
> > > +
> > > +	if (mp->m_ddev_targp)
> > > +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > > +
> > > +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > > +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > > +
> > > +	if (mp->m_rtdev_targp)
> > > +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > > +}
> > > +
> > >  /*
> > >   * Release any resource obtained during a mount.
> > >   */
> > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > > index 579df52b..fc0fd060 100644
> > > --- a/libxfs/libxfs_io.h
> > > +++ b/libxfs/libxfs_io.h
> > > @@ -23,10 +23,12 @@ struct xfs_perag;
> > >  struct xfs_buftarg {
> > >  	struct xfs_mount	*bt_mount;
> > >  	dev_t			dev;
> > > +	bool			lost_writes;
> > >  };
> > >  
> > >  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..92e497f9 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 */
> > >  
> > > @@ -1227,9 +1228,11 @@ libxfs_brelse(
> > >  
> > >  	if (!bp)
> > >  		return;
> > > -	if (bp->b_flags & LIBXFS_B_DIRTY)
> > > +	if (bp->b_flags & LIBXFS_B_DIRTY) {
> > >  		fprintf(stderr,
> > >  			"releasing dirty buffer to free list!\n");
> > > +		bp->b_target->lost_writes = true;
> > > +	}
> > >  
> > >  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
> > >  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> > > @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
> > >  		return 0 ;
> > >  
> > >  	list_for_each_entry(bp, list, b_node.cn_mru) {
> > > -		if (bp->b_flags & LIBXFS_B_DIRTY)
> > > +		if (bp->b_flags & LIBXFS_B_DIRTY) {
> > >  			fprintf(stderr,
> > >  				"releasing dirty buffer (bulk) to free list!\n");
> > > +			bp->b_target->lost_writes = true;
> > > +		}
> > >  		count++;
> > >  	}
> > >  
> > > @@ -1479,6 +1484,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.
> > >   *
> > > 
> > 
> 

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Brian Foster]

On Thu, Feb 20, 2020 at 08:58:40AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 09:06:23AM -0500, Brian Foster wrote:
> > On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Add a new function that will ensure that everything we scribbled on has
> > > landed on stable media, and report the results.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  db/init.c |   14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > 
> > > diff --git a/db/init.c b/db/init.c
> > > index 0ac37368..e92de232 100644
> > > --- a/db/init.c
> > > +++ b/db/init.c
> > > @@ -184,6 +184,7 @@ main(
> > >  	char	*input;
> > >  	char	**v;
> > >  	int	start_iocur_sp;
> > > +	int	d, l, r;
> > >  
> > >  	init(argc, argv);
> > >  	start_iocur_sp = iocur_sp;
> > > @@ -216,6 +217,19 @@ main(
> > >  	 */
> > >  	while (iocur_sp > start_iocur_sp)
> > >  		pop_cur();
> > > +
> > > +	libxfs_flush_devices(mp, &d, &l, &r);
> > > +	if (d)
> > > +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> > > +				progname, d);
> > > +	if (l)
> > > +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> > > +				progname, l);
> > > +	if (r)
> > > +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> > > +				progname, r);
> > > +
> > > +
> > 
> > Seems like we could reduce some boilerplate by passing progname into
> > libxfs_flush_devices() and letting it dump out of the error messages,
> > unless there's some future code that cares about individual device error
> > state.
> 
> Such a program could call libxfs_flush_devices directly, as we do here.
> 

Right.. but does anything actually care about that level of granularity
right now beyond having a nicer error message?

> Also, progname is defined in libxfs so we don't even need to pass it as
> an argument.
> 

Ok.

> I had originally thought that we should try not to add fprintf calls to
> libxfs because libraries aren't really supposed to be doing things like
> that, but perhaps you're right that all of this should be melded into
> something else.
> 

Yeah, fair point, though I guess it depends on the particular library. 

> > That said, it also seems the semantics of libxfs_flush_devices() are a
> > bit different from convention. Just below we invoke
> > libxfs_device_close() for each device (rather than for all three), and
> > device_close() also happens to call fsync() and platform_flush_device()
> > itself...
> 
> Yeah, the division of responsibilities is a little hazy here -- I would
> think that unmounting a filesystem should flush all the memory caches
> and then the disk cache, but OTOH it's the utility that opens the
> devices and should therefore flush and close them.
> 
> I dunno.  My current thinking is that libxfs_umount should call
> libxfs_flush_devices() and print error messages as necessary, and return
> error codes as appropriate.  xfs_repair can then check the umount return
> value and translate that into exit(1) as required.  The device_close
> functions will fsync a second time, but that shouldn't be a big deal
> because we haven't dirtied anything in the meantime.
> 
> Thoughts?
> 

I was thinking of having a per-device libxfs_device_flush() along the
lines of libxfs_device_close() and separating out that functionality,
but one could argue we're also a bit inconsistent between libxfs_init()
opening the devices and having to close them individually. I think
having libxfs_umount() do a proper purge -> flush and returning any
errors instead is a fair tradeoff for simplicity. Removing the
flush_devices() API also eliminates risk of somebody incorrectly
attempting the flush after the umount frees the buftarg structures
(without reinitializing pointers :P).

Brian

> --D
> 
> > Brian
> > 
> > >  	libxfs_umount(mp);
> > >  	if (x.ddev)
> > >  		libxfs_device_close(x.ddev);
> > > 
> > 
> 

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 12:58:50PM -0500, Brian Foster wrote:
> On Thu, Feb 20, 2020 at 08:46:38AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 20, 2020 at 09:06:12AM -0500, Brian Foster wrote:
> > > On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Add a new function that will ensure that everything we changed has
> > > > landed on stable media, and report the results.  Subsequent commits will
> > > > teach the individual programs to report when things go wrong.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  include/xfs_mount.h |    3 +++
> > > >  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
> > > >  libxfs/libxfs_io.h  |    2 ++
> > > >  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
> > > >  4 files changed, 73 insertions(+), 2 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > > > index 29b3cc1b..c80aaf69 100644
> > > > --- a/include/xfs_mount.h
> > > > +++ b/include/xfs_mount.h
> > > > @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> > > >  extern void	libxfs_umount (xfs_mount_t *);
> > > >  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
> > > >  
> > > > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > > > +		int *rtdev);
> > > > +
> > > >  #endif	/* __XFS_MOUNT_H__ */
> > > > diff --git a/libxfs/init.c b/libxfs/init.c
> > > > index a0d4b7f4..d1d3f4df 100644
> > > > --- a/libxfs/init.c
> > > > +++ b/libxfs/init.c
> > > > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> > > >  	}
> > > >  	btp->bt_mount = mp;
> > > >  	btp->dev = dev;
> > > > +	btp->lost_writes = false;
> > > > +
> > > >  	return btp;
> > > >  }
> > > >  
> > > > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> > > >  	mp->m_rsumip = mp->m_rbmip = NULL;
> > > >  }
> > > >  
> > > > +static inline int
> > > > +libxfs_flush_buftarg(
> > > > +	struct xfs_buftarg	*btp)
> > > > +{
> > > > +	if (btp->lost_writes)
> > > > +		return -ENOTRECOVERABLE;
> > > 
> > > I'm curious why we'd want to skip the flush just because some writes
> > > happened to fail..? I suppose the fs might be borked, but it seems a
> > > little strange to at least not try the flush, particularly since we
> > > might still flush any of the other two possible devices.
> > 
> > My thinking here was that if the write verifiers (or the pwrite() calls
> > themselves) failed then there's no point in telling the disk to flush
> > its write cache since we already know it's not in sync with the buffer
> > cache.
> > 
> 
> I suppose, but it seems there is some value in flushing what we did
> write.. That's effectively historical behavior (since we ignored
> errors), right?

It's the historical behavior, yes.  I don't think it makes much sense,
but OTOH I'm not opposed to restoring that.

> > > > +
> > > > +	return libxfs_blkdev_issue_flush(btp);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > > > + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> > > > + * flag to be set in the buftarg.  If there were no lost writes, flush the
> > > > + * device to make sure the writes made it to stable storage.
> > > > + *
> > > > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > > > + * couldn't write something to disk; or the results of the block device flush
> > > > + * operation.
> > > 
> > > Why not -EIO?
> > 
> > Originally I thought it might be useful to be able to distinguish
> > between "dirty buffers never even made it out of the buffer cache" vs.
> > "dirty buffers were sent to the disk but the disk sent back media
> > errors", though in the end the userspace tools don't make any
> > distinction.
> > 
> > That said, looking at this again, maybe I should track write verifier
> > failure separately so that we can return EFSCORRUPTED for that?
> > 
> 
> It's not clear to me that anything application level would care much
> about verifier failure vs. I/O failure, but I've no objection to doing
> something like that either.

Yeah.  The single usecase I can think of is where repair trips over a
write verifier and we should make it really obvious to the sysadmin that
repair is buggy and needs either (a) an upgrade or (b) a complaint filed
on linux-xfs.

--D

> Brian
> 
> > --D
> > 
> > > 
> > > Brian
> > > 
> > > > + */
> > > > +void
> > > > +libxfs_flush_devices(
> > > > +	struct xfs_mount	*mp,
> > > > +	int			*datadev,
> > > > +	int			*logdev,
> > > > +	int			*rtdev)
> > > > +{
> > > > +	*datadev = *logdev = *rtdev = 0;
> > > > +
> > > > +	libxfs_bcache_purge();
> > > > +
> > > > +	if (mp->m_ddev_targp)
> > > > +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > > > +
> > > > +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > > > +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > > > +
> > > > +	if (mp->m_rtdev_targp)
> > > > +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > > > +}
> > > > +
> > > >  /*
> > > >   * Release any resource obtained during a mount.
> > > >   */
> > > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > > > index 579df52b..fc0fd060 100644
> > > > --- a/libxfs/libxfs_io.h
> > > > +++ b/libxfs/libxfs_io.h
> > > > @@ -23,10 +23,12 @@ struct xfs_perag;
> > > >  struct xfs_buftarg {
> > > >  	struct xfs_mount	*bt_mount;
> > > >  	dev_t			dev;
> > > > +	bool			lost_writes;
> > > >  };
> > > >  
> > > >  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..92e497f9 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 */
> > > >  
> > > > @@ -1227,9 +1228,11 @@ libxfs_brelse(
> > > >  
> > > >  	if (!bp)
> > > >  		return;
> > > > -	if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > +	if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > >  		fprintf(stderr,
> > > >  			"releasing dirty buffer to free list!\n");
> > > > +		bp->b_target->lost_writes = true;
> > > > +	}
> > > >  
> > > >  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
> > > >  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> > > > @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
> > > >  		return 0 ;
> > > >  
> > > >  	list_for_each_entry(bp, list, b_node.cn_mru) {
> > > > -		if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > +		if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > >  			fprintf(stderr,
> > > >  				"releasing dirty buffer (bulk) to free list!\n");
> > > > +			bp->b_target->lost_writes = true;
> > > > +		}
> > > >  		count++;
> > > >  	}
> > > >  
> > > > @@ -1479,6 +1484,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.
> > > >   *
> > > > 
> > > 
> > 
> 

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 12:58:57PM -0500, Brian Foster wrote:
> On Thu, Feb 20, 2020 at 08:58:40AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 20, 2020 at 09:06:23AM -0500, Brian Foster wrote:
> > > On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Add a new function that will ensure that everything we scribbled on has
> > > > landed on stable media, and report the results.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  db/init.c |   14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/db/init.c b/db/init.c
> > > > index 0ac37368..e92de232 100644
> > > > --- a/db/init.c
> > > > +++ b/db/init.c
> > > > @@ -184,6 +184,7 @@ main(
> > > >  	char	*input;
> > > >  	char	**v;
> > > >  	int	start_iocur_sp;
> > > > +	int	d, l, r;
> > > >  
> > > >  	init(argc, argv);
> > > >  	start_iocur_sp = iocur_sp;
> > > > @@ -216,6 +217,19 @@ main(
> > > >  	 */
> > > >  	while (iocur_sp > start_iocur_sp)
> > > >  		pop_cur();
> > > > +
> > > > +	libxfs_flush_devices(mp, &d, &l, &r);
> > > > +	if (d)
> > > > +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> > > > +				progname, d);
> > > > +	if (l)
> > > > +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> > > > +				progname, l);
> > > > +	if (r)
> > > > +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> > > > +				progname, r);
> > > > +
> > > > +
> > > 
> > > Seems like we could reduce some boilerplate by passing progname into
> > > libxfs_flush_devices() and letting it dump out of the error messages,
> > > unless there's some future code that cares about individual device error
> > > state.
> > 
> > Such a program could call libxfs_flush_devices directly, as we do here.
> > 
> 
> Right.. but does anything actually care about that level of granularity
> right now beyond having a nicer error message?

No, afaict.

> > Also, progname is defined in libxfs so we don't even need to pass it as
> > an argument.
> > 
> 
> Ok.
> 
> > I had originally thought that we should try not to add fprintf calls to
> > libxfs because libraries aren't really supposed to be doing things like
> > that, but perhaps you're right that all of this should be melded into
> > something else.
> > 
> 
> Yeah, fair point, though I guess it depends on the particular library. 

I mean... is libxfs even a real library? :)

> > > That said, it also seems the semantics of libxfs_flush_devices() are a
> > > bit different from convention. Just below we invoke
> > > libxfs_device_close() for each device (rather than for all three), and
> > > device_close() also happens to call fsync() and platform_flush_device()
> > > itself...
> > 
> > Yeah, the division of responsibilities is a little hazy here -- I would
> > think that unmounting a filesystem should flush all the memory caches
> > and then the disk cache, but OTOH it's the utility that opens the
> > devices and should therefore flush and close them.
> > 
> > I dunno.  My current thinking is that libxfs_umount should call
> > libxfs_flush_devices() and print error messages as necessary, and return
> > error codes as appropriate.  xfs_repair can then check the umount return
> > value and translate that into exit(1) as required.  The device_close
> > functions will fsync a second time, but that shouldn't be a big deal
> > because we haven't dirtied anything in the meantime.
> > 
> > Thoughts?
> > 
> 
> I was thinking of having a per-device libxfs_device_flush() along the
> lines of libxfs_device_close() and separating out that functionality,
> but one could argue we're also a bit inconsistent between libxfs_init()
> opening the devices and having to close them individually.

Yeah, I don't understand why libxfs_destroy doesn't empty out the same
struct libxfs_init that libxfs_init populates.  Or why we have a global
variable named "x", or why the buffer cache is a global variable.
However, those sound like refactoring for another series.

> I think
> having libxfs_umount() do a proper purge -> flush and returning any
> errors instead is a fair tradeoff for simplicity. Removing the
> flush_devices() API also eliminates risk of somebody incorrectly
> attempting the flush after the umount frees the buftarg structures
> (without reinitializing pointers :P).

Ok, I'll add a separate patch to null out the xfs_mount so that any
further use (afaict there aren't any) will crash immediately on reuse.

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > >  	libxfs_umount(mp);
> > > >  	if (x.ddev)
> > > >  		libxfs_device_close(x.ddev);
> > > > 
> > > 
> > 
> 

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Brian Foster]

On Thu, Feb 20, 2020 at 10:26:42AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 12:58:50PM -0500, Brian Foster wrote:
> > On Thu, Feb 20, 2020 at 08:46:38AM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 20, 2020 at 09:06:12AM -0500, Brian Foster wrote:
> > > > On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > Add a new function that will ensure that everything we changed has
> > > > > landed on stable media, and report the results.  Subsequent commits will
> > > > > teach the individual programs to report when things go wrong.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > >  include/xfs_mount.h |    3 +++
> > > > >  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
> > > > >  libxfs/libxfs_io.h  |    2 ++
> > > > >  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
> > > > >  4 files changed, 73 insertions(+), 2 deletions(-)
> > > > > 
> > > > > 
> > > > > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > > > > index 29b3cc1b..c80aaf69 100644
> > > > > --- a/include/xfs_mount.h
> > > > > +++ b/include/xfs_mount.h
> > > > > @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> > > > >  extern void	libxfs_umount (xfs_mount_t *);
> > > > >  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
> > > > >  
> > > > > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > > > > +		int *rtdev);
> > > > > +
> > > > >  #endif	/* __XFS_MOUNT_H__ */
> > > > > diff --git a/libxfs/init.c b/libxfs/init.c
> > > > > index a0d4b7f4..d1d3f4df 100644
> > > > > --- a/libxfs/init.c
> > > > > +++ b/libxfs/init.c
> > > > > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> > > > >  	}
> > > > >  	btp->bt_mount = mp;
> > > > >  	btp->dev = dev;
> > > > > +	btp->lost_writes = false;
> > > > > +
> > > > >  	return btp;
> > > > >  }
> > > > >  
> > > > > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> > > > >  	mp->m_rsumip = mp->m_rbmip = NULL;
> > > > >  }
> > > > >  
> > > > > +static inline int
> > > > > +libxfs_flush_buftarg(
> > > > > +	struct xfs_buftarg	*btp)
> > > > > +{
> > > > > +	if (btp->lost_writes)
> > > > > +		return -ENOTRECOVERABLE;
> > > > 
> > > > I'm curious why we'd want to skip the flush just because some writes
> > > > happened to fail..? I suppose the fs might be borked, but it seems a
> > > > little strange to at least not try the flush, particularly since we
> > > > might still flush any of the other two possible devices.
> > > 
> > > My thinking here was that if the write verifiers (or the pwrite() calls
> > > themselves) failed then there's no point in telling the disk to flush
> > > its write cache since we already know it's not in sync with the buffer
> > > cache.
> > > 
> > 
> > I suppose, but it seems there is some value in flushing what we did
> > write.. That's effectively historical behavior (since we ignored
> > errors), right?
> 
> It's the historical behavior, yes.  I don't think it makes much sense,
> but OTOH I'm not opposed to restoring that.
> 

The way I think about it is if repair or something happens to rewrite a
bunch of metadata structures, etc. and then a particular I/O happens to
fail, we'll still end up with a corrupted fs in the end, but I don't see
that as a reason not to care about the integrity of the data that might
have been successfully written. We're most likely borked either way,
this just seems a bit less risky (and also less of a wart/landmine given
that the close codepath is still going to do the flush anyways).

> > > > > +
> > > > > +	return libxfs_blkdev_issue_flush(btp);
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > > > > + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> > > > > + * flag to be set in the buftarg.  If there were no lost writes, flush the
> > > > > + * device to make sure the writes made it to stable storage.
> > > > > + *
> > > > > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > > > > + * couldn't write something to disk; or the results of the block device flush
> > > > > + * operation.
> > > > 
> > > > Why not -EIO?
> > > 
> > > Originally I thought it might be useful to be able to distinguish
> > > between "dirty buffers never even made it out of the buffer cache" vs.
> > > "dirty buffers were sent to the disk but the disk sent back media
> > > errors", though in the end the userspace tools don't make any
> > > distinction.
> > > 
> > > That said, looking at this again, maybe I should track write verifier
> > > failure separately so that we can return EFSCORRUPTED for that?
> > > 
> > 
> > It's not clear to me that anything application level would care much
> > about verifier failure vs. I/O failure, but I've no objection to doing
> > something like that either.
> 
> Yeah.  The single usecase I can think of is where repair trips over a
> write verifier and we should make it really obvious to the sysadmin that
> repair is buggy and needs either (a) an upgrade or (b) a complaint filed
> on linux-xfs.
> 

We do have the write verifier failure messages, but yeah, I can see that
being a more accurate distinction between -EIO and -EFSCORRUPTED.

Brian

> --D
> 
> > Brian
> > 
> > > --D
> > > 
> > > > 
> > > > Brian
> > > > 
> > > > > + */
> > > > > +void
> > > > > +libxfs_flush_devices(
> > > > > +	struct xfs_mount	*mp,
> > > > > +	int			*datadev,
> > > > > +	int			*logdev,
> > > > > +	int			*rtdev)
> > > > > +{
> > > > > +	*datadev = *logdev = *rtdev = 0;
> > > > > +
> > > > > +	libxfs_bcache_purge();
> > > > > +
> > > > > +	if (mp->m_ddev_targp)
> > > > > +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > > > > +
> > > > > +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > > > > +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > > > > +
> > > > > +	if (mp->m_rtdev_targp)
> > > > > +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Release any resource obtained during a mount.
> > > > >   */
> > > > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > > > > index 579df52b..fc0fd060 100644
> > > > > --- a/libxfs/libxfs_io.h
> > > > > +++ b/libxfs/libxfs_io.h
> > > > > @@ -23,10 +23,12 @@ struct xfs_perag;
> > > > >  struct xfs_buftarg {
> > > > >  	struct xfs_mount	*bt_mount;
> > > > >  	dev_t			dev;
> > > > > +	bool			lost_writes;
> > > > >  };
> > > > >  
> > > > >  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..92e497f9 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 */
> > > > >  
> > > > > @@ -1227,9 +1228,11 @@ libxfs_brelse(
> > > > >  
> > > > >  	if (!bp)
> > > > >  		return;
> > > > > -	if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > > +	if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > > >  		fprintf(stderr,
> > > > >  			"releasing dirty buffer to free list!\n");
> > > > > +		bp->b_target->lost_writes = true;
> > > > > +	}
> > > > >  
> > > > >  	pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
> > > > >  	list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> > > > > @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
> > > > >  		return 0 ;
> > > > >  
> > > > >  	list_for_each_entry(bp, list, b_node.cn_mru) {
> > > > > -		if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > > +		if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > > >  			fprintf(stderr,
> > > > >  				"releasing dirty buffer (bulk) to free list!\n");
> > > > > +			bp->b_target->lost_writes = true;
> > > > > +		}
> > > > >  		count++;
> > > > >  	}
> > > > >  
> > > > > @@ -1479,6 +1484,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.
> > > > >   *
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Dave Chinner]

On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a new function that will ensure that everything we changed has
> landed on stable media, and report the results.  Subsequent commits will
> teach the individual programs to report when things go wrong.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  include/xfs_mount.h |    3 +++
>  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
>  libxfs/libxfs_io.h  |    2 ++
>  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
>  4 files changed, 73 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> index 29b3cc1b..c80aaf69 100644
> --- a/include/xfs_mount.h
> +++ b/include/xfs_mount.h
> @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
>  extern void	libxfs_umount (xfs_mount_t *);
>  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
>  
> +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> +		int *rtdev);
> +
>  #endif	/* __XFS_MOUNT_H__ */
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a0d4b7f4..d1d3f4df 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
>  	}
>  	btp->bt_mount = mp;
>  	btp->dev = dev;
> +	btp->lost_writes = false;
> +
>  	return btp;
>  }
>  
> @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
>  	mp->m_rsumip = mp->m_rbmip = NULL;
>  }
>  
> +static inline int
> +libxfs_flush_buftarg(
> +	struct xfs_buftarg	*btp)
> +{
> +	if (btp->lost_writes)
> +		return -ENOTRECOVERABLE;
> +
> +	return libxfs_blkdev_issue_flush(btp);
> +}
> +
> +/*
> + * Purge the buffer cache to write all dirty buffers to disk and free all
> + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> + * flag to be set in the buftarg.  If there were no lost writes, flush the
> + * device to make sure the writes made it to stable storage.
> + *
> + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> + * couldn't write something to disk; or the results of the block device flush
> + * operation.
> + */
> +void
> +libxfs_flush_devices(
> +	struct xfs_mount	*mp,
> +	int			*datadev,
> +	int			*logdev,
> +	int			*rtdev)
> +{
> +	*datadev = *logdev = *rtdev = 0;
> +
> +	libxfs_bcache_purge();
> +
> +	if (mp->m_ddev_targp)
> +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> +
> +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> +
> +	if (mp->m_rtdev_targp)
> +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> +}

So one of the things I'm trying to do is make this use similar code
to the kernel. basically this close process is equivalent of
xfs_wait_buftarg() which waits for all references to buffers to
got away, and if any buffer it tagged with WRITE_FAIL then it issues
and alert before it frees the buffer.

IOWs, any sort of delayed/async write failure that hasn't been
otherwise caught by the async/delwri code is caught by the device
close code....

Doing things like this (storing a "lost writes" flag on the buftarg)
I think is just going to make it harder to switch to kernel
equivalent code because it just introduces even more of an impedance
mismatch between userspace and kernel code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Dave Chinner]

On Thu, Feb 20, 2020 at 10:34:50AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 12:58:57PM -0500, Brian Foster wrote:
> > On Thu, Feb 20, 2020 at 08:58:40AM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 20, 2020 at 09:06:23AM -0500, Brian Foster wrote:
> > > > On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > Add a new function that will ensure that everything we scribbled on has
> > > > > landed on stable media, and report the results.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > >  db/init.c |   14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > 
> > > > > diff --git a/db/init.c b/db/init.c
> > > > > index 0ac37368..e92de232 100644
> > > > > --- a/db/init.c
> > > > > +++ b/db/init.c
> > > > > @@ -184,6 +184,7 @@ main(
> > > > >  	char	*input;
> > > > >  	char	**v;
> > > > >  	int	start_iocur_sp;
> > > > > +	int	d, l, r;
> > > > >  
> > > > >  	init(argc, argv);
> > > > >  	start_iocur_sp = iocur_sp;
> > > > > @@ -216,6 +217,19 @@ main(
> > > > >  	 */
> > > > >  	while (iocur_sp > start_iocur_sp)
> > > > >  		pop_cur();
> > > > > +
> > > > > +	libxfs_flush_devices(mp, &d, &l, &r);
> > > > > +	if (d)
> > > > > +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> > > > > +				progname, d);
> > > > > +	if (l)
> > > > > +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> > > > > +				progname, l);
> > > > > +	if (r)
> > > > > +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> > > > > +				progname, r);
> > > > > +
> > > > > +
> > > > 
> > > > Seems like we could reduce some boilerplate by passing progname into
> > > > libxfs_flush_devices() and letting it dump out of the error messages,
> > > > unless there's some future code that cares about individual device error
> > > > state.
> > > 
> > > Such a program could call libxfs_flush_devices directly, as we do here.
> > > 
> > 
> > Right.. but does anything actually care about that level of granularity
> > right now beyond having a nicer error message?
> 
> No, afaict.
> 
> > > Also, progname is defined in libxfs so we don't even need to pass it as
> > > an argument.
> > > 
> > 
> > Ok.
> > 
> > > I had originally thought that we should try not to add fprintf calls to
> > > libxfs because libraries aren't really supposed to be doing things like
> > > that, but perhaps you're right that all of this should be melded into
> > > something else.
> > > 
> > 
> > Yeah, fair point, though I guess it depends on the particular library. 
> 
> I mean... is libxfs even a real library? :)

It's an internal abstraction to allow code to be shared easily with
the kernel and between xfsprogs binaries. It is not a library in the
sense it has a fixed API and ABI that compatibility has to be
maintained across releases (i.e. like, say, libhandle). It is a
library in the sense is contains shared code that things within the
build don't have to re-implement them over and over again, and
because it is internal that means the rules for external/distro
level libraries don't need to be strictly applied.

e.g. if -everything- uses a global variable and has to declares it
themselves so the shared internal code can access it, then why not
just declare it in the shared code? :)

> > > I dunno.  My current thinking is that libxfs_umount should call
> > > libxfs_flush_devices() and print error messages as necessary, and return
> > > error codes as appropriate.  xfs_repair can then check the umount return
> > > value and translate that into exit(1) as required.  The device_close
> > > functions will fsync a second time, but that shouldn't be a big deal
> > > because we haven't dirtied anything in the meantime.
> > > 
> > > Thoughts?
> > > 
> > 
> > I was thinking of having a per-device libxfs_device_flush() along the
> > lines of libxfs_device_close() and separating out that functionality,
> > but one could argue we're also a bit inconsistent between libxfs_init()
> > opening the devices and having to close them individually.
> 
> Yeah, I don't understand why libxfs_destroy doesn't empty out the same
> struct libxfs_init that libxfs_init populates.  Or why we have a global
> variable named "x", or why the buffer cache is a global variable.
> However, those sound like refactoring for another series.

You mean structure the unmount code like we do in teh kernel? e.g:

> > I think
> > having libxfs_umount() do a proper purge -> flush and returning any
> > errors instead is a fair tradeoff for simplicity. Removing the
> > flush_devices() API also eliminates risk of somebody incorrectly
> > attempting the flush after the umount frees the buftarg structures
> > (without reinitializing pointers :P).

You mean like this code that I'm slowly working on to bring the
xfs_buf.c code across to userspace and get rid of the heap of crap
we have in libxfs/{rdwr,cache}.c now and be able to use AIO properly
and do non-synchronous delayed writes like we do in the kernel?

libxfs/init.c:
....
static void
buftarg_cleanup(
        struct xfs_buftarg      *btp)
{
        if (!btp)
                return;

        while (btp->bt_lru.l_count > 0)
                xfs_buftarg_shrink(btp, 1000);
        xfs_buftarg_wait(btp);
        xfs_buftarg_free(btp);
}

/*
 * Release any resource obtained during a mount.
 */
void
libxfs_umount(
        struct xfs_mount        *mp)
{
        struct xfs_perag        *pag;
        int                     agno;

        libxfs_rtmount_destroy(mp);

        buftarg_cleanup(mp->m_ddev_targp);
        buftarg_cleanup(mp->m_rtdev_targp);
        if (mp->m_logdev_targp != mp->m_ddev_targp)
                buftarg_cleanup(mp->m_logdev_targp);
.....

libxfs/xfs_buftarg.c:
.....
void
xfs_buftarg_free(
        struct xfs_buftarg      *btp)
{
        ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
        percpu_counter_destroy(&btp->bt_io_count);
        platform_flush_device(btp->bt_fd, btp->bt_bdev);
	libxfs_device_close(btp->bt_bdev);
        free(btp);
}

I haven't added the error returns for this code yet - I'm still
doing the conversion and making it work.

I'll probably have to throw the vast majority of that patchset away
and start again if all this API change that darrick has done is
merged. And that will probably involve me throwing away all of the
changes in this patch series because they just don't make any sense
once the code is restructured properly....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed

[Darrick J. Wong]

On Fri, Feb 21, 2020 at 10:40:55AM +1100, Dave Chinner wrote:
> On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a new function that will ensure that everything we changed has
> > landed on stable media, and report the results.  Subsequent commits will
> > teach the individual programs to report when things go wrong.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/xfs_mount.h |    3 +++
> >  libxfs/init.c       |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  libxfs/libxfs_io.h  |    2 ++
> >  libxfs/rdwr.c       |   27 +++++++++++++++++++++++++--
> >  4 files changed, 73 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > index 29b3cc1b..c80aaf69 100644
> > --- a/include/xfs_mount.h
> > +++ b/include/xfs_mount.h
> > @@ -187,4 +187,7 @@ extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> >  extern void	libxfs_umount (xfs_mount_t *);
> >  extern void	libxfs_rtmount_destroy (xfs_mount_t *);
> >  
> > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > +		int *rtdev);
> > +
> >  #endif	/* __XFS_MOUNT_H__ */
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index a0d4b7f4..d1d3f4df 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> >  	}
> >  	btp->bt_mount = mp;
> >  	btp->dev = dev;
> > +	btp->lost_writes = false;
> > +
> >  	return btp;
> >  }
> >  
> > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> >  	mp->m_rsumip = mp->m_rbmip = NULL;
> >  }
> >  
> > +static inline int
> > +libxfs_flush_buftarg(
> > +	struct xfs_buftarg	*btp)
> > +{
> > +	if (btp->lost_writes)
> > +		return -ENOTRECOVERABLE;
> > +
> > +	return libxfs_blkdev_issue_flush(btp);
> > +}
> > +
> > +/*
> > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > + * incore buffers.  Buffers that cannot be written will cause the lost_writes
> > + * flag to be set in the buftarg.  If there were no lost writes, flush the
> > + * device to make sure the writes made it to stable storage.
> > + *
> > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > + * couldn't write something to disk; or the results of the block device flush
> > + * operation.
> > + */
> > +void
> > +libxfs_flush_devices(
> > +	struct xfs_mount	*mp,
> > +	int			*datadev,
> > +	int			*logdev,
> > +	int			*rtdev)
> > +{
> > +	*datadev = *logdev = *rtdev = 0;
> > +
> > +	libxfs_bcache_purge();
> > +
> > +	if (mp->m_ddev_targp)
> > +		*datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > +
> > +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > +		*logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > +
> > +	if (mp->m_rtdev_targp)
> > +		*rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > +}
> 
> So one of the things I'm trying to do is make this use similar code
> to the kernel. basically this close process is equivalent of
> xfs_wait_buftarg() which waits for all references to buffers to
> got away, and if any buffer it tagged with WRITE_FAIL then it issues
> and alert before it frees the buffer.
> 
> IOWs, any sort of delayed/async write failure that hasn't been
> otherwise caught by the async/delwri code is caught by the device
> close code....
> 
> Doing things like this (storing a "lost writes" flag on the buftarg)
> I think is just going to make it harder to switch to kernel
> equivalent code because it just introduces even more of an impedance
> mismatch between userspace and kernel code...

Hmm.  In today's version of the code I've taken Brian's suggestions and
hidden all of the flushing and whinging details inside libxfs_umount.
This eliminates all of the external API and dependency hell (except that
_umount now returns The Usual Errorcode) which will make it easier(?) to
rip all of that out some day when we're ready to land your libaio port.
Maybe?

In the meantime, I really /do/ need to solve the user complaints about
xfs_repair spewing evidence on stderr that it hasn't fully fixed the fs
and yet exits with 0 status.

It would be much easier for me to design my patchsets to minimize your
rebasing headaches if you patchbombed your development tree on a
semi-regular basis like I do, if nothing else so that we can all see
what could be in the pipeline. :)

(I dunno, maybe we need a separate git backup^W^Wpatchbomb list. :P)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Darrick J. Wong]

On Fri, Feb 21, 2020 at 11:01:19AM +1100, Dave Chinner wrote:
> On Thu, Feb 20, 2020 at 10:34:50AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 20, 2020 at 12:58:57PM -0500, Brian Foster wrote:
> > > On Thu, Feb 20, 2020 at 08:58:40AM -0800, Darrick J. Wong wrote:
> > > > On Thu, Feb 20, 2020 at 09:06:23AM -0500, Brian Foster wrote:
> > > > > On Wed, Feb 19, 2020 at 05:42:13PM -0800, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > 
> > > > > > Add a new function that will ensure that everything we scribbled on has
> > > > > > landed on stable media, and report the results.
> > > > > > 
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > >  db/init.c |   14 ++++++++++++++
> > > > > >  1 file changed, 14 insertions(+)
> > > > > > 
> > > > > > 
> > > > > > diff --git a/db/init.c b/db/init.c
> > > > > > index 0ac37368..e92de232 100644
> > > > > > --- a/db/init.c
> > > > > > +++ b/db/init.c
> > > > > > @@ -184,6 +184,7 @@ main(
> > > > > >  	char	*input;
> > > > > >  	char	**v;
> > > > > >  	int	start_iocur_sp;
> > > > > > +	int	d, l, r;
> > > > > >  
> > > > > >  	init(argc, argv);
> > > > > >  	start_iocur_sp = iocur_sp;
> > > > > > @@ -216,6 +217,19 @@ main(
> > > > > >  	 */
> > > > > >  	while (iocur_sp > start_iocur_sp)
> > > > > >  		pop_cur();
> > > > > > +
> > > > > > +	libxfs_flush_devices(mp, &d, &l, &r);
> > > > > > +	if (d)
> > > > > > +		fprintf(stderr, _("%s: cannot flush data device (%d).\n"),
> > > > > > +				progname, d);
> > > > > > +	if (l)
> > > > > > +		fprintf(stderr, _("%s: cannot flush log device (%d).\n"),
> > > > > > +				progname, l);
> > > > > > +	if (r)
> > > > > > +		fprintf(stderr, _("%s: cannot flush realtime device (%d).\n"),
> > > > > > +				progname, r);
> > > > > > +
> > > > > > +
> > > > > 
> > > > > Seems like we could reduce some boilerplate by passing progname into
> > > > > libxfs_flush_devices() and letting it dump out of the error messages,
> > > > > unless there's some future code that cares about individual device error
> > > > > state.
> > > > 
> > > > Such a program could call libxfs_flush_devices directly, as we do here.
> > > > 
> > > 
> > > Right.. but does anything actually care about that level of granularity
> > > right now beyond having a nicer error message?
> > 
> > No, afaict.
> > 
> > > > Also, progname is defined in libxfs so we don't even need to pass it as
> > > > an argument.
> > > > 
> > > 
> > > Ok.
> > > 
> > > > I had originally thought that we should try not to add fprintf calls to
> > > > libxfs because libraries aren't really supposed to be doing things like
> > > > that, but perhaps you're right that all of this should be melded into
> > > > something else.
> > > > 
> > > 
> > > Yeah, fair point, though I guess it depends on the particular library. 
> > 
> > I mean... is libxfs even a real library? :)
> 
> It's an internal abstraction to allow code to be shared easily with
> the kernel and between xfsprogs binaries. It is not a library in the
> sense it has a fixed API and ABI that compatibility has to be
> maintained across releases (i.e. like, say, libhandle). It is a
> library in the sense is contains shared code that things within the
> build don't have to re-implement them over and over again, and
> because it is internal that means the rules for external/distro
> level libraries don't need to be strictly applied.
> 
> e.g. if -everything- uses a global variable and has to declares it
> themselves so the shared internal code can access it, then why not
> just declare it in the shared code? :)

Fair enough.  It's a sharedcode library.

> > > > I dunno.  My current thinking is that libxfs_umount should call
> > > > libxfs_flush_devices() and print error messages as necessary, and return
> > > > error codes as appropriate.  xfs_repair can then check the umount return
> > > > value and translate that into exit(1) as required.  The device_close
> > > > functions will fsync a second time, but that shouldn't be a big deal
> > > > because we haven't dirtied anything in the meantime.
> > > > 
> > > > Thoughts?
> > > > 
> > > 
> > > I was thinking of having a per-device libxfs_device_flush() along the
> > > lines of libxfs_device_close() and separating out that functionality,
> > > but one could argue we're also a bit inconsistent between libxfs_init()
> > > opening the devices and having to close them individually.
> > 
> > Yeah, I don't understand why libxfs_destroy doesn't empty out the same
> > struct libxfs_init that libxfs_init populates.  Or why we have a global
> > variable named "x", or why the buffer cache is a global variable.
> > However, those sound like refactoring for another series.
> 
> You mean structure the unmount code like we do in teh kernel? e.g:
> 
> > > I think
> > > having libxfs_umount() do a proper purge -> flush and returning any
> > > errors instead is a fair tradeoff for simplicity. Removing the
> > > flush_devices() API also eliminates risk of somebody incorrectly
> > > attempting the flush after the umount frees the buftarg structures
> > > (without reinitializing pointers :P).
> 
> You mean like this code that I'm slowly working on to bring the
> xfs_buf.c code across to userspace and get rid of the heap of crap
> we have in libxfs/{rdwr,cache}.c now and be able to use AIO properly
> and do non-synchronous delayed writes like we do in the kernel?
> 
> libxfs/init.c:
> ....
> static void
> buftarg_cleanup(
>         struct xfs_buftarg      *btp)
> {
>         if (!btp)
>                 return;
> 
>         while (btp->bt_lru.l_count > 0)
>                 xfs_buftarg_shrink(btp, 1000);
>         xfs_buftarg_wait(btp);
>         xfs_buftarg_free(btp);

Not quite what the v3 series does, but only because it's still stuck
with "whack the bcache and then go see what happened to each buftarg".

> }
> 
> /*
>  * Release any resource obtained during a mount.
>  */
> void
> libxfs_umount(
>         struct xfs_mount        *mp)
> {
>         struct xfs_perag        *pag;
>         int                     agno;
> 
>         libxfs_rtmount_destroy(mp);
> 
>         buftarg_cleanup(mp->m_ddev_targp);
>         buftarg_cleanup(mp->m_rtdev_targp);
>         if (mp->m_logdev_targp != mp->m_ddev_targp)
>                 buftarg_cleanup(mp->m_logdev_targp);

Yep, that's exactly where I moved the cleanup call in v3.

> .....
> 
> libxfs/xfs_buftarg.c:
> .....
> void
> xfs_buftarg_free(
>         struct xfs_buftarg      *btp)
> {
>         ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
>         percpu_counter_destroy(&btp->bt_io_count);
>         platform_flush_device(btp->bt_fd, btp->bt_bdev);
> 	libxfs_device_close(btp->bt_bdev);
>         free(btp);

I'm assuming this means you've killed off the buffer handling parts of
struct libxfs_xinit too?

> }
> 
> I haven't added the error returns for this code yet - I'm still
> doing the conversion and making it work.
> 
> I'll probably have to throw the vast majority of that patchset away
> and start again if all this API change that darrick has done is
> merged. And that will probably involve me throwing away all of the
> changes in this patch series because they just don't make any sense
> once the code is restructured properly....

...or just throw them at me in whatever state they're in now and let me
help figure out how to get there?

Everyone: don't be afraid of the 'RFCRAP' for interim patchsets.
Granted, posting git branches with a timestamp might be more
practicable...

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 5/8] xfs_db: check that metadata updates have been committed

[Dave Chinner]

On Thu, Feb 20, 2020 at 04:39:21PM -0800, Darrick J. Wong wrote:
> On Fri, Feb 21, 2020 at 11:01:19AM +1100, Dave Chinner wrote:
> > On Thu, Feb 20, 2020 at 10:34:50AM -0800, Darrick J. Wong wrote:
> > > > I think
> > > > having libxfs_umount() do a proper purge -> flush and returning any
> > > > errors instead is a fair tradeoff for simplicity. Removing the
> > > > flush_devices() API also eliminates risk of somebody incorrectly
> > > > attempting the flush after the umount frees the buftarg structures
> > > > (without reinitializing pointers :P).
> > 
> > You mean like this code that I'm slowly working on to bring the
> > xfs_buf.c code across to userspace and get rid of the heap of crap
> > we have in libxfs/{rdwr,cache}.c now and be able to use AIO properly
> > and do non-synchronous delayed writes like we do in the kernel?
> > 
> > libxfs/init.c:
> > ....
> > static void
> > buftarg_cleanup(
> >         struct xfs_buftarg      *btp)
> > {
> >         if (!btp)
> >                 return;
> > 
> >         while (btp->bt_lru.l_count > 0)
> >                 xfs_buftarg_shrink(btp, 1000);
> >         xfs_buftarg_wait(btp);
> >         xfs_buftarg_free(btp);
> 
> Not quite what the v3 series does, but only because it's still stuck
> with "whack the bcache and then go see what happened to each buftarg".

Right - I've completely reimplemented the caching and LRUs so that
global bcache thingy goes away.

> > }
> > 
> > /*
> >  * Release any resource obtained during a mount.
> >  */
> > void
> > libxfs_umount(
> >         struct xfs_mount        *mp)
> > {
> >         struct xfs_perag        *pag;
> >         int                     agno;
> > 
> >         libxfs_rtmount_destroy(mp);
> > 
> >         buftarg_cleanup(mp->m_ddev_targp);
> >         buftarg_cleanup(mp->m_rtdev_targp);
> >         if (mp->m_logdev_targp != mp->m_ddev_targp)
> >                 buftarg_cleanup(mp->m_logdev_targp);
> 
> Yep, that's exactly where I moved the cleanup call in v3.

OK, good :P

> > .....
> > 
> > libxfs/xfs_buftarg.c:
> > .....
> > void
> > xfs_buftarg_free(
> >         struct xfs_buftarg      *btp)
> > {
> >         ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
> >         percpu_counter_destroy(&btp->bt_io_count);
> >         platform_flush_device(btp->bt_fd, btp->bt_bdev);
> > 	libxfs_device_close(btp->bt_bdev);
> >         free(btp);
> 
> I'm assuming this means you've killed off the buffer handling parts of
> struct libxfs_xinit too?

Which bits are they? THe bcache init stuff is gone, yes, but right
now that function still does all the device opening and passing the
dev_ts to xfs_buftarg_alloc() for it to initialise similar to the
way the kernel initialises the buftarg.


> > I haven't added the error returns for this code yet - I'm still
> > doing the conversion and making it work.
> > 
> > I'll probably have to throw the vast majority of that patchset away
> > and start again if all this API change that darrick has done is
> > merged. And that will probably involve me throwing away all of the
> > changes in this patch series because they just don't make any sense
> > once the code is restructured properly....
> 
> ...or just throw them at me in whatever state they're in now and let me
> help figure out how to get there?
> 
> Everyone: don't be afraid of the 'RFCRAP' for interim patchsets.
> Granted, posting git branches with a timestamp might be more
> practicable...

I'm not afraid of them - I've got to at least get it to the compile
stage with all the infrastructure in place and that's been the hold
up. :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com