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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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