On Thu, Sep 05, 2019 at 08:35:15PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Convert the v1 calls to v5 calls.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> 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....
> @@ -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?
> @@ -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.
> + */
> +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?
> + 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.
> +}
> +
> +/*
> + * Convert records and free resources used to do a v1 emulation of v5 bulk
> + * request.
> + */
> +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?
>
> +/* 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...
> +/* 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.
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?
> +/* 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?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
