Re: [PATCH 1/4] xfs_io: add a bulkstat command

Dave Chinner
2019-09-12T23:51:23Z

On Thu, Sep 05, 2019 at 08:35:44PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Add a bulkstat command to xfs_io so that we can test our new xfrog code.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> io/Makefile | 9 -
> io/bulkstat.c | 533 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> io/init.c | 1
> io/io.h | 1
> libfrog/bulkstat.c | 20 ++
> libfrog/bulkstat.h | 3
> man/man8/xfs_io.8 | 68 +++++++
> 7 files changed, 631 insertions(+), 4 deletions(-)
> create mode 100644 io/bulkstat.c
>
>
> diff --git a/io/Makefile b/io/Makefile
> index 484e2b5a..1112605e 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -9,10 +9,11 @@ LTCOMMAND = xfs_io
> LSRCFILES = xfs_bmap.sh xfs_freeze.sh xfs_mkfile.sh
> HFILES = init.h io.h
> CFILES = init.c \
> - attr.c bmap.c crc32cselftest.c cowextsize.c encrypt.c file.c freeze.c \
> - fsync.c getrusage.c imap.c inject.c label.c link.c mmap.c open.c \
> - parent.c pread.c prealloc.c pwrite.c reflink.c resblks.c scrub.c \
> - seek.c shutdown.c stat.c swapext.c sync.c truncate.c utimes.c
> + attr.c bmap.c bulkstat.c crc32cselftest.c cowextsize.c encrypt.c \
> + file.c freeze.c fsync.c getrusage.c imap.c inject.c label.c link.c \
> + mmap.c open.c parent.c pread.c prealloc.c pwrite.c reflink.c \
> + resblks.c scrub.c seek.c shutdown.c stat.c swapext.c sync.c \
> + truncate.c utimes.c
>
> LLDLIBS = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD)
> LTDEPENDENCIES = $(LIBXCMD) $(LIBHANDLE) $(LIBFROG)
> diff --git a/io/bulkstat.c b/io/bulkstat.c
> new file mode 100644
> index 00000000..76ba682b
> --- /dev/null
> +++ b/io/bulkstat.c
> @@ -0,0 +1,533 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Oracle. All Rights Reserved.
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + */
> +#include "xfs.h"
> +#include "platform_defs.h"
> +#include "command.h"
> +#include "init.h"
> +#include "libfrog/fsgeom.h"
> +#include "libfrog/bulkstat.h"
> +#include "libfrog/paths.h"
> +#include "io.h"
> +#include "input.h"
> +
> +static bool debug;
> +
> +static void
> +dump_bulkstat_time(
> + const char *tag,
> + uint64_t sec,
> + uint32_t nsec)
> +{
> + printf("\t%s = %"PRIu64".%"PRIu32"\n", tag, sec, nsec);
> +}
> +
> +static void
> +dump_bulkstat(
> + struct xfs_bulkstat *bstat)
> +{
> + printf("bs_ino = %"PRIu64"\n", bstat->bs_ino);
> + printf("\tbs_size = %"PRIu64"\n", bstat->bs_size);
> +
> + printf("\tbs_blocks = %"PRIu64"\n", bstat->bs_blocks);
> + printf("\tbs_xflags = 0x%"PRIx64"\n", bstat->bs_xflags);
> +
> + dump_bulkstat_time("bs_atime", bstat->bs_atime, bstat->bs_atime_nsec);
> + dump_bulkstat_time("bs_ctime", bstat->bs_ctime, bstat->bs_ctime_nsec);
> + dump_bulkstat_time("bs_mtime", bstat->bs_mtime, bstat->bs_mtime_nsec);
> + dump_bulkstat_time("bs_btime", bstat->bs_btime, bstat->bs_btime_nsec);
> +
> + printf("\tbs_gen = 0x%"PRIx32"\n", bstat->bs_gen);
> + printf("\tbs_uid = %"PRIu32"\n", bstat->bs_uid);
> + printf("\tbs_gid = %"PRIu32"\n", bstat->bs_gid);
> + printf("\tbs_projectid = %"PRIu32"\n", bstat->bs_projectid);
> +
> + printf("\tbs_blksize = %"PRIu32"\n", bstat->bs_blksize);
> + printf("\tbs_rdev = %"PRIu32"\n", bstat->bs_rdev);
> + printf("\tbs_cowextsize_blks = %"PRIu32"\n", bstat->bs_cowextsize_blks);
> + printf("\tbs_extsize_blks = %"PRIu32"\n", bstat->bs_extsize_blks);
> +
> + printf("\tbs_nlink = %"PRIu32"\n", bstat->bs_nlink);
> + printf("\tbs_extents = %"PRIu32"\n", bstat->bs_extents);
> + printf("\tbs_aextents = %"PRIu32"\n", bstat->bs_aextents);
> + printf("\tbs_version = %"PRIu16"\n", bstat->bs_version);
> + printf("\tbs_forkoff = %"PRIu16"\n", bstat->bs_forkoff);
> +
> + printf("\tbs_sick = 0x%"PRIx16"\n", bstat->bs_sick);
> + printf("\tbs_checked = 0x%"PRIx16"\n", bstat->bs_checked);
> + printf("\tbs_mode = 0%"PRIo16"\n", bstat->bs_mode);
> +};
> +
> +static void
> +bulkstat_help(void)
> +{
> + printf(_(
> +"Bulk-queries the filesystem for inode stat information and prints it.\n"
> +"\n"
> +" -a Only iterate this AG.\n"
> +" -d Print debugging output.\n"
> +" -e Stop after this inode.\n"
> +" -n Ask for this many results at once.\n"
> +" -s Inode to start with.\n"
> +" -v Use this version of the ioctl (1 or 5).\n"));
> +}
> +
> +static int
> +bulkstat_f(
> + int argc,
> + char **argv)
> +{
> + struct xfs_fd xfd = XFS_FD_INIT(file->fd);
> + struct xfs_bulkstat_req *breq;
> + unsigned long long startino = 0;
> + unsigned long long endino = -1ULL;
> + unsigned long batch_size = 4096;
> + unsigned long agno = 0;
> + unsigned long ver = 0;
> + bool has_agno = false;
> + unsigned int i;
> + int c;
> + int ret;
> +
> + while ((c = getopt(argc, argv, "a:cde:n:qs:v:")) != -1) {

options c and q are not documented above, and not handled in the
case statement below.

> + switch (c) {
> + case 'a':
> + errno = 0;
> + agno = strtoul(optarg, NULL, 10);
> + if (errno) {
> + perror(optarg);
> + return 1;
> + }
> + has_agno = true;
> + break;

Why not use cvt_u32() and friends for these so they are directly
converted to the correct type and overflow checked at the same time?

[...]

> +static void
> +inumbers_help(void)
> +{
> + printf(_(
> +"Queries the filesystem for inode group information and prints it.\n"
> +"\n"
> +" -a Only iterate this AG.\n"
> +" -d Print debugging output.\n"
> +" -e Stop after this inode.\n"
> +" -n Ask for this many results at once.\n"
> +" -s Inode to start with.\n"
> +" -v Use this version of the ioctl (1 or 5).\n"));
> +}
> +
> +static int
> +inumbers_f(
> + int argc,
> + char **argv)
> +{
> + struct xfs_fd xfd = XFS_FD_INIT(file->fd);
> + struct xfs_inumbers_req *ireq;
> + unsigned long long startino = 0;
> + unsigned long long endino = -1ULL;
> + unsigned long batch_size = 4096;
> + unsigned long agno = 0;
> + unsigned long ver = 0;
> + bool has_agno = false;
> + unsigned int i;
> + int c;
> + int ret;
> +
> + while ((c = getopt(argc, argv, "a:cde:n:qs:v:")) != -1) {

Again, c and q not defined. Same comments about cvt_type() as
well...

> + if (has_agno)
> + xfrog_inumbers_set_ag(ireq, agno);
> +
> + switch (ver) {
> + case 1:
> + xfd.flags |= XFROG_FLAG_BULKSTAT_FORCE_V1;
> + break;
> + case 5:
> + xfd.flags |= XFROG_FLAG_BULKSTAT_FORCE_V5;
> + break;
> + default:
> + break;
> + }

Common helper?

> +static cmdinfo_t bulkstat_cmd = {
> + .name = "bulkstat",
> + .cfunc = bulkstat_f,
> + .argmin = 0,
> + .argmax = -1,
> + .flags = CMD_NOMAP_OK,
> + .help = bulkstat_help,
> +};

Theres are all oneshot commands, right?

> +
> +static cmdinfo_t bulkstat_single_cmd = {
> + .name = "bulkstat_single",
> + .cfunc = bulkstat_single_f,
> + .argmin = 0,
> + .argmax = -1,
> + .flags = CMD_NOMAP_OK,
> + .help = bulkstat_single_help,
> +};

Doesn't this require at least one parameter?

>
> .SH FILESYSTEM COMMANDS
> .TP
> +.BI "bulkstat [ \-a " agno " ] [ \-d ] [ \-e " endino " ] [ \-n " batchsize " ] [ \-s " startino " ]
> +Display raw stat information about a bunch of inodes in an XFS filesystem.
> +Options are as follows:
> +.RS 1.0i
> +.PD 0
> +.TP
> +.BI \-a " agno"
> +Display only results from the given allocation group.
> +Defaults to zero.

Need to say "If not specified, will all AGs in the fielsystem"

> @@ -1067,6 +1106,35 @@ was specified on the command line, the maximum possible inode number in
> the system will be printed along with its size.
> .PD
> .TP
> +.BI "inumbers [ \-a " agno " ] [ \-d ] [ \-e " endino " ] [ \-n " batchsize " ] [ \-s " startino " ]
> +Prints allocation information about groups of inodes in an XFS filesystem.
> +Callers can use this information to figure out which inodes are allocated.
> +Options are as follows:
> +.RS 1.0i
> +.PD 0
> +.TP
> +.BI \-a " agno"
> +Display only results from the given allocation group.
> +Defaults to zero.

Same again.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com