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

test/fio: extend fio objectstore plugin to better simulate OSD behavior #19101

Merged
merged 2 commits into from Dec 5, 2017

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Nov 22, 2017

Signed-off-by: Igor Fedotov ifedotov@suse.com

@ifed01
Copy link
Contributor Author

ifed01 commented Nov 28, 2017

@cbodley - ping

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

looks good overall. it's great to be able to make jobs that are closer to actual osd workloads 👍

i worry a bit about the extra overhead in fio_ceph_os_queue() in the form of the pglog mutex and the allocation of buffers and map/set nodes, but at least we only pay for those when the specific features are enabled

assert(o->oi_attr_len_high > o->oi_attr_len_low);
// fill with the garbage as we do not care of the actual content...
attrset["_"] = buffer::create(o->oi_attr_len_low +
rand() % (o->oi_attr_len_high - o->oi_attr_len_low));
Copy link
Contributor

@cbodley cbodley Nov 28, 2017

Choose a reason for hiding this comment

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

see new include/random.h to replace the use of rand() % n:

auto len = ceph::util::generate_random_number(o->oi_attr_len_low, o->oi_attr_len_high);

that should also work in the case where o->oi_attr_len_low == o->oi_attr_len_high, so it could simplify a lot of this split logic

if (o->oi_attr_len_low == o->oi_attr_len_high) {
if (o->oi_attr_len_low) {
// fill with the garbage as we do not care of the actual content...
attrset["_"] = buffer::create(o->oi_attr_len_low);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider preallocating a single buffer in the Job that's big enough to fit all of these. that would avoid these extra buffer allocations for each io

@@ -11,6 +11,7 @@
#include <memory>
#include <system_error>
#include <vector>
#include <atomic>
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

ObjectStore::Sequencer sequencer;
// Can't use mutex directly in vectors hence dynamic allocation

ceph::shared_ptr<std::mutex> lock;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::unique_ptr will also work here (vector only requires it to be movable or copyable), and would save an extra allocation on construction

@ifed01
Copy link
Contributor Author

ifed01 commented Nov 30, 2017

@cbodley - fixed

@ifed01
Copy link
Contributor Author

ifed01 commented Dec 5, 2017

retest this please

@ifed01 ifed01 merged commit 559c54f into ceph:master Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants