* [PATCH 2/2] btrfs-progs: add support for tabular format for device stats
2022-07-18 11:34 [PATCH 1/2] btrfs-progs: factor out device stats printing code Nikolay Borisov
@ 2022-07-18 11:34 ` Nikolay Borisov
2022-07-18 16:55 ` David Sterba
2022-07-18 17:18 ` Boris Burkov
2022-07-18 16:53 ` [PATCH 1/2] btrfs-progs: factor out device stats printing code David Sterba
` (2 subsequent siblings)
3 siblings, 2 replies; 7+ messages in thread
From: Nikolay Borisov @ 2022-07-18 11:34 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] btrfs-progs: add support for tabular format for device stats
2022-07-18 11:34 ` [PATCH 2/2] btrfs-progs: add support for tabular format for device stats Nikolay Borisov
@ 2022-07-18 16:55 ` David Sterba
2022-07-18 17:18 ` Boris Burkov
1 sibling, 0 replies; 7+ messages in thread
From: David Sterba @ 2022-07-18 16:55 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] btrfs-progs: add support for tabular format for device stats
2022-07-18 11:34 ` [PATCH 2/2] btrfs-progs: add support for tabular format for device stats Nikolay Borisov
2022-07-18 16:55 ` David Sterba
@ 2022-07-18 17:18 ` Boris Burkov
1 sibling, 0 replies; 7+ messages in thread
From: Boris Burkov @ 2022-07-18 17:18 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code
2022-07-18 11:34 [PATCH 1/2] btrfs-progs: factor out device stats printing code Nikolay Borisov
2022-07-18 11:34 ` [PATCH 2/2] btrfs-progs: add support for tabular format for device stats Nikolay Borisov
@ 2022-07-18 16:53 ` David Sterba
2022-07-18 17:00 ` David Sterba
2022-07-18 17:11 ` Boris Burkov
3 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2022-07-18 16:53 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code
2022-07-18 11:34 [PATCH 1/2] btrfs-progs: factor out device stats printing code Nikolay Borisov
2022-07-18 11:34 ` [PATCH 2/2] btrfs-progs: add support for tabular format for device stats Nikolay Borisov
2022-07-18 16:53 ` [PATCH 1/2] btrfs-progs: factor out device stats printing code David Sterba
@ 2022-07-18 17:00 ` David Sterba
2022-07-18 17:11 ` Boris Burkov
3 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2022-07-18 17:00 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: factor out device stats printing code
2022-07-18 11:34 [PATCH 1/2] btrfs-progs: factor out device stats printing code Nikolay Borisov
` (2 preceding siblings ...)
2022-07-18 17:00 ` David Sterba
@ 2022-07-18 17:11 ` Boris Burkov
3 siblings, 0 replies; 7+ messages in thread
From: Boris Burkov @ 2022-07-18 17:11 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread