Skip to content

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Sep 5, 2025

Motivation and Context

If we call ddt_log_load() for legacy ddt, we will end up going into ddt_log_update_stats() and filling uninitialized value into ddo_dspace. This value will then get added to dedup_table_size during ddt_get_dedup_object_stats().

Fixes #17019

Description

How Has This Been Tested?

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)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • 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:

@behlendorf behlendorf requested a review from robn September 5, 2025 16:26
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

It looks like this is possible because ddt_log_load_one() returns 0 instead of ENOENT when the zap_lookup() for the ddt name fails. Then ddt_log_load() carries on and calls ddt_log_update_stats(). The proposed fix certainly does the job to resolve this, but it seems like allowing ENOENT to be returned would as well. @robn what do you think.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 5, 2025
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.

I would address it by making ddt_log_update_stats() to not use doi if dmu_object_info() return error.

If we call ddt_log_load() for legacy ddt, we will end up going into
ddt_log_update_stats() and filling uninitialized value into ddo_dspace.
This value will then get added to dedup_table_size during
ddt_get_dedup_object_stats().

Signed-off-by: Chunwei Chen <[email protected]>
Fixes openzfs#17019
@tuxoko
Copy link
Contributor Author

tuxoko commented Sep 5, 2025

Yeah, I added check for dmu_object_info.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 8, 2025
@behlendorf behlendorf merged commit e3c3e86 into openzfs:master Sep 8, 2025
53 of 57 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Sep 10, 2025
If we call ddt_log_load() for legacy ddt, we will end up going into
ddt_log_update_stats() and filling uninitialized value into ddo_dspace.
This value will then get added to dedup_table_size during
ddt_get_dedup_object_stats().

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Closes openzfs#17019
Closes openzfs#17699

Signed-off-by: Chunwei Chen <[email protected]>
Co-authored-by: Chunwei Chen <[email protected]>
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.

Exabyte dedup_table_size reported on ZFS 2.3 without fast dedup.
4 participants