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

[Dave Chinner <david@fromorbit.com>]

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