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.

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

zdb: show dedup table and log attributes #16755

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

robn
Copy link
Member

@robn robn commented Nov 14, 2024

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

Commenting on #16752, and realised we didn't have a way to definitely show what kind dedup options are in place.

Description

Extends dump_ddt() to show container config (version, flags) and log flags and other info. A bit more lowlevel detail as befits a debugger.

$ zdb -D tank
DDT-blake3: version=1 [FDT]; flags=0x03 [FLAT LOG]; rootobj=65
DDT-blake3-zap-duplicate: dspace=0xa3400; mspace=0x88000; entries=1888
DDT-blake3-zap-unique: dspace=0x121800; mspace=0x108000; entries=2968
DDT-log-blake3-0: flags=0x01 [FLUSHING]; obj=66; len=0x0; txg=649; entries=0
DDT-log-blake3-1: flags=0x00; obj=67; len=0x160000; txg=657; entries=360
$ zdb -D tank
DDT-blake3: version=0 [LEGACY]; flags=0x00; rootobj=1
DDT-blake3-zap-duplicate: dspace=0x6e800; mspace=0x48000; entries=1824
DDT-blake3-zap-unique: dspace=0xba400; mspace=0x88000; entries=2752

Goes on to show the histograms and etc as it did before, so something for everyone.

How Has This Been Tested?

Just eyeballing on various test dedup configs and load generators I have lying around.

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:

cmd/zdb/zdb.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 14, 2024
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

As was told in other thread, len, dspace and mspace should still better be decimal.

cmd/zdb/zdb.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added the Status: Revision Needed Changes are required for the PR to be accepted label Nov 23, 2024
@github-actions github-actions bot removed the Status: Revision Needed Changes are required for the PR to be accepted label Nov 25, 2024
@robn
Copy link
Member Author

robn commented Nov 25, 2024

Ok, repushed with decimal bytes. Surprising controversy! 😅

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

This makes me wish we had some 32bit build target.

cmd/zdb/zdb.c Outdated Show resolved Hide resolved
cmd/zdb/zdb.c Outdated Show resolved Hide resolved
There's interesting info in there that is going to help with
understanding dedup behaviour at any given moment.

Since this is a format change, tests that rely on that output have been
modified to match.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
Copy link
Member Author

robn commented Nov 26, 2024

istg it drives me mad that user and kernel code have different ideas of what %lu and %llu are.

@amotin
Copy link
Member

amotin commented Nov 26, 2024

istg it drives me mad that user and kernel code have different ideas of what %lu and %llu are.

Is there a place where they don't mean long unsigned and long long unsigned?

@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 26, 2024
@robn
Copy link
Member Author

robn commented Nov 26, 2024

istg it drives me mad that user and kernel code have different ideas of what %lu and %llu are.

Is there a place where they don't mean long unsigned and long long unsigned?

Fair, my grumble was not very specific. It's more that on Linux (at least), uint64_t is long unsigned int in the kernel, but long long unsigned int in userspace. In shared code, the compiler will complain about both %lu and %llu, one in the kernel build, one in the user build. That is very annoying, but at least a reminder to add a cast. In userspace-only code, we end up here.

I keep meaning to find out exactly why uint64_t is defined differently.

I suppose I have no real cause for complaint, %lu and %llu say what they are, and PRIu64 is there for exactly this reason. But also that's a hassle to remember to type out, including closing the quotes; the inline %abc is much easier. If -Wformat would outright reject any use of %lu with something that isn't explicitly long unsigned (ditto %llu) then it'd be fine; I'd quickly train the muscle memory to use PRIu64. When the compiler doesn't complain though, I don't notice.

@amotin amotin merged commit 0ffa6f3 into openzfs:master Nov 27, 2024
24 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Dec 3, 2024
There's interesting info in there that is going to help with
understanding dedup behavior at any given moment.

Since this is a format change, tests that rely on that output have been
modified to match.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#16755
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants