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

Pack zrlock_t by 8 bytes. #14317

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Pack zrlock_t by 8 bytes. #14317

merged 1 commit into from
Jan 5, 2023

Conversation

amotin
Copy link
Member

@amotin amotin commented Dec 23, 2022

On FreeBSD this reduces this structure size from 64 to 56 bytes. dnode_handle_t respectively reduces from 72 to 64 bytes. It sounds like a waste to need 72 bytes to be able to relocate 808 bytes of dnode_t, which relocation on FreeBSD is not even supported.

How Has This Been Tested?

Built and booted with and without debug.

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:

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Dec 23, 2022
@amotin amotin requested a review from behlendorf December 23, 2022 15:09
Copy link
Contributor

@ryao ryao left a comment

Choose a reason for hiding this comment

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

There are around 5 more instances of this in the codebase if I recall correctly. Clang’s static analyzer will report them if you turn on this checker:

https://clang.llvm.org/docs/analyzer/checkers.html#id72

I had meant to write patches, but I have enough higher priority reports to handle that I have yet to make time to write patches for these. Since this catches one of them, I figure that I should point out that there are more and they are relatively easy to find.

On FreeBSD this reduces this structure size from 64 to 56 bytes.
dnode_handle_t respectively reduces from 72 to 64 bytes. It sounds
like a waste to need 72 bytes to be able to relocate 808 bytes of
dnode_t, which relocation on FreeBSD is not even supported.

Signed-off-by:  Alexander Motin <[email protected]>
Sponsored by:   iXsystems, Inc.
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 5, 2023
@behlendorf behlendorf merged commit db832c4 into openzfs:master Jan 5, 2023
amotin added a commit to amotin/zfs that referenced this pull request Mar 2, 2023
On FreeBSD this reduces this structure size from 64 to 56 bytes.
dnode_handle_t respectively reduces from 72 to 64 bytes. It sounds
like a waste to need 72 bytes to be able to relocate 808 bytes of
dnode_t, which relocation on FreeBSD is not even supported.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Sponsored by:   iXsystems, Inc.
Closes openzfs#14317
behlendorf pushed a commit that referenced this pull request Mar 2, 2023
On FreeBSD this reduces this structure size from 64 to 56 bytes.
dnode_handle_t respectively reduces from 72 to 64 bytes. It sounds
like a waste to need 72 bytes to be able to relocate 808 bytes of
dnode_t, which relocation on FreeBSD is not even supported.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Sponsored by:   iXsystems, Inc.
Closes #14317
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
On FreeBSD this reduces this structure size from 64 to 56 bytes.
dnode_handle_t respectively reduces from 72 to 64 bytes. It sounds
like a waste to need 72 bytes to be able to relocate 808 bytes of
dnode_t, which relocation on FreeBSD is not even supported.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by:  Alexander Motin <[email protected]>
Sponsored by:   iXsystems, Inc.
Closes openzfs#14317
@amotin amotin deleted the zrlock branch October 9, 2023 13:23
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