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