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

Fix alignment problems #3494

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

jbaublitz
Copy link
Member

@jbaublitz jbaublitz commented Nov 9, 2023

I think the alignment issue was introduced in #2854. See commit message for more details.

@jbaublitz jbaublitz self-assigned this Nov 9, 2023
@jbaublitz
Copy link
Member Author

/packit test

@jbaublitz
Copy link
Member Author

I want to verify that MAX_METADATA_SIZE would be 1 MiB aligned.

@jbaublitz
Copy link
Member Author

It is not. I'm going to add a round down function for the MAX_META_SIZE.

@jbaublitz
Copy link
Member Author

/packit test

Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

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

One small request and a question.

src/engine/strat_engine/cmd.rs Show resolved Hide resolved
src/engine/strat_engine/cmd.rs Outdated Show resolved Hide resolved
Currently, data allocations from the cap device are data block aligned.
Metadata allocations, however, are not. This can lead to data
allocations after a metadata allocation to become unaligned as well.
This has performance implications. This commit addresses this in a
backwards compatiable way by using our previous metadata size
calculation but rounds the total desired metadata device size up to the
nearest data block size multiple.

This has a few very desirable properties:
1. The metadata allocations will always be aligned to the data block
size boundary.
2. The metadata allocations may appear a bit larger, but the
overall growth of the step function tracks the linear growth of the
original metadata space calculation function.
3. While this commit does not resolve alignment issues for pools that
have already bumped into this problem, this code should theoretically
cause future allocations on affected pools to be in alignment,
minimizing performance impact for future allocations on affected pools.
@jbaublitz
Copy link
Member Author

/packit test

@jbaublitz
Copy link
Member Author

@mulkieran Do you want this to go in first or the transaction revert commit?

@mulkieran
Copy link
Member

@mulkieran Do you want this to go in first or the transaction revert commit?

I think this should go in first.

@mulkieran mulkieran merged commit 4c00298 into stratis-storage:master Nov 13, 2023
35 checks passed
@jbaublitz
Copy link
Member Author

Okay sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants