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

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

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);
> > 
>