Re: [PATCH 2/2] btrfs-progs: add support for tabular format for device stats

From: Boris Burkov <boris@bur.io>
To: Nikolay Borisov <nborisov@suse.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 2/2] btrfs-progs: add support for tabular format for device stats
Date: Mon, 18 Jul 2022 10:18:43 -0700	[thread overview]
Message-ID: <YtWV8xZ823vCC7qb@zen> (raw)
In-Reply-To: <20220718113439.2997247-2-nborisov@suse.com>

On Mon, Jul 18, 2022 at 02:34:39PM +0300, Nikolay Borisov wrote:
> Add support for the -T switch to 'device stats" command such that
> executing 'btrfs device stats -T' produces:
> 
> Id Path     Write errors Read errors Flush errors Corruption errors Generation errors
> -- -------- ------------ ----------- ------------ ----------------- -----------------
>  1 /dev/vdc            0           0            0                 0                 0
>  2 /dev/vdd            0           0            0                 0                 0

Works for me, and the output looks nice. One little weird thing was that
for me, without naming a device it errored out. Maybe I haven't rebased
my kernel recently enough? Works great when I name a device like:
'btrfs device stats -T /dev/foo' # works
but not like this:
'btrfs device stats -T' # error

I find the code flow of sticking in "if tabular" everywhere a little
unpleasant, so I was thinking it might help to try using function
pointers for the print function that you set when you process the
options. Having the little "prepare context" step makes it a little more
annoying since you need two functions. Food for thought, at least.

> 
> Link: https://lore.kernel.org/linux-btrfs/d7bd334d-13ad-8c5c-2122-1afc722fcc9c@dirtcellar.net
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  cmds/device.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index feffe9184726..926fbbd64615 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -27,6 +27,7 @@
>  #include "kerncompat.h"
>  #include "kernel-shared/ctree.h"
>  #include "ioctl.h"
> +#include "common/string-table.h"
>  #include "common/utils.h"
>  #include "kernel-shared/volumes.h"
>  #include "kernel-shared/zoned.h"
> @@ -572,6 +573,7 @@ static const char * const cmd_device_stats_usage[] = {
>  	"",
>  	"-c|--check             return non-zero if any stat counter is not zero",
>  	"-z|--reset             show current stats and reset values to zero",
> +	"-T                     show current stats in tabular format",

what do you think about --tabular for the long name?

>  	HELPINFO_INSERT_GLOBALS,
>  	HELPINFO_INSERT_FORMAT,
>  	NULL
> @@ -642,17 +644,75 @@ static int _print_device_stat_string(struct format_ctx *fctx,
>  	return err;
>  }
> 
> +
> +static int _print_device_stat_tabular(struct string_table *table, int row,
> +		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
> +{
> +	char *canonical_path = path_canonicalize(path);
> +	int j;
> +	int err = 0;
> +	static const struct {
> +		const char name[32];
> +		enum btrfs_dev_stat_values stat_idx;
> +	} dev_stats[] = {
> +		{ "write_io_errs", BTRFS_DEV_STAT_WRITE_ERRS },
> +		{ "read_io_errs", BTRFS_DEV_STAT_READ_ERRS },
> +		{ "flush_io_errs", BTRFS_DEV_STAT_FLUSH_ERRS },
> +		{ "corruption_errs", BTRFS_DEV_STAT_CORRUPTION_ERRS },
> +		{ "generation_errs", BTRFS_DEV_STAT_GENERATION_ERRS },
> +	};

Can you avoid duplicating this?

> +
> +	/* skip header + --- line */
> +	row += 2;
> +
> +	/* No path when device is missing. */
> +	if (!canonical_path) {
> +		canonical_path = malloc(32);
> +
> +		if (!canonical_path) {
> +			error("not enough memory for path buffer");
> +			return -ENOMEM;
> +		}
> +
> +		snprintf(canonical_path, 32, "devid:%llu", args->devid);
> +	}
> +	table_printf(table, 0, row, ">%llu", args->devid);
> +	table_printf(table, 1, row, ">%s", canonical_path);
> +	free(canonical_path);
> +
> +	for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> +		enum btrfs_dev_stat_values stat_idx = dev_stats[j].stat_idx;
> +		/* We got fewer items than we know */
> +		if (args->nr_items < stat_idx + 1)
> +			continue;
> +
> +	table_printf(table, 2, row, ">%llu", args->values[stat_idx]);
> +	table_printf(table, 3, row, ">%llu", args->values[stat_idx]);
> +	table_printf(table, 4, row, ">%llu", args->values[stat_idx]);
> +	table_printf(table, 5, row, ">%llu", args->values[stat_idx]);
> +	table_printf(table, 6, row, ">%llu", args->values[stat_idx]);
> +
> +	if (check && (args->values[stat_idx] > 0))
> +		err |= 64;
> +	}
> +
> +	return err;
> +}
> +
>  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  {
>  	char *dev_path;
>  	struct btrfs_ioctl_fs_info_args fi_args;
>  	struct btrfs_ioctl_dev_info_args *di_args = NULL;
> +	struct string_table *table = NULL;
>  	int ret;
>  	int fdmnt;
>  	int i;
>  	int err = 0;
>  	bool check = false;
> +	bool free_table = false;
>  	__u64 flags = 0;
> +	bool tabular = false;
>  	DIR *dirstream = NULL;
>  	struct format_ctx fctx;
> 
> @@ -665,7 +725,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			{NULL, 0, NULL, 0}
>  		};
> 
> -		c = getopt_long(argc, argv, "cz", long_options, NULL);
> +		c = getopt_long(argc, argv, "Tcz", long_options, NULL);
>  		if (c < 0)
>  			break;
> 
> @@ -676,6 +736,9 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  		case 'z':
>  			flags = BTRFS_DEV_STATS_RESET;
>  			break;
> +		case 'T':
> +			tabular = true;
> +			break;
>  		default:
>  			usage_unknown_option(cmd, argv);
>  		}
> @@ -703,11 +766,35 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  		goto out;
>  	}
> 
> -	fmt_start(&fctx, device_stats_rowspec, 24, 0);
> -	fmt_print_start_group(&fctx, "device-stats", JSON_TYPE_ARRAY);
> +	if (tabular) {
> +		/*
> +		 * cols = Id/Path/write/read/flush/corruption/generation
> +		 * rows = num devices + 2 (header and ---- line)
> +		 */
> +		table = table_create(7, fi_args.num_devices + 2);
> +		if (!table) {
> +			error("not enough memory");
> +			goto out;
> +		}
> +		free_table = true;
> +		table_printf(table, 0,0, "<Id");
> +		table_printf(table, 1,0, "<Path");
> +		table_printf(table, 2,0, "<Write errors");
> +		table_printf(table, 3,0, "<Read errors");
> +		table_printf(table, 4,0, "<Flush errors");
> +		table_printf(table, 5,0, "<Corruption errors");
> +		table_printf(table, 6,0, "<Generation errors");
> +		for (i = 0; i < 7; i++)
> +			table_printf(table, i, 1, "*-");
> +	} else {
> +		fmt_start(&fctx, device_stats_rowspec, 24, 0);
> +		fmt_print_start_group(&fctx, "device-stats", JSON_TYPE_ARRAY);
> +	}
> +
>  	for (i = 0; i < fi_args.num_devices; i++) {
>  		struct btrfs_ioctl_get_dev_stats args = {0};
>  		char path[BTRFS_DEVICE_PATH_NAME_MAX + 1];
> +		int err2;
> 
>  		strncpy(path, (char *)di_args[i].path,
>  			BTRFS_DEVICE_PATH_NAME_MAX);
> @@ -724,7 +811,11 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			goto out;
>  		}
> 
> -		int err2 = _print_device_stat_string(&fctx, &args, path, check);
> +		if (tabular)
> +			err2 = _print_device_stat_tabular(table, i, &args, path, check);
> +		else
> +			err2 = _print_device_stat_string(&fctx, &args, path, check);
> +
>  		if (err2) {
>  			if (err2 < 0) {
>  				err = err2;
> @@ -734,12 +825,18 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  		}
>  	}
> 
> -	fmt_print_end_group(&fctx, "device-stats");
> -	fmt_end(&fctx);
> +	if (tabular) {
> +		table_dump(table);
> +	} else {
> +		fmt_print_end_group(&fctx, "device-stats");
> +		fmt_end(&fctx);
> +	}
> 
>  out:
>  	free(di_args);
>  	close_file_or_dir(fdmnt, dirstream);
> +	if (free_table)
> +		table_free(table);
> 
>  	return err;
>  }
> --
> 2.17.1
> 