Zfs: zpool list -v console output formatting

[devZer0]

Distribution Name | CentOS7
Distribution Version | 7.4.1708 (Core)
Linux Kernel | 3.10.0-693.17.1.el7.x86_64
Architecture | .x86_64
ZFS Version | 0.7.6-1
SPL Version | 0.7.6-1

Describe the problem you're observing

while commands like "zpool iostat -l -q -v 1" , "zpool status" or "zfs list"

produce nicely formatted output and all is properly column/tab aligned, "zpool list -v" output looks horribly broken

# zpool list -v
NAME SIZE ALLOC FREE EXPANDSZ FRAG CAP DEDUP HEALTH ALTROOT
timemachinepool 10,9T 2,58T 8,29T - 4% 23% 1.00x ONLINE -
raidz2 10,9T 2,58T 8,29T - 4% 23%
ata-WDC_WD2003FYYS-02W0B0_WD-WMAY00390617 - - - - - -
ata-WDC_WD2003FYYS-02W0B0_WD-WMAUR0553783 - - - - - -
ata-WDC_WD2003FYYS-02W0B0_WD-WMAUR0599953 - - - - - -
ata-WDC_WD20EFRX-68EUZN0_WD-WCC4M7VPE2A6 - - - - - -
ata-WDC_WD20EFRX-68EUZN0_WD-WCC4M7VPENJ5 - - - - - -
ata-ST32000542AS_9XW025NK - - - - - -

[devZer0]

even that one does not format correctly (the lines with the pool names)

[kingneutron]

Workaround (not 100% perfect but close) :

zpool list -v |column -t

NAME SIZE ALLOC FREE EXPANDSZ FRAG CAP DEDUP HEALTH ALTROOT
zmixed3 2.72T 1.63T 1.09T - 21% 59% 1.00x ONLINE -
mirror 2.72T 1.63T 1.09T - 21% 59%
ata-ST3000VN007
ata-WDC_WD30EURX
zredpool2 1.81T 1007G 849G - 21% 54% 1.00x ONLINE -
mirror 1.81T 1007G 849G - 21% 54%
ata-WDC_WD20EFRX
ata-WDC_WD20EFRX

[jgottula]

Hey.

This very same thing was bothering me quite a bit recently. I took a dive into zpool_main.c, and eventually realized what the problem was. I came up with a (probably rather hacky and not really pull-request-worthy) patch that makes the column alignment actually work properly so that it's, you know, readable.

(Skip to the end if you only care about the patch itself; explanationy stuff incoming.)

---

Commands like zpool status, zpool iostat, and zpool list each have a struct that they use to communicate information to the callback function that they use to print each individual line of their output. (I'm talking about status_cbdata_t, iostat_cbdata_t, list_cbdata_t, etc.) These structs are pretty similar: in particular, all of them have a cb_namewidth integer member.

Prior to iterating over the pool vdevs and triggering the callbacks that print the console output, the cb_namewidth value is set to the length of the longest name of all of the vdevs that are going to be printed. Then, each time the callback fires to output a line, it can access that value and use it to properly space-pad the vdev name when it's printed (which ensures that the next column will be lined up neatly).

The root of the problem is as follows. Unlike zpool_do_status or zpool_do_iostat, zpool_do_list never actually bothers to initialize the cb_namewidth value. The entire struct is initialized with {0}, though, so cb_namewidth is guaranteed to be 0. Which means that the printf's used in print_list_stats to print the vdev names will always have a zero width field parameter. (Well, actually, I think it will tend to be negative, not zero, due to the fact that strlen(name) and depth are subtracted from cb->cb_namewidth. It appears that, according to the C standard, if a negative integer is provided for the width parameter, then it's treated as if it was positive and the - left-align flag was in the format specifier. Wasn't actually aware of that particular little detail until I looked it up...)

---

Here's the patch I hacked together. (Featuring an additional .txt extension, "because GitHub".)
_(May 18 2018 update: I uploaded a slightly-revised patch. I think the original patch file I posted was actually a bit munged due to me manually editing it slightly, and so the patch utility was grumpy about actually applying it. No actual changes were made to the content of the patch though.)_
zpool-list-column-alignment-fix.patch.txt
zpool-list-column-alignment-fix-v2.patch.txt

Basically I just duplicated the get_namewidth function (used by the zpool iostat code), renamed it a bit, ripped out a couple things from it; and then put in a call to pool_list_iter in zpool_do_list so it gets called appropriately.

My patch does compile, and it "works for me". I don't know if it'll work correctly in every single situation with every possible pool configuration; or whether I'm doing things slightly incorrectly/non-optimally; and of course the whole "duplicating code and then renaming it" thing is rather dubious. (Unfortunately the iostat-specific function get_namewidth has a completely generic name, but is only suitable for iostat because it expects data to be an iostat_cbdata_t * pointer; so, short of refactoring that idiocy, the obvious lazy/fast choice was to just copy, paste, and modify.) So I'm not necessarily going to make a pull request out of this; but it's probably a reasonable base to start from if someone wants to do that.

---

Without the patch:

# zpool list -v
NAME   SIZE  ALLOC   FREE  EXPANDSZ   FRAG    CAP  DEDUP  HEALTH  ALTROOT
pool  13.6T  12.9T   694G         -     5%    95%  1.00x  ONLINE  -
  raidz2  13.6T  12.9T   694G         -     5%    95%
    JGPool2A      -      -      -         -      -      -
    JGPool2B      -      -      -         -      -      -
    JGPool2C      -      -      -         -      -      -
    JGPool2D      -      -      -         -      -      -
    JGPool2E      -      -      -         -      -      -
log      -      -      -         -      -      -
  mirror  3.97G   732K  3.97G         -     0%     0%
    ZIL_Samsung      -      -      -         -      -      -
    ZIL_Corsair      -      -      -         -      -      -
cache      -      -      -         -      -      -
  L2ARC_Samsung  26.0G  24.5G  1.50G         -     0%    94%
  L2ARC_Corsair  6.00G  5.98G  18.0M         -     0%    99%

With the patch:

# zpool list -v
NAME              SIZE  ALLOC   FREE  EXPANDSZ   FRAG    CAP  DEDUP  HEALTH  ALTROOT
pool             13.6T  13.0T   689G         -     5%    95%  1.00x  ONLINE  -
  raidz2         13.6T  13.0T   689G         -     5%    95%
    JGPool2A         -      -      -         -      -      -
    JGPool2B         -      -      -         -      -      -
    JGPool2C         -      -      -         -      -      -
    JGPool2D         -      -      -         -      -      -
    JGPool2E         -      -      -         -      -      -
log                  -      -      -         -      -      -
  mirror         3.97G   732K  3.97G         -     0%     0%
    ZIL_Samsung      -      -      -         -      -      -
    ZIL_Corsair      -      -      -         -      -      -
cache                -      -      -         -      -      -
  L2ARC_Samsung  26.0G  24.7G  1.26G         -     0%    95%
  L2ARC_Corsair  6.00G  5.97G  26.9M         -     0%    99%

I hope this helps.

[tonyhutter]

@jgottula are you planning on opening a pull request with your patch? I think it would be a useful fix.

[jgottula]

If anyone had trouble getting the patch I posted a few days ago to apply, I discovered that it was probably a bit munged. So I've uploaded a revised version that I actually bothered to test with the patch utility.
zpool-list-column-alignment-fix-v2.patch.txt

---

are you planning on opening a pull request with your patch? I think it would be a useful fix.

Possibly.

From looking through the code, it does feel like an area that sort of needs a general cleanup (renaming functions, reducing duplication of code), and if I simply dropped the patch in as-is, I'd frankly just be making that situation worse.

So I'm more inclined to leave this as a starting point for anyone else who's more familiar with the relevant code and wants to do a PR that "does it right", so to speak.

[devZer0]

still exists with 0.8.0-rc2

[behlendorf]

PR opened, see #8147 and please feel free to review.

[behlendorf]

@jgottula would you mind reviewing and testing the proposed fix for this in #8147.