Re: [PATCH 3/6] misc: convert XFS_IOC_FSBULKSTAT to XFS_IOC_BULKSTAT

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

On Fri, Sep 13, 2019 at 10:54:26AM +1000, Dave Chinner wrote:
> On Thu, Sep 05, 2019 at 08:35:15PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert the v1 calls to v5 calls.

This really should have been more descriptive, sorry...

"Convert xfrog_bulkstat() to take arguments using v5 bulkstat semantics
and return bulkstat information in v5 structures.  If the v5 ioctl is
not available, xfrog_bulkstat() will emulate the v5 ioctl using the v1
ioctl."

> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fsr/xfs_fsr.c      |   45 ++++++--
> >  io/open.c          |   17 ++-
> >  libfrog/bulkstat.c |  290 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  libfrog/bulkstat.h |   10 +-
> >  libfrog/fsgeom.h   |    9 ++
> >  quota/quot.c       |   29 ++---
> >  scrub/inodes.c     |   45 +++++---
> >  scrub/inodes.h     |    2 
> >  scrub/phase3.c     |    6 +
> >  scrub/phase5.c     |    8 +
> >  scrub/phase6.c     |    2 
> >  scrub/unicrash.c   |    6 +
> >  scrub/unicrash.h   |    4 -
> >  spaceman/health.c  |   28 +++--
> >  14 files changed, 411 insertions(+), 90 deletions(-)
> > 
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index a53eb924..cc3cc93a 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -466,6 +466,17 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
> >  				ptr = strchr(ptr, ' ');
> >  				if (ptr) {
> >  					startino = strtoull(++ptr, NULL, 10);
> > +					/*
> > +					 * NOTE: The inode number read in from
> > +					 * the leftoff file is the last inode
> > +					 * to have been fsr'd.  Since the new
> > +					 * xfrog_bulkstat function wants to be
> > +					 * passed the first inode that we want
> > +					 * to examine, increment the value that
> > +					 * we read in.  The debug message below
> > +					 * prints the lastoff value.
> > +					 */
> > +					startino++;
> >  				}
> >  			}
> >  			if (startpass < 0)
> > @@ -484,7 +495,7 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
> >  
> >  	if (vflag) {
> >  		fsrprintf(_("START: pass=%d ino=%llu %s %s\n"),
> > -			  fs->npass, (unsigned long long)startino,
> > +			  fs->npass, (unsigned long long)startino - 1,
> >  			  fs->dev, fs->mnt);
> >  	}
> 
> This could probably go in a spearate patch....

It can't, because we're changing the meaning of the xfrog_bulkstat
arguments to match the ioctls.

> > @@ -724,7 +724,6 @@ inode_f(
> >  	char			**argv)
> >  {
> >  	struct xfs_bstat	bstat;
> > -	uint32_t		count = 0;
> >  	uint64_t		result_ino = 0;
> >  	uint64_t		userino = NULLFSINO;
> >  	char			*p;
> > @@ -775,21 +774,31 @@ inode_f(
> >  		}
> >  	} else if (ret_next) {
> >  		struct xfs_fd	xfd = XFS_FD_INIT(file->fd);
> > +		struct xfs_bulkstat_req	*breq;
> > +
> > +		breq = xfrog_bulkstat_alloc_req(1, userino + 1);
> > +		if (!breq) {
> > +			perror("alloc bulkstat");
> > +			exitcode = 1;
> > +			return 0;
> > +		}
> >  
> >  		/* get next inode */
> > -		ret = xfrog_bulkstat(&xfd, &userino, 1, &bstat, &count);
> 
> Why the "+ 1" on userino setup for the new interface?

The inode parameter to bulkstat changes between v1 and v5:

FSBULKSTAT (i.e. v1) takes a *lastino pointer that's supposed to point
to the inode number before the one you want.  If you want to bulkstat
starting with inode 100, you set *lastino = 99.

FSBULKSTAT_SINGLE (v1) takes a *lastino pointer that points to the inode
you want.  If you want to B_S inode 100, you set *lastino = 100.

BULKSTAT (v5) takes a startino number in the request header that should
be the inode you want.  If you want to bulkstat starting with inode 100,
you set breq->startino = 100.

In fsr's case, it traditionally used FSBULKSTAT (v1) and records the
last inode number that it defragged, so in order to use BULKSTAT (v5) we
have to increment the startino value here.  If v5 bulkstat is available
then we pass that number straight to the kernel.  If not, the
xfrog_bulk_req_setup function decrements the inode number so that it can
use the v1 ioctl to emulate the v5 behavior.

> 
> > @@ -29,29 +42,278 @@ xfrog_bulkstat_single(
> >  	return 0;
> >  }
> >  
> > -/* Bulkstat a bunch of inodes.  Returns zero or a positive error code. */
> > -int
> > -xfrog_bulkstat(
> > +/*
> > + * Set up emulation of a v5 bulk request ioctl with a v1 bulk request ioctl.
> > + * Returns 0 if the emulation should proceed; ECANCELED if there are no
> > + * records; or a positive error code.

/*
 * Given a v5 BULKSTAT request, set up a v1 FSBULKSTAT control structure
 * so that we can emulate the v5 request using the old v1 code as best
 * we can.  This enables callers to run on older kernels.
 *
 * Returns 0 if the emulation should succeed, -ECANCELED if there won't
 * be any records; or a negative error code.
 */

How about that?

> > + */
> > +static int
> > +xfrog_bulk_req_setup(
> >  	struct xfs_fd		*xfd,
> > -	uint64_t		*lastino,
> > -	uint32_t		icount,
> > -	struct xfs_bstat	*ubuffer,
> > -	uint32_t		*ocount)
> > +	struct xfs_bulk_ireq	*hdr,
> > +	struct xfs_fsop_bulkreq	*bulkreq,
> > +	size_t			rec_size)
> > +{
> > +	void			*buf;
> > +
> > +	if (hdr->flags & XFS_BULK_IREQ_AGNO) {
> > +		uint32_t	agno = cvt_ino_to_agno(xfd, hdr->ino);
> > +
> > +		if (hdr->ino == 0)
> > +			hdr->ino = cvt_agino_to_ino(xfd, hdr->agno, 0);
> > +		else if (agno < hdr->agno)
> > +			return EINVAL;
> > +		else if (agno > hdr->agno)
> > +			goto no_results;
> > +	}
> > +
> > +	if (cvt_ino_to_agno(xfd, hdr->ino) > xfd->fsgeom.agcount)
> > +		goto no_results;
> > +
> > +	buf = malloc(hdr->icount * rec_size);
> > +	if (!buf)
> > +		return errno;
> > +
> > +	if (hdr->ino)
> > +		hdr->ino--;
> 
> This goes with my last question: why?

(See above)

> > +	bulkreq->lastip = (__u64 *)&hdr->ino,
> > +	bulkreq->icount = hdr->icount,
> > +	bulkreq->ocount = (__s32 *)&hdr->ocount,
> > +	bulkreq->ubuffer = buf;
> > +	return 0;
> > +
> > +no_results:
> > +	hdr->ocount = 0;
> > +	return ECANCELED;
> 
> We should be returning negative errors for everything.

Heh, that's going to be a lengthy overhaul of everything that comes
after this.

> 
> > +}
> > +
> > +/*
> > + * Convert records and free resources used to do a v1 emulation of v5 bulk
> > + * request.
> > + */

/*
 * If we have used the v1 FSBULKSTAT ioctl to emulate the v5 BULKSTAT
 * ioctl for a caller, migrate the v1 bulkstat data into the caller's
 * v5 bulkstat buffer and tear down the emulation control structures.
 */

> > +static int
> > +xfrog_bulk_req_teardown(
> 
> What's "teardown" got to do with converting results to a v1 format?
> 
> Indeed, why is there even emulation of v1 calls in the first place?
> why don't callers that need v1 format just use the existing v1
> ioctls directly?

This code emulates v5 bulkstat using old v1 bulkstat, but we've covered
this above.

> 
> >  
> > +/* Bulkstat a bunch of inodes using the v1 interface. */
> > +static int
> > +xfrog_bulkstat1(
> > +	struct xfs_fd		*xfd,
> > +	struct xfs_bulkstat_req	*req)
> > +{
> > +	struct xfs_fsop_bulkreq	bulkreq = { 0 };
> > +	int			error;
> > +
> > +	error = xfrog_bulkstat_prep_v1_emulation(xfd);
> > +	if (error)
> > +		return error;
> > +
> > +	error = xfrog_bulk_req_setup(xfd, &req->hdr, &bulkreq,
> > +			sizeof(struct xfs_bstat));
> > +	if (error == ECANCELED)
> > +		goto out_teardown;
> > +	if (error)
> > +		return error;
> > +
> > +	error = ioctl(xfd->fd, XFS_IOC_FSBULKSTAT, &bulkreq);
> > +	if (error)
> > +		error = errno;
> 
> negative errors, please.
> 
> > +
> > +out_teardown:
> > +	return xfrog_bulk_req_teardown(xfd, &req->hdr, &bulkreq,
> > +			sizeof(struct xfs_bstat), xfrog_bstat_ino,
> > +			&req->bulkstat, sizeof(struct xfs_bulkstat),
> > +			xfrog_bstat_cvt, 1, error);
> > +}
> 
> What conversion is necessary here given we've done a v1 format
> bulkstat?
> 
> > +/* Bulkstat a bunch of inodes.  Returns zero or a positive error code. */
> > +int
> > +xfrog_bulkstat(
> > +	struct xfs_fd		*xfd,
> > +	struct xfs_bulkstat_req	*req)
> > +{
> > +	int			error;
> > +
> > +	if (xfd->flags & XFROG_FLAG_BULKSTAT_FORCE_V1)
> > +		goto try_v1;
> > +
> > +	error = xfrog_bulkstat5(xfd, req);
> > +	if (error == 0 || (xfd->flags & XFROG_FLAG_BULKSTAT_FORCE_V5))
> > +		return error;
> > +
> > +	/* If the v5 ioctl wasn't found, we punt to v1. */
> > +	switch (error) {
> > +	case EOPNOTSUPP:
> > +	case ENOTTY:
> > +		xfd->flags |= XFROG_FLAG_BULKSTAT_FORCE_V1;
> > +		break;
> > +	}
> > +
> > +try_v1:
> > +	return xfrog_bulkstat1(xfd, req);
> > +}
> 
> Oh, wait, "v1 emulation" is supposed to mean "use a v1 call to
> return v5 format structures"? That's emulation of the _v5_ ioctl,
> which kinda says to me there's some naming problems here...

Yes.

> 
> > +/* Convert bulkstat (v5) to bstat (v1). */
> > +void
> > +xfrog_bulkstat_to_bstat(
> > +	struct xfs_fd			*xfd,
> > +	struct xfs_bstat		*bs1,
> > +	const struct xfs_bulkstat	*bstat)
> 
> Which I read as "convert bulkstat to bulkstat" and it doesn't tell
> me what is actually going on.  xfrog_bulkstat_v5_to_v1() indicates
> what format conversion is actually taking place...
> 
> and um, naming the v5 field bstat, and the struct xfs_bstat field
> bs1 is entirely confusing.

Indeed.  I'll change it as you suggest.

> void
> xfrog_bulkstat_v5_to_v1(
> 	struct xfs_fd		*xfd
> 	const struct xfs_bulkstat *from,
> 	struct xfs_bstat	*to)
> {
> 	to->bs_ino = from->bs_ino;
> ....
> 
> > +{
> > +	bs1->bs_ino = bstat->bs_ino;
> > +	bs1->bs_mode = bstat->bs_mode;
> > +	bs1->bs_nlink = bstat->bs_nlink;
> > +	bs1->bs_uid = bstat->bs_uid;
> > +	bs1->bs_gid = bstat->bs_gid;
> > +	bs1->bs_rdev = bstat->bs_rdev;
> > +	bs1->bs_blksize = bstat->bs_blksize;
> > +	bs1->bs_size = bstat->bs_size;
> > +	bs1->bs_atime.tv_sec = bstat->bs_atime;
> > +	bs1->bs_mtime.tv_sec = bstat->bs_mtime;
> > +	bs1->bs_ctime.tv_sec = bstat->bs_ctime;
> 
> What about 32 bit overflows here?

Oops.  Ok.

> > +/* Convert bstat (v1) to bulkstat (v5). */
> > +void
> > +xfrog_bstat_to_bulkstat(
> > +	struct xfs_fd			*xfd,
> > +	struct xfs_bulkstat		*bstat,
> > +	const struct xfs_bstat		*bs1)
> > +{
> 
> same comments about names here.
> >  
> > +/* Only use v1 bulkstat/inumbers ioctls. */
> > +#define XFROG_FLAG_BULKSTAT_FORCE_V1	(1 << 0)
> > +
> > +/* Only use v5 bulkstat/inumbers ioctls. */
> > +#define XFROG_FLAG_BULKSTAT_FORCE_V5	(1 << 1)
> 
> These don't actually define what format the results are presented
> in. What happens if the user wants v1 format structures but wants
> the V5 ioctl to be used?
> 
> > --- a/scrub/inodes.c
> > +++ b/scrub/inodes.c
> > @@ -50,13 +50,15 @@ static void
> >  xfs_iterate_inodes_range_check(
> >  	struct scrub_ctx	*ctx,
> >  	struct xfs_inogrp	*inogrp,
> > -	struct xfs_bstat	*bstat)
> > +	struct xfs_bulkstat	*bstat)
> >  {
> > -	struct xfs_bstat	*bs;
> > +	struct xfs_bulkstat	*bs;
> >  	int			i;
> >  	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) {
> > @@ -66,11 +68,13 @@ xfs_iterate_inodes_range_check(
> >  
> >  		/* Load the one inode. */
> >  		error = xfrog_bulkstat_single(&ctx->mnt,
> > -				inogrp->xi_startino + i, bs);
> > -		if (error || bs->bs_ino != inogrp->xi_startino + i) {
> > -			memset(bs, 0, sizeof(struct xfs_bstat));
> > +				inogrp->xi_startino + i, &bs1);
> > +		if (error || bs1.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);
> 
> I'm confused - why is xfrog_bulkstat_single() returning a v1 format
> structure here and not using v5 format for everything?

Because I probably should have converted both at the same time. :/

--D

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