nydusd: add the config support of amplify_io
#1452
Merged
Conversation
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
hangvane
requested review from
bergwolf,
liubin and
changweige
and removed request for
a team
imeoer
reviewed
Oct 24, 2023
hangvane
force-pushed
the
amplify_io
branch
2 times, most recently
from
3ea439d
to
760fbcb
Compare
imeoer
reviewed
Oct 25, 2023
Codecov Report
@@ Coverage Diff @@
## master #1452 +/- ##
==========================================
+ Coverage 62.49% 62.50% +0.01%
==========================================
Files 123 123
Lines 43131 43148 +17
Branches 43131 43148 +17
==========================================
+ Hits 26955 26970 +15
- Misses 14868 14870 +2
Partials 1308 1308
|
ccx1024cc
requested changes
Oct 30, 2023
|
@jiangliu PTAL. |
hangvane
force-pushed
the
amplify_io
branch
3 times, most recently
from
02c2c25
to
922a192
Compare
imeoer
reviewed
Nov 3, 2023
imeoer
reviewed
Nov 3, 2023
imeoer
reviewed
Nov 3, 2023
hangvane
force-pushed
the
amplify_io
branch
2 times, most recently
from
003ecc0
to
65a874b
Compare
imeoer
reviewed
Nov 6, 2023
imeoer
reviewed
Nov 6, 2023
hangvane
force-pushed
the
amplify_io
branch
2 times, most recently
from
2dbd1e7
to
88edb18
Compare
jiangliu
reviewed
Nov 9, 2023
api/src/config.rs
Outdated
| @@ -681,10 +681,10 @@ impl CacheConfigV2 { | |||
| } | |||
|
|
|||
| if self.prefetch.enable { | |||
| if self.prefetch.batch_size > 0x10000000 { | |||
| if self.prefetch.prefetch_request_batch_size > 0x10000000 { | |||
There was a problem hiding this comment.
It's an field in inside prefetch, so why adding prefetch_request prefix?
There was a problem hiding this comment.
There's at least three types of batch_size in nydus. Is it explicit enough to leave it as batch_size?
There was a problem hiding this comment.
Reverted to prefetch.batch_size. Please reply again if there's some other problems.
ccx1024cc
approved these changes
Nov 9, 2023
jiangliu
reviewed
Nov 9, 2023
| @@ -117,6 +117,10 @@ impl BlobFactory { | |||
| ) -> IOResult<Arc<dyn BlobCache>> { | |||
| let backend_cfg = config.get_backend_config()?; | |||
| let cache_cfg = config.get_cache_config()?; | |||
| let user_io_batch_size = config | |||
| .get_rafs_config() | |||
| .map_or_else(|_| default_user_io_batch_size(), |v| v.user_io_batch_size) | |||
There was a problem hiding this comment.
Should be?
.map_or_else(default_user_io_batch_size, |v| v.user_io_batch_size)
There was a problem hiding this comment.
function is expected to take 1 argument, but it takes 0 arguments
expected function that takes 1 argument
jiangliu
reviewed
Nov 9, 2023
storage/src/cache/cachedfile.rs
Outdated
| @@ -300,19 +301,19 @@ impl FileCacheEntry { | |||
| } | |||
| } | |||
|
|
|||
| fn prefetch_batch_size(&self) -> u64 { | |||
| if self.prefetch_config.merging_size < 0x2_0000 { | |||
| fn prefetch_request_batch_size(&self) -> u64 { | |||
There was a problem hiding this comment.
How about prefetch_batch_size()?
Variable names about prefetch are confused currently. So we merge variable names that have the same meaning, while DO NOT affect the field names read from the configuration file. Signed-off-by: Wenhao Ren <wenhaoren@mail.dlut.edu.cn>
Add the support of `amplify_io` in the config file of nydusd to configure read amplification. Signed-off-by: Wenhao Ren <wenhaoren@mail.dlut.edu.cn>
jiangliu
approved these changes
Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Relevant Issue (if applicable)
If there are Issues related to this PullRequest, please list it.
Details
Please describe the details of PullRequest.
The first commit is to rename the variables with similar usages into same names.
i.e.,
prefetch.batch_size/prefetch.merging_size->batch_size(filed name)/prefetch_batch_size(function name) (newly changed)threads/threads_count->threads_countbatch_size/amplify_io->user_io_batch_sizebandwidth_rate/bandwidth_limit->bandwidth_limitThis change is only for the variables in nydusd runtime, and does not affect the filed names in the config file.
The second commit adds the support of
amplify_ioin the config file of nydusd to restrict read amplification.Types of changes
What types of changes does your PullRequest introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply.