-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Module Parameter Regarding Log Size Limit #12284
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this default value, what do we expect the performance impact to be? I'm just wondering if there's any cases where a user may not tune this for their workload, resulting in worse performance than prior to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a workload hits zfs_dirty_data_max limit first. You will not see any performance difference after this patch.
Can a workload hits zfs_wrlog_data_max limit before hit zfs_dirty_data_max?
Yes. If a workload involves write and then rewrite the same data space within the same txg, the zilog size will be greater than dirty data size. It will result in worse performance after this patch if it hits this limit.
I find most of this kind workloads are results of synthetic benchmarks. benchmark tool often writes and rewrites over a pre-allocated file.
In real world, database workload may be in this category. I have heard of put entire database inside memory (L1ARC). Repeatedly write without triggering zfs_dirty_data_sync_percent is one of the key feature. If that's the case, zfs_wrlog_data_max should be adjust to the size of slog device.
The reason to limit zilog size is described as following.
As mentioned above, if a workload can accumulate a huge size of zilog without hit zfs_dirty_data_sync_percent through repeatedly over-write, that could be tens GB big size zilog, once a fsync occurred, there is no way to write out this amount zilog data in a short period of time. That may cause system hangs.
The proposed new module parameter is more like a safety feature than a performance improvement.
I am open to change the initial value, such like 25% of the total available memory. I find sometimes zilog could overflow the memory if memory could not fit that amount of data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, makes sense to me. I'm not necessarily opposed to this default value, just wanted to ask.. I'm good as-is.