Re: [PATCH 1/1] xfs_spaceman: report health problems

[Dave Chinner <david@fromorbit.com>]

On Mon, Aug 26, 2019 at 02:20:54PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use the fs and ag geometry ioctls to report health problems to users.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  include/xfrog.h         |    2 
>  libfrog/fsgeom.c        |   11 +
>  man/man8/xfs_spaceman.8 |   28 +++
>  spaceman/Makefile       |    2 
>  spaceman/health.c       |  432 +++++++++++++++++++++++++++++++++++++++++++++++
>  spaceman/init.c         |    1 
>  spaceman/space.h        |    1 
>  7 files changed, 476 insertions(+), 1 deletion(-)
>  create mode 100644 spaceman/health.c
> 
> 
> diff --git a/include/xfrog.h b/include/xfrog.h
> index 5748e967..3a43a403 100644
> --- a/include/xfrog.h
> +++ b/include/xfrog.h
> @@ -177,4 +177,6 @@ struct xfs_inogrp;
>  int xfrog_inumbers(struct xfs_fd *xfd, uint64_t *lastino, uint32_t icount,
>  		struct xfs_inogrp *ubuffer, uint32_t *ocount);
>  
> +int xfrog_ag_geometry(int fd, unsigned int agno, struct xfs_ag_geometry *ageo);
> +
>  #endif	/* __XFROG_H__ */
> diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
> index 17479e4a..cddb5a39 100644
> --- a/libfrog/fsgeom.c
> +++ b/libfrog/fsgeom.c
> @@ -131,3 +131,14 @@ xfrog_close(
>  	xfd->fd = -1;
>  	return ret;
>  }
> +
> +/* Try to obtain an AG's geometry. */
> +int
> +xfrog_ag_geometry(
> +	int			fd,
> +	unsigned int		agno,
> +	struct xfs_ag_geometry	*ageo)
> +{
> +	ageo->ag_number = agno;
> +	return ioctl(fd, XFS_IOC_AG_GEOMETRY, ageo);

Does it need this first:

	memset(ageo, 0, sizeof(*ageo));

Because I don't think the callers zero it....

> +static const struct flag_map inode_flags[] = {
> +	{
> +		.mask = XFS_BS_SICK_INODE,
> +		.descr = "inode core",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_BMBTD,
> +		.descr = "data fork",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_BMBTA,
> +		.descr = "extended attribute fork",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_BMBTC,
> +		.descr = "copy on write fork",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_DIR,
> +		.descr = "directory",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_XATTR,
> +		.descr = "extended attributes",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_SYMLINK,
> +		.descr = "symbolic link target",
> +	},
> +	{
> +		.mask = XFS_BS_SICK_PARENT,
> +		.descr = "parent pointers",

This needs a "has_parent_pointers" feature check function,
doesn't it? Or is this already a valid status for directory inodes?

> +static int
> +report_bulkstat_health(
> +	xfs_agnumber_t		agno)
> +{
> +	struct xfs_bstat	bstat[128];
> +	char			descr[256];
> +	uint64_t		startino = 0;
> +	uint64_t		lastino = -1ULL;
> +	uint32_t		ocount;
> +	uint32_t		i;
> +	int			error;
> +
> +	if (agno != NULLAGNUMBER) {
> +		startino = xfrog_agino_to_ino(&file->xfd, agno, 0);
> +		lastino = xfrog_agino_to_ino(&file->xfd, agno + 1, 0) - 1;
> +	}
> +
> +	while ((error = xfrog_bulkstat(&file->xfd, &startino, 128, bstat,

Nit: use a define for the number of inodes to bulkstat.

> +health_f(
> +	int			argc,
> +	char			**argv)
> +{
> +	unsigned long long	x;
> +	xfs_agnumber_t		agno;
> +	bool			default_report = true;
> +	int			c;
> +	int			ret;
> +
> +	reported = 0;
> +
> +	if (file->xfd.fsgeom.version != XFS_FSOP_GEOM_VERSION_V5) {
> +		perror("health");
> +		return 1;
> +	}
> +
> +	while ((c = getopt(argc, argv, "a:cfi:q")) != EOF) {
> +		switch (c) {
> +		case 'a':
> +			default_report = false;
> +			errno = 0;
> +			x = strtoll(optarg, NULL, 10);
> +			if (!errno && x >= NULLAGNUMBER)
> +				errno = ERANGE;
> +			if (errno) {
> +				perror("ag health");
> +				return 1;
> +			}
> +			agno = x;
> +			ret = report_ag_sick(agno);
> +			if (!ret && comprehensive)
> +				ret = report_bulkstat_health(agno);
> +			if (ret)
> +				return 1;
> +			break;
> +		case 'c':
> +			comprehensive = true;
> +			break;

There's a command line ordering problem here - - "-a" and "-f" 
check the comprehensive flag and do additional stuff based on it.

So I think the -a and -f processing need to be done outside
the args processing loop, or we need two loops to extract the
-c first...

Otherwise looks OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com