Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libzutil: allow to display powers of 1000 bytes #16579

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

libzutil: allow to display powers of 1000 bytes #16579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/libzutil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,15 @@ _LIBZUTIL_H boolean_t zfs_isnumber(const char *);
* ZFS_NICENUM_TIME: Print nanosecs, microsecs, millisecs, seconds...
* ZFS_NICENUM_RAW: Print the raw number without any formatting
* ZFS_NICENUM_RAWTIME: Same as RAW, but print dashes ('-') for zero.
* ZFS_NICENUM_1000: Same as ZFS_NICENUM_BYTES but use powers of 1000.
*/
enum zfs_nicenum_format {
ZFS_NICENUM_1024 = 0,
ZFS_NICENUM_BYTES = 1,
ZFS_NICENUM_TIME = 2,
ZFS_NICENUM_RAW = 3,
ZFS_NICENUM_RAWTIME = 4
ZFS_NICENUM_RAWTIME = 4,
ZFS_NICENUM_1000 = 5
};

/*
Expand Down
18 changes: 13 additions & 5 deletions lib/libzutil/zutil_nicenum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <ctype.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <libzutil.h>
#include <string.h>

Expand Down Expand Up @@ -64,19 +65,22 @@ zfs_nicenum_format(uint64_t num, char *buf, size_t buflen,
uint64_t n = num;
int index = 0;
const char *u;
const char *units[3][7] = {
const char *units[6][7] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be?

- const char *units[6][7]
+ const char *units[4][7]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because [ZFS_NICENUM_1000] = [5]

[ZFS_NICENUM_1024] = {"", "K", "M", "G", "T", "P", "E"},
[ZFS_NICENUM_BYTES] = {"B", "K", "M", "G", "T", "P", "E"},
[ZFS_NICENUM_TIME] = {"ns", "us", "ms", "s", "?", "?", "?"}
[ZFS_NICENUM_TIME] = {"ns", "us", "ms", "s", "?", "?", "?"},
[ZFS_NICENUM_1000] = {"B", "K", "M", "G", "T", "P", "E"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking ahead - can you rename ZFS_NICENUM_1000 to ZFS_NICENUM_BYTES_1000? That way we could potentially add a SI base 1000 prefix for non-byte values, like:

	    [ZFS_NICENUM_1000] = {"", "K", "M", "G", "T", "P", "E"}
	    [ZFS_NICENUM_BYTES_1000] = {"B", "K", "M", "G", "T", "P", "E"}

I can imagine in the future that we make all non-byte values SI powers of 1000 by default. It's kind of silly that we report zpool status error counters in powers of 1024, for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change the binary 1024 nicenum values to the binary SI prefix?

So [ZFS_NICENUM_1024] = {"", "K", "M", "G", "T", "P", "E"},
becomes [ZFS_NICENUM_1024] = {"", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei"}, ?

But a lot other code and maybe ZTS will need some changes with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the 5-char limit for nicenum values. You'd give up precision with 2-char SI names. Like, currently you can report "20.7M" but with the change you could only report "20Mi". I'm sure we could fix the 5-char limit, but I don't know what kind of fallout there would be from it. We've been reporting 5-char nicenum values for a long time...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, then we should work with the 5-char limit currently and introduce the environment variable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? You asked the opposite before: #16579 (comment)

};

const int units_len[] = {[ZFS_NICENUM_1024] = 6,
[ZFS_NICENUM_BYTES] = 6,
[ZFS_NICENUM_TIME] = 4};
[ZFS_NICENUM_TIME] = 4,
[ZFS_NICENUM_1000] = 6};

const int k_unit[] = { [ZFS_NICENUM_1024] = 1024,
[ZFS_NICENUM_BYTES] = 1024,
[ZFS_NICENUM_TIME] = 1000};
[ZFS_NICENUM_TIME] = 1000,
[ZFS_NICENUM_1000] = 1000};

double val;

Expand Down Expand Up @@ -180,5 +184,9 @@ zfs_niceraw(uint64_t num, char *buf, size_t buflen)
void
zfs_nicebytes(uint64_t num, char *buf, size_t buflen)
{
zfs_nicenum_format(num, buf, buflen, ZFS_NICENUM_BYTES);
if (getenv("ZFS_OUTPUT_BYTES_SI") != NULL) {
zfs_nicenum_format(num, buf, buflen, ZFS_NICENUM_1000);
} else {
zfs_nicenum_format(num, buf, buflen, ZFS_NICENUM_BYTES);
}
}
9 changes: 9 additions & 0 deletions man/man8/zdb.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,15 @@ refcnt blocks LSIZE PSIZE DSIZE blocks LSIZE PSIZE DSIZE
dedup = 1.11, compress = 1.80, copies = 1.00, dedup * compress / copies = 2.00
.Ed
.
.Sh ENVIRONMENT VARIABLES
.Bl -tag -width "ZFS_OUTPUT_BYTES_SI"
.\" Shared with zfs.8 and zpool.8
.It Sy ZFS_OUTPUT_BYTES_SI
Make K/M/G/T/P/E prefixes in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about rewording these lines to make it explicitly clear this is for byte values only?:

When printing byte values, make all prefixes (like KB, MB, GB, TB, PB, EB) 
represent powers of 1000, not 1024.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

.Nm zdb
output to represent powers of 1000 bytes, i.e. KB, MB, GB, TB, PB, EB.
.El
.
.Sh SEE ALSO
.Xr zfs 8 ,
.Xr zpool 8
6 changes: 6 additions & 0 deletions man/man8/zfs.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,12 @@ to set the maximum pipe size for sends/recieves.
Disabled by default on Linux
due to an unfixed deadlock in Linux's pipe size handling code.
.
.\" Shared with zdb.8 and zpool.8
.It Sy ZFS_OUTPUT_BYTES_SI
Make K/M/G/T/P/E prefixes in
.Nm zfs
output to represent powers of 1000 bytes, i.e. KB, MB, GB, TB, PB, EB.
.
.\" Shared with zpool.8
.It Sy ZFS_MODULE_TIMEOUT
Time, in seconds, to wait for
Expand Down
6 changes: 6 additions & 0 deletions man/man8/zpool.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,12 @@ If
.Sy ZPOOL_SCRIPTS_ENABLED
is not set, it is assumed that the user is allowed to run
.Nm zpool Cm status Ns / Ns Cm iostat Fl c .
.\" Shared with zdb.8 and zfs.8
.It Sy ZFS_OUTPUT_BYTES_SI
Make K/M/G/T/P/E prefixes in
.Nm zpool
output to represent powers of 1000 bytes, i.e. KB, MB, GB, TB, PB, EB.
.
.\" Shared with zfs.8
.It Sy ZFS_MODULE_TIMEOUT
Time, in seconds, to wait for
Expand Down