Re: [PATCH 4/6] misc: convert to v5 bulkstat_single call

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

On Fri, Sep 13, 2019 at 11:02:37AM +1000, Dave Chinner wrote:
> On Thu, Sep 05, 2019 at 08:35:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  spaceman/health.c  |    4 +-
> >  7 files changed, 105 insertions(+), 32 deletions(-)
> > 
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index cc3cc93a..e8fa39ab 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -724,6 +724,7 @@ fsrfile(
> >  	xfs_ino_t		ino)
> >  {
> >  	struct xfs_fd		fsxfd = XFS_FD_INIT_EMPTY;
> > +	struct xfs_bulkstat	bulkstat;
> >  	struct xfs_bstat	statbuf;
> >  	jdm_fshandle_t		*fshandlep;
> >  	int			fd = -1;
> > @@ -748,12 +749,13 @@ fsrfile(
> >  		goto out;
> >  	}
> >  
> > -	error = xfrog_bulkstat_single(&fsxfd, ino, &statbuf);
> > +	error = xfrog_bulkstat_single(&fsxfd, ino, 0, &bulkstat);
> >  	if (error) {
> >  		fsrprintf(_("unable to get bstat on %s: %s\n"),
> >  			fname, strerror(error));
> >  		goto out;
> >  	}
> > +	xfrog_bulkstat_to_bstat(&fsxfd, &statbuf, &bulkstat);
> 
> So this is so none of the rest of fsr needs to be converted to use
> the new structure versions? When will this go away?
> 
> >  	do {
> > -		struct xfs_bstat tbstat;
> > +		struct xfs_bulkstat	tbstat;
> >  		char		name[64];
> >  		int		ret;
> >  
> > @@ -983,7 +985,7 @@ fsr_setup_attr_fork(
> >  		 * this to compare against the target and determine what we
> >  		 * need to do.
> >  		 */
> > -		ret = xfrog_bulkstat_single(&txfd, tstatbuf.st_ino, &tbstat);
> > +		ret = xfrog_bulkstat_single(&txfd, tstatbuf.st_ino, 0, &tbstat);
> >  		if (ret) {
> >  			fsrprintf(_("unable to get bstat on temp file: %s\n"),
> >  						strerror(ret));
> 
> Because this looks like we now have a combination of v1 and v5
> structures being used...
> 
> >  
> > diff --git a/io/swapext.c b/io/swapext.c
> > index 2b4918f8..ca024b93 100644
> > --- a/io/swapext.c
> > +++ b/io/swapext.c
> > @@ -28,6 +28,7 @@ swapext_f(
> >  	char			**argv)
> >  {
> >  	struct xfs_fd		fxfd = XFS_FD_INIT(file->fd);
> > +	struct xfs_bulkstat	bulkstat;
> >  	int			fd;
> >  	int			error;
> >  	struct xfs_swapext	sx;
> > @@ -48,12 +49,13 @@ swapext_f(
> >  		goto out;
> >  	}
> >  
> > -	error = xfrog_bulkstat_single(&fxfd, stat.st_ino, &sx.sx_stat);
> > +	error = xfrog_bulkstat_single(&fxfd, stat.st_ino, 0, &bulkstat);
> >  	if (error) {
> >  		errno = error;
> >  		perror("bulkstat");
> >  		goto out;
> >  	}
> > +	xfrog_bulkstat_to_bstat(&fxfd, &sx.sx_stat, &bulkstat);
> 
> and this is required because bstat is part of the swapext ioctl ABI?

Yes.  I think a lof of the retained bulkstat weirdness in fsr could go
away if (a) we maintained an open xfs_fd to the filesystem and (b)
wrapped the swapext ioctl... but there's already too much here so I
stopped short of refactoring fsr.

> >  	sx.sx_version = XFS_SX_VERSION;
> >  	sx.sx_fdtarget = file->fd;
> >  	sx.sx_fdtmp = fd;
> > diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
> > index b4468243..2a70824e 100644
> > --- a/libfrog/bulkstat.c
> > +++ b/libfrog/bulkstat.c
> > @@ -20,26 +20,99 @@ xfrog_bulkstat_prep_v1_emulation(
> >  	return xfd_prepare_geometry(xfd);
> >  }
> >  
> > +/* Bulkstat a single inode using v5 ioctl. */
> > +static int
> > +xfrog_bulkstat_single5(
> > +	struct xfs_fd			*xfd,
> > +	uint64_t			ino,
> > +	unsigned int			flags,
> > +	struct xfs_bulkstat		*bulkstat)
> > +{
> > +	struct xfs_bulkstat_req		*req;
> > +	int				ret;
> > +
> > +	if (flags & ~(XFS_BULK_IREQ_SPECIAL))
> > +		return EINVAL;
> 
> negative error returns, please.
> 
> > @@ -57,8 +57,6 @@ xfs_iterate_inodes_range_check(
> >  	int			error;
> >  
> >  	for (i = 0, bs = bstat; i < XFS_INODES_PER_CHUNK; i++) {
> > -		struct xfs_bstat bs1;
> > -
> >  		if (!(inogrp->xi_allocmask & (1ULL << i)))
> >  			continue;
> >  		if (bs->bs_ino == inogrp->xi_startino + i) {
> > @@ -68,13 +66,11 @@ xfs_iterate_inodes_range_check(
> >  
> >  		/* Load the one inode. */
> >  		error = xfrog_bulkstat_single(&ctx->mnt,
> > -				inogrp->xi_startino + i, &bs1);
> > -		if (error || bs1.bs_ino != inogrp->xi_startino + i) {
> > +				inogrp->xi_startino + i, 0, bs);
> > +		if (error || bs->bs_ino != inogrp->xi_startino + i) {
> >  			memset(bs, 0, sizeof(struct xfs_bulkstat));
> >  			bs->bs_ino = inogrp->xi_startino + i;
> >  			bs->bs_blksize = ctx->mnt_sv.f_frsize;
> > -		} else {
> > -			xfrog_bstat_to_bulkstat(&ctx->mnt, bs, &bs1);
> >  		}
> >  		bs++;
> >  	}
> 
> So this immediately tears down the confusing stuff that was set up
> in the previous patch. Perhaps separate out the scrub changes and do
> both bulkstat and bulkstat_single conversions in one patch?

Ok.

--D

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