Skip to content

Fix caching of DDT log and BRT #17875

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
Closed
amotin wants to merge 2 commits into from
Closed

Fix caching of DDT log and BRT #17875

amotin wants to merge 2 commits into from

Conversation

@amotin
Copy link
Member

@amotin amotin commented Oct 27, 2025 •
edited
Loading

Both DDT log and BRT counters we read on pool import and then only append or overwrite in full blocks. We don't need them in DMU or ARC caches. Fortunately we have DMU_UNCACHEDIO for this now.

Even more we don't need DDT log and BRT in non-evictable metadata DMU caches, since it will likely never fit there, while block the cache from its original users. Since DMU_OT_IS_METADATA_CACHED() has no way to differentiate the new metadata types, mark BRT with storage type of DMU_OT_DDT_ZAP. As side effect it will also put it on dedup device, but that should actually be right.

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Oct 27, 2025
amotin added 2 commits October 29, 2025 15:51
@amotin
BRT: Round bv_entcount up to BRT_BLOCKSIZE
a2b074a 
Since we set bv_mos_brtvdev block size, and since we keep dirty
bitmap at the same granularity, we should keep the allocations
and writes done with.  Otherwise it makes the last block write
short, that will be odd once we implement writing of only dirty
blocks, but also requires read-modify-write on DMU layer.

Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
@amotin
Fix caching of DDT log and BRT
48807d7 
Both DDT log and BRT counters we read on pool import and then only
append or overwrite in full blocks.  We don't need them in DMU or
ARC caches.  Fortunately we have DMU_UNCACHEDIO for this now.

Even more we don't need BRT in non-evictable metadata DMU caches,
since it will likely never fit there, while block the cache from
its original users.  Since DMU_OT_IS_METADATA_CACHED() has no way
to differentiate the new metadata types, mark BRT with storage
type of DMU_OT_DDT_ZAP.  As side effect it will also put it on
dedup device, but that should actually be right.

Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
@amotin
Copy link
Member Author

amotin commented Oct 29, 2025

Rebased.

behlendorf
behlendorf approved these changes Oct 29, 2025
View reviewed changes
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.

Nice. Yeah, since we're always doing full block updates using DMU_UNCACHEDIO is clearly the thing to do.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 30, 2025
behlendorf pushed a commit that referenced this pull request Oct 30, 2025
@amotin @behlendorf
Fix caching of DDT log and BRT
ec268cd 
Both DDT log and BRT counters we read on pool import and then only
append or overwrite in full blocks.  We don't need them in DMU or
ARC caches.  Fortunately we have DMU_UNCACHEDIO for this now.

Even more we don't need BRT in non-evictable metadata DMU caches,
since it will likely never fit there, while block the cache from
its original users.  Since DMU_OT_IS_METADATA_CACHED() has no way
to differentiate the new metadata types, mark BRT with storage
type of DMU_OT_DDT_ZAP.  As side effect it will also put it on
dedup device, but that should actually be right.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes #17875
amotin added a commit to amotin/zfs that referenced this pull request Nov 8, 2025
@amotin
BRT: Fix ranges to blocks conversion math
8295aa4 
BRT_RANGESIZE_TO_NBLOCKS() takes number of ranges as its argument.
To get number of blocks we should multiply it by the entry size,
not divide by it, as it was due to missing parentheses.

Before openzfs#17875 this could cause small memory corruptions for vdevs
bigger than 64TB, but the change made the bug more noticeable.

Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
This was referenced Nov 8, 2025
Merged
Closed
behlendorf pushed a commit that referenced this pull request Nov 10, 2025
@amotin
BRT: Fix ranges to blocks conversion math
8aaed7d 
BRT_RANGESIZE_TO_NBLOCKS() takes number of ranges as its argument.
To get number of blocks we should multiply it by the entry size,
not divide by it, as it was due to missing parentheses.

Before #17875 this could cause small memory corruptions for vdevs
bigger than 64TB, but the change made the bug more noticeable.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes #17886
Closes #17915
@amotin amotin deleted the ddt_brt_caching branch November 10, 2025 23:58
@adamdmoss adamdmoss mentioned this pull request Nov 11, 2025
Open
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 12, 2025
@amotin @tonyhutter
BRT: Fix ranges to blocks conversion math
19b9d93 
BRT_RANGESIZE_TO_NBLOCKS() takes number of ranges as its argument.
To get number of blocks we should multiply it by the entry size,
not divide by it, as it was due to missing parentheses.

Before openzfs#17875 this could cause small memory corruptions for vdevs
bigger than 64TB, but the change made the bug more noticeable.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17886
Closes openzfs#17915
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 12, 2025
@amotin @tonyhutter
BRT: Fix ranges to blocks conversion math
5a77c19 
BRT_RANGESIZE_TO_NBLOCKS() takes number of ranges as its argument.
To get number of blocks we should multiply it by the entry size,
not divide by it, as it was due to missing parentheses.

Before openzfs#17875 this could cause small memory corruptions for vdevs
bigger than 64TB, but the change made the bug more noticeable.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17886
Closes openzfs#17915
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Nov 12, 2025
@amotin @behlendorf
BRT: Round bv_entcount up to BRT_BLOCKSIZE
178a8be 
Since we set bv_mos_brtvdev block size, and since we keep dirty
bitmap at the same granularity, we should keep the allocations
and writes done with.  Otherwise it makes the last block write
short, that will be odd once we implement writing of only dirty
blocks, but also requires read-modify-write on DMU layer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17875
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Nov 12, 2025
@amotin @behlendorf
Fix caching of DDT log and BRT
e3acd0a 
Both DDT log and BRT counters we read on pool import and then only
append or overwrite in full blocks.  We don't need them in DMU or
ARC caches.  Fortunately we have DMU_UNCACHEDIO for this now.

Even more we don't need BRT in non-evictable metadata DMU caches,
since it will likely never fit there, while block the cache from
its original users.  Since DMU_OT_IS_METADATA_CACHED() has no way
to differentiate the new metadata types, mark BRT with storage
type of DMU_OT_DDT_ZAP.  As side effect it will also put it on
dedup device, but that should actually be right.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17875
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Nov 12, 2025
@amotin @behlendorf
BRT: Fix ranges to blocks conversion math
e305c7d 
BRT_RANGESIZE_TO_NBLOCKS() takes number of ranges as its argument.
To get number of blocks we should multiply it by the entry size,
not divide by it, as it was due to missing parentheses.

Before openzfs#17875 this could cause small memory corruptions for vdevs
bigger than 64TB, but the change made the bug more noticeable.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <alexander.motin@TrueNAS.com>
Closes openzfs#17886
Closes openzfs#17915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@behlendorf behlendorf behlendorf approved these changes

Assignees

No one assigned

Labels

Status: Accepted Ready to integrate (reviewed, tested)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amotin @behlendorf