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

[Dave Chinner <david@fromorbit.com>]

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