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

Jump to bottom

libzutil: allow to display powers of 1000 bytes #16579

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

libzutil: allow to display powers of 1000 bytes #16579

wants to merge 1 commit into from

Conversation

jcassette
Copy link

Motivation and Context

ZFS displays bytes with K/M/G/T/P/E prefixes. They represent powers of 1024 bytes, i.e. KiB, MiB, GiB, TiB, PiB, EiB.
Some users may want these prefixes to represent powers of 1000 bytes, i.e. KB, MB, GB, TB, PB, EB.

Description

This adds the new unit format and allows to use such display by defining an environment variable.

How Has This Been Tested?

Not tested. If this draft gathers interest then I will add tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@jcassette jcassette marked this pull request as draft September 28, 2024 13:18
@jcassette jcassette force-pushed the kbyte-1000 branch 3 times, most recently from 7516b6b to 89ef8f6 Compare September 28, 2024 14:31
mcmilk
mcmilk suggested changes Sep 29, 2024
View reviewed changes
Copy link
Contributor

@mcmilk mcmilk left a comment

Choose a reason for hiding this comment

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

There should also be a testcase for this new formatting.

lib/libzutil/zutil_nicenum.c

const int k_unit[] = { [ZFS_NICENUM_1024] = 1024,
[ZFS_NICENUM_BYTES] = 1024,
[ZFS_NICENUM_TIME] = 1000};
[ZFS_NICENUM_TIME] = 1000,
[ZFS_NICENUM_BYTES_1000] = 1000};
Copy link
Contributor

Choose a reason for hiding this comment

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

Would ZFS_NICENUM_1000 be a better fit?

Copy link
Author

@jcassette jcassette Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO it is not consistent with ZFS_NICENUM_1024, which uses an empty string for values less than 1024, whereas ZFS_NICENUM_BYTES uses "B" in that case.
Also, having a proper ZFS_NICENUM_1000 could be helpful for the concerns expressed by @tonyhutter ?
Maybe just ZFS_NICENUM_KBYTES?
Let me know and I will stick with what you choose.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ZFS_NICENUM_1000

lib/libzutil/zutil_nicenum.c
@@ -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_KB_IS_1000") != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You intruduce a new environment variable, but there is no documentation about it.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the man pages now.

@tonyhutter
Copy link
Contributor

This PR works when the value is a byte value, but there are other places where we use ZFS_NICENUM_1024 (perhaps incorrectly) that wont be affected. For example, the JSON code uses ZFS_NICENUM_1024 in an bunch of places:

zfs/cmd/zpool/zpool_main.c

Line 9177 in ca0141f

cb->cb_literal, cb->cb_json_as_int, ZFS_NICENUM_1024);

libzutil: allow to display powers of 1000 bytes
c8adac6 
ZFS displays bytes with K/M/G/T/P/E prefixes. They represent powers of
1024 bytes, i.e. KiB, MiB, GiB, TiB, PiB, EiB.
Some users may want these prefixes to represent powers of 1000 bytes,
i.e. KB, MB, GB, TB, PB, EB.
This adds the new unit format and allows to use such display by
defining an environment variable.

Signed-off-by: Julien Cassette <[email protected]>
@jcassette
Copy link
Author

jcassette commented Oct 9, 2024 •
edited
Loading

Hello and thanks for the feedback

@mcmilk :

There should also be a testcase for this new formatting.

Sure. Could you direct me to an existing test case that I could use as a base?

@tonyhutter :

This PR works when the value is a byte value, but there are other places where we use ZFS_NICENUM_1024 (perhaps incorrectly) that wont be affected. For example, the JSON code uses ZFS_NICENUM_1024 in an bunch of places:

Sorry I missed that.
I can see that ZFS_NICENUM_BYTES is used directly there. This PR does not work in that case currently, and I am not sure how to fix that.
If there are places where ZFS_NICENUM_1024 is used to represent bytes, could that be fixed in another PR by using ZFS_NICENUM_BYTES?

zfs/cmd/zpool/zpool_main.c

Lines 9176 to 9177 in ca0141f

nice_num_str_nvlist(vds, "read_errors", vs->vs_read_errors,
cb->cb_literal, cb->cb_json_as_int, ZFS_NICENUM_1024);

In this example, it seems weird to express a number of errors in powers of 1024. (I think nobody expects "100K errors" to be 100Ki = 102400 errors). Maybe a new unit like ZFS_NICENUM_1000 should be used instead?

@mcmilk
Copy link
Contributor

mcmilk commented Oct 9, 2024

There should also be a testcase for this new formatting.

@jcassette - Sorry, there is currently no such test case, I thought I had seen such test. So no - it seems that this is not needed. Sorry for the noise by me.

@tonyhutter
Copy link
Contributor

I suspect we historically went with the ambiguous K/M/G/T prefixes to get a more precision in the zpool iostat|status 5-char columns. That is, you could print "500.4M" rather then "500MB" or "500MiB" (which wouldn't even fit in 5 chars...)

One possible solution could consist of:

  1. ZFS_KB_IS_1000 envar - Default to 1000 instead of 1024 for byte values
  2. ZFS_USE_IEC_PREFIX envar - Print KB/MB/GB/TB (base 10) or KiB/MiB/GiB/TiB (base 2) instead of K/M/G/T. This might be a pain to implement due to the 5-char columns, but I suspect it would still be doable.
  3. Use ZFS_NICENUM_1000 for error counters in JSON output (and anywhere else its needed).

I'm fine with just number 1 being tacked in this PR, but you can try to fix the other ones if you want.

@jumbi77
Copy link
Contributor

jumbi77 commented Oct 9, 2024

In broader context referencing PR #14598

@tonyhutter
Copy link
Contributor

@jumbi77 thanks for the heads-up on that PR. I think we may need to be even more careful about ZFS_KB_IS_1000 since we've only talked about it for displaying numbers, not setting them. For example, this gets a little ambiguous:

export ZFS_KB_IS_1000=1
zfs set recordsize=8K tank

So we may want to rename the envar to something that wouldn't be ambiguous for those cases. Like include the word "display" or "output" in the envar name or something.

@amotin amotin added the Status: Work in Progress Not yet ready for general review label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyhutter tonyhutter tonyhutter left review comments

@mcmilk mcmilk mcmilk requested changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jcassette @tonyhutter @mcmilk @jumbi77 @amotin