| Message ID | 20220718113439.2997247-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs-progs: factor out device stats printing code | expand |
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.
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 >
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; }
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(-) -- 2.17.1