[PATCH 1/2] btrfs-progs: factor out device stats printing code

[PATCH 1/2] btrfs-progs: factor out device stats printing code

[Nikolay Borisov]

This is in preparation for introducing tabular output for device stats. Simply
factor out string-specific output lines in a separate function.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
 1 file changed, 76 insertions(+), 65 deletions(-)

diff --git a/cmds/device.c b/cmds/device.c
index 7d3febff96c2..feffe9184726 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -577,6 +577,71 @@ static const char * const cmd_device_stats_usage[] = {
 	NULL
 };

+static int _print_device_stat_string(struct format_ctx *fctx,
+		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
+{
+	char *canonical_path = path_canonicalize(path);
+	char devid_str[32];
+	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 },
+	};
+	/*
+	 * The plain text and json formats cannot be
+	 * mapped directly in all cases and we have to switch
+	 */
+	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
+
+	/* 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);
+	}
+	snprintf(devid_str, 32, "%llu", args->devid);
+	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
+	/* Plain text does not print device info */
+	if (json) {
+		fmt_print(fctx, "device", canonical_path);
+		fmt_print(fctx, "devid", args->devid);
+	}
+
+	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;
+
+		/* Own format due to [/dev/name].value */
+		if (json) {
+			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
+		} else {
+			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
+					(unsigned long long)args->values[stat_idx]);
+		}
+		if (check && (args->values[stat_idx] > 0))
+			err |= 64;
+	}
+
+	fmt_print_end_group(fctx, NULL);
+	free(canonical_path);
+
+	return err;
+}
+
 static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	char *dev_path;
@@ -586,7 +651,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 	int fdmnt;
 	int i;
 	int err = 0;
-	int check = 0;
+	bool check = false;
 	__u64 flags = 0;
 	DIR *dirstream = NULL;
 	struct format_ctx fctx;
@@ -606,7 +671,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)

 		switch (c) {
 		case 'c':
-			check = 1;
+			check = true;
 			break;
 		case 'z':
 			flags = BTRFS_DEV_STATS_RESET;
@@ -656,70 +721,16 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 			error("device stats ioctl failed on %s: %m",
 			      path);
 			err |= 1;
-		} else {
-			char *canonical_path;
-			char devid_str[32];
-			int j;
-			static const struct {
-				const char name[32];
-				u64 num;
-			} 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 },
-			};
-			/*
-			 * The plain text and json formats cannot be
-			 * mapped directly in all cases and we have to switch
-			 */
-			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
-
-			canonical_path = path_canonicalize(path);
-
-			/* No path when device is missing. */
-			if (!canonical_path) {
-				canonical_path = malloc(32);
-				if (!canonical_path) {
-					error("not enough memory for path buffer");
-					goto out;
-				}
-				snprintf(canonical_path, 32,
-					 "devid:%llu", args.devid);
-			}
-			snprintf(devid_str, 32, "%llu", args.devid);
-			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
-			/* Plain text does not print device info */
-			if (json) {
-				fmt_print(&fctx, "device", canonical_path);
-				fmt_print(&fctx, "devid", di_args[i].devid);
-			}
+			goto out;
+		}

-			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
-				/* We got fewer items than we know */
-				if (args.nr_items < dev_stats[j].num + 1)
-					continue;
-
-				/* Own format due to [/dev/name].value */
-				if (json) {
-					fmt_print(&fctx, dev_stats[j].name,
-						args.values[dev_stats[j].num]);
-				} else {
-					printf("[%s].%-16s %llu\n",
-						canonical_path,
-						dev_stats[j].name,
-						(unsigned long long)
-						args.values[dev_stats[j].num]);
-				}
-				if ((check == 1)
-				    && (args.values[dev_stats[j].num] > 0))
-					err |= 64;
-			}
-			fmt_print_end_group(&fctx, NULL);
-			free(canonical_path);
+		int err2 = _print_device_stat_string(&fctx, &args, path, check);
+		if (err2) {
+			if (err2 < 0) {
+				err = err2;
+				goto out;
+			} else
+				err |= err2;
 		}
 	}

--
2.17.1

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

[Nikolay Borisov]

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

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",
 	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 },
+	};
+
+	/* 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

Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code

[David Sterba]

On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 65 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index 7d3febff96c2..feffe9184726 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -577,6 +577,71 @@ static const char * const cmd_device_stats_usage[] = {
>  	NULL
>  };
> 
> +static int _print_device_stat_string(struct format_ctx *fctx,
> +		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
> +{
> +	char *canonical_path = path_canonicalize(path);
> +	char devid_str[32];
> +	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 },
> +	};
> +	/*
> +	 * The plain text and json formats cannot be
> +	 * mapped directly in all cases and we have to switch
> +	 */
> +	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> +
> +	/* 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);
> +	}
> +	snprintf(devid_str, 32, "%llu", args->devid);
> +	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
> +	/* Plain text does not print device info */
> +	if (json) {
> +		fmt_print(fctx, "device", canonical_path);
> +		fmt_print(fctx, "devid", args->devid);
> +	}
> +
> +	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;
> +
> +		/* Own format due to [/dev/name].value */
> +		if (json) {
> +			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
> +		} else {
> +			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
> +					(unsigned long long)args->values[stat_idx]);
> +		}
> +		if (check && (args->values[stat_idx] > 0))
> +			err |= 64;
> +	}
> +
> +	fmt_print_end_group(fctx, NULL);
> +	free(canonical_path);
> +
> +	return err;
> +}
> +
>  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  {
>  	char *dev_path;
> @@ -586,7 +651,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  	int fdmnt;
>  	int i;
>  	int err = 0;
> -	int check = 0;
> +	bool check = false;
>  	__u64 flags = 0;
>  	DIR *dirstream = NULL;
>  	struct format_ctx fctx;
> @@ -606,7 +671,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> 
>  		switch (c) {
>  		case 'c':
> -			check = 1;
> +			check = true;
>  			break;
>  		case 'z':
>  			flags = BTRFS_DEV_STATS_RESET;
> @@ -656,70 +721,16 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			error("device stats ioctl failed on %s: %m",
>  			      path);
>  			err |= 1;
> -		} else {
> -			char *canonical_path;
> -			char devid_str[32];
> -			int j;
> -			static const struct {
> -				const char name[32];
> -				u64 num;
> -			} 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 },
> -			};
> -			/*
> -			 * The plain text and json formats cannot be
> -			 * mapped directly in all cases and we have to switch
> -			 */
> -			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> -
> -			canonical_path = path_canonicalize(path);
> -
> -			/* No path when device is missing. */
> -			if (!canonical_path) {
> -				canonical_path = malloc(32);
> -				if (!canonical_path) {
> -					error("not enough memory for path buffer");
> -					goto out;
> -				}
> -				snprintf(canonical_path, 32,
> -					 "devid:%llu", args.devid);
> -			}
> -			snprintf(devid_str, 32, "%llu", args.devid);
> -			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
> -			/* Plain text does not print device info */
> -			if (json) {
> -				fmt_print(&fctx, "device", canonical_path);
> -				fmt_print(&fctx, "devid", di_args[i].devid);
> -			}
> +			goto out;
> +		}
> 
> -			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> -				/* We got fewer items than we know */
> -				if (args.nr_items < dev_stats[j].num + 1)
> -					continue;
> -
> -				/* Own format due to [/dev/name].value */
> -				if (json) {
> -					fmt_print(&fctx, dev_stats[j].name,
> -						args.values[dev_stats[j].num]);
> -				} else {
> -					printf("[%s].%-16s %llu\n",
> -						canonical_path,
> -						dev_stats[j].name,
> -						(unsigned long long)
> -						args.values[dev_stats[j].num]);
> -				}
> -				if ((check == 1)
> -				    && (args.values[dev_stats[j].num] > 0))
> -					err |= 64;
> -			}
> -			fmt_print_end_group(&fctx, NULL);
> -			free(canonical_path);
> +		int err2 = _print_device_stat_string(&fctx, &args, path, check);

Please don't define variables in the statement block.

> +		if (err2) {
> +			if (err2 < 0) {
> +				err = err2;
> +				goto out;
> +			} else
> +				err |= err2;
>  		}
>  	}
> 
> --
> 2.17.1

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

[David Sterba]

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
> 
> 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",

We'll need the long option too, but I see it's not in the 'filesystem
usage' either, yet.

>  	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,

Please don't use the underscored version, that's maybe in some old in
btrfs-progs code but should not be in new.

Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code

[David Sterba]

On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Added to devel, with some fixups, thanks.

Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code

[Boris Burkov]

On Mon, Jul 18, 2022 at 02:34:38PM +0300, Nikolay Borisov wrote:
> This is in preparation for introducing tabular output for device stats. Simply
> factor out string-specific output lines in a separate function.

LGTM, and works fine. I mentioned a few nits inline.

> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  cmds/device.c | 141 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 65 deletions(-)
> 
> diff --git a/cmds/device.c b/cmds/device.c
> index 7d3febff96c2..feffe9184726 100644
> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -577,6 +577,71 @@ static const char * const cmd_device_stats_usage[] = {
>  	NULL
>  };
> 

Documenting the return value seems valuable, since it has different
semantics for positive/negative

> +static int _print_device_stat_string(struct format_ctx *fctx,
> +		struct btrfs_ioctl_get_dev_stats *args, char *path, bool check)
> +{
> +	char *canonical_path = path_canonicalize(path);
> +	char devid_str[32];
> +	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 },
> +	};
> +	/*
> +	 * The plain text and json formats cannot be
> +	 * mapped directly in all cases and we have to switch
> +	 */
> +	const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> +
> +	/* 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;

I believe the old code didn't actually set err to ENOMEM in this case. I
assume this is an improvement, but figured it was worth noting.

> +		}
> +
> +		snprintf(canonical_path, 32, "devid:%llu", args->devid);
> +	}
> +	snprintf(devid_str, 32, "%llu", args->devid);
> +	fmt_print_start_group(fctx, NULL, JSON_TYPE_MAP);
> +	/* Plain text does not print device info */
> +	if (json) {
> +		fmt_print(fctx, "device", canonical_path);
> +		fmt_print(fctx, "devid", args->devid);
> +	}
> +
> +	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;
> +
> +		/* Own format due to [/dev/name].value */
> +		if (json) {
> +			fmt_print(fctx, dev_stats[j].name, args->values[stat_idx]);
> +		} else {
> +			printf("[%s].%-16s %llu\n", canonical_path, dev_stats[j].name,
> +					(unsigned long long)args->values[stat_idx]);
> +		}
> +		if (check && (args->values[stat_idx] > 0))
> +			err |= 64;

now that err starts at zero and gets returned, |= doesn't really do
anything here, compared to just =, does it?

> +	}
> +
> +	fmt_print_end_group(fctx, NULL);
> +	free(canonical_path);
> +
> +	return err;
> +}
> +
>  static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  {
>  	char *dev_path;
> @@ -586,7 +651,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  	int fdmnt;
>  	int i;
>  	int err = 0;
> -	int check = 0;
> +	bool check = false;
>  	__u64 flags = 0;
>  	DIR *dirstream = NULL;
>  	struct format_ctx fctx;
> @@ -606,7 +671,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> 
>  		switch (c) {
>  		case 'c':
> -			check = 1;
> +			check = true;
>  			break;
>  		case 'z':
>  			flags = BTRFS_DEV_STATS_RESET;
> @@ -656,70 +721,16 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
>  			error("device stats ioctl failed on %s: %m",
>  			      path);
>  			err |= 1;
> -		} else {
> -			char *canonical_path;
> -			char devid_str[32];
> -			int j;
> -			static const struct {
> -				const char name[32];
> -				u64 num;
> -			} 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 },
> -			};
> -			/*
> -			 * The plain text and json formats cannot be
> -			 * mapped directly in all cases and we have to switch
> -			 */
> -			const bool json = (bconf.output_format == CMD_FORMAT_JSON);
> -
> -			canonical_path = path_canonicalize(path);
> -
> -			/* No path when device is missing. */
> -			if (!canonical_path) {
> -				canonical_path = malloc(32);
> -				if (!canonical_path) {
> -					error("not enough memory for path buffer");
> -					goto out;
> -				}
> -				snprintf(canonical_path, 32,
> -					 "devid:%llu", args.devid);
> -			}
> -			snprintf(devid_str, 32, "%llu", args.devid);
> -			fmt_print_start_group(&fctx, NULL, JSON_TYPE_MAP);
> -			/* Plain text does not print device info */
> -			if (json) {
> -				fmt_print(&fctx, "device", canonical_path);
> -				fmt_print(&fctx, "devid", di_args[i].devid);
> -			}
> +			goto out;
> +		}
> 
> -			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> -				/* We got fewer items than we know */
> -				if (args.nr_items < dev_stats[j].num + 1)
> -					continue;
> -
> -				/* Own format due to [/dev/name].value */
> -				if (json) {
> -					fmt_print(&fctx, dev_stats[j].name,
> -						args.values[dev_stats[j].num]);
> -				} else {
> -					printf("[%s].%-16s %llu\n",
> -						canonical_path,
> -						dev_stats[j].name,
> -						(unsigned long long)
> -						args.values[dev_stats[j].num]);
> -				}
> -				if ((check == 1)
> -				    && (args.values[dev_stats[j].num] > 0))
> -					err |= 64;
> -			}
> -			fmt_print_end_group(&fctx, NULL);
> -			free(canonical_path);
> +		int err2 = _print_device_stat_string(&fctx, &args, path, check);
> +		if (err2) {
> +			if (err2 < 0) {
> +				err = err2;
> +				goto out;
> +			} else
> +				err |= err2;
>  		}
>  	}
> 
> --
> 2.17.1
> 

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

[Boris Burkov]

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
>