Re: [PATCH 2/6] libfrog: introduce xfs_fd to wrap an fd to a file on an xfs filesystem

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

On Tue, Aug 27, 2019 at 04:41:57PM +1000, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 01:30:44PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Introduce a new "xfs_fd" context structure where we can store a file
> > descriptor and all the runtime fs context (geometry, which ioctls work,
> > etc.) that goes with it.  We're going to create wrappers for the
> > bulkstat and inumbers ioctls in subsequent patches; and when we
> > introduce the v5 bulkstat/inumbers ioctls we'll need all that context to
> > downgrade gracefully on old kernels.  Start the transition by adopting
> > xfs_fd natively in scrub.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/xfrog.h    |   20 ++++++++++++++++++++
> 
> naming.
> 
> >  libfrog/fsgeom.c   |   25 +++++++++++++++++++++++++
> >  scrub/fscounters.c |   22 ++++++++++++----------
> >  scrub/inodes.c     |   10 +++++-----
> >  scrub/phase1.c     |   41 ++++++++++++++++++++---------------------
> >  scrub/phase2.c     |    2 +-
> >  scrub/phase3.c     |    4 ++--
> >  scrub/phase4.c     |    8 ++++----
> >  scrub/phase5.c     |    2 +-
> >  scrub/phase6.c     |    6 +++---
> >  scrub/phase7.c     |    2 +-
> >  scrub/repair.c     |    4 ++--
> >  scrub/scrub.c      |   12 ++++++------
> >  scrub/spacemap.c   |   12 ++++++------
> >  scrub/vfs.c        |    2 +-
> >  scrub/xfs_scrub.h  |    7 ++++---
> >  16 files changed, 113 insertions(+), 66 deletions(-)
> > 
> > 
> > diff --git a/include/xfrog.h b/include/xfrog.h
> > index 5420b47c..f3808911 100644
> > --- a/include/xfrog.h
> > +++ b/include/xfrog.h
> > @@ -19,4 +19,24 @@
> >  struct xfs_fsop_geom;
> >  int xfrog_geometry(int fd, struct xfs_fsop_geom *fsgeo);
> >  
> > +/*
> > + * Structure for recording whatever observations we want about the level of
> > + * xfs runtime support for this fd.  Right now we only store the fd and fs
> > + * geometry.
> > + */
> > +struct xfs_fd {
> > +	/* ioctl file descriptor */
> > +	int			fd;
> > +
> > +	/* filesystem geometry */
> > +	struct xfs_fsop_geom	fsgeom;
> > +};
> > +
> > +/* Static initializers */
> > +#define XFS_FD_INIT(_fd)	{ .fd = (_fd), }
> > +#define XFS_FD_INIT_EMPTY	XFS_FD_INIT(-1)
> > +
> > +int xfrog_prepare_geometry(struct xfs_fd *xfd);
> > +int xfrog_close(struct xfs_fd *xfd);
> 
> I'd much prefer to see these named xfd_prepare_geometry() and
> xfd_close()...

Done.

> >  	ci = calloc(1, sizeof(struct xfs_count_inodes) +
> > -			(ctx->geo.agcount * sizeof(uint64_t)));
> > +			(ctx->mnt.fsgeom.agcount * sizeof(uint64_t)));
> 
> This gets a bit verbose, but.... <shrug> .... doesn't make that much
> of a mess...
> 
> Ok, apart from the naming thing, this looks ok.

Ok.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com