-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move output IO throttler to IO queue level #2332
base: master
Are you sure you want to change the base?
Changes from 2 commits
497f4dd
33e1356
f2229aa
c2816ec
c618cdf
c34e6f1
8f1e3f9
505cfe7
d15fcc6
fe12391
679683f
f3a3afd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -527,7 +527,7 @@ sstring io_request::opname() const { | |
std::abort(); | ||
} | ||
|
||
const fair_group& get_fair_group(const io_queue& ioq, unsigned stream) { | ||
const shared_throttle& io_throttle(const io_queue& ioq, unsigned stream) { | ||
return ioq._group->_fgs[stream]; | ||
} | ||
|
||
|
@@ -588,8 +588,8 @@ io_queue::io_queue(io_group_ptr group, internal::io_sink& sink) | |
}); | ||
} | ||
|
||
fair_group::config io_group::make_fair_group_config(const io_queue::config& qcfg) noexcept { | ||
fair_group::config cfg; | ||
shared_throttle::config io_group::make_throttle_config(const io_queue::config& qcfg) noexcept { | ||
shared_throttle::config cfg; | ||
cfg.label = fmt::format("io-queue-{}", qcfg.devid); | ||
double min_weight = std::min(io_queue::read_request_base_count, qcfg.disk_req_write_to_read_multiplier); | ||
double min_size = std::min(io_queue::read_request_base_count, qcfg.disk_blocks_write_to_read_multiplier); | ||
|
@@ -609,7 +609,7 @@ io_group::io_group(io_queue::config io_cfg, unsigned nr_queues) | |
: _config(std::move(io_cfg)) | ||
, _allocated_on(this_shard_id()) | ||
{ | ||
auto fg_cfg = make_fair_group_config(_config); | ||
auto fg_cfg = make_throttle_config(_config); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: maybe we could also adjust the variable name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point |
||
_fgs.emplace_back(fg_cfg, nr_queues); | ||
if (_config.duplex) { | ||
_fgs.emplace_back(fg_cfg, nr_queues); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pulled the change to my local repository and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ACK, fails for me too |
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.
Minor: in
fair_queue.hh
the names of parameters of the constructor do not match the ones that are used here. In the header there isfair_queue(shared_throttle& shared, config cfg)
.Do we want to keep both places consistent?
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.
Yes, I will need to update header name too