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

[Christoph Hellwig <hch@infradead.org>]

On Tue, Feb 25, 2020 at 07:14:27AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 25, 2020 at 10:08:17AM -0500, Brian Foster wrote:
> > On Mon, Feb 24, 2020 at 04:11:05PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Make sure that any metadata that we repaired or regenerated has been
> > > written to disk.  If that fails, exit with 1 to signal that there are
> > > still errors in the filesystem.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  repair/xfs_repair.c |    7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > index eb1ce546..ccb13f4a 100644
> > > --- a/repair/xfs_repair.c
> > > +++ b/repair/xfs_repair.c
> > > @@ -703,6 +703,7 @@ main(int argc, char **argv)
> > >  	struct xfs_sb	psb;
> > >  	int		rval;
> > >  	struct xfs_ino_geometry	*igeo;
> > > +	int		error;
> > >  
> > >  	progname = basename(argv[0]);
> > >  	setlocale(LC_ALL, "");
> > > @@ -1104,7 +1105,11 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
> > >  	 */
> > >  	libxfs_bcache_flush();
> > >  	format_log_max_lsn(mp);
> > > -	libxfs_umount(mp);
> > > +
> > > +	/* Report failure if anything failed to get written to our fs. */
> > > +	error = -libxfs_umount(mp);
> > > +	if (error)
> > > +		exit(1);
> > 
> > I wonder a bit whether repair should really exit like this vs. report
> > the error as it does for most others, but I could go either way. I'll
> > defer to Eric:
> 
> I suppose I could do:
> 
> 	error = -libxfs_umount();
> 	if (error)
> 		do_error(_("fs unmount failed (err=%d), re-run repair!\n"),
> 				error);
> 
> Though then you'd end up with:
> 
> 	# xfs_repair /dev/fd0
> 	...
> 	Refusing to write corrupted metadata to the data device!
> 	fs unmount failed (err=117), re-run repair!
> 	# echo $?
> 	1
> 
> Which seems a little redundant.  But let's see what Eric thinks.

I think this message would be at last somewhat useful.