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

os/bluestore: expand lttng tracepoints, improve fio_ceph_objectstore backend #29674

Merged
merged 10 commits into from
Sep 30, 2019

Conversation

athanatos
Copy link
Contributor

@athanatos athanatos commented Aug 15, 2019

Adds bluestore tracepoints for recording a variety of initial conditions for sampled ios including:

  • rocksdb properties like estimate-pending-compaction-bytes, num-running-compactions etc
  • current throttle values at IO initialization
  • duration of each bluestore stage

Also updates the fio_ceph_objectstore backend to optionally emit json formatted perf counters to a file as well as support for running multiple jobs and varying the throttle limits during a run.

The intention is that this functionality as well ceph/cbt#190 will enable somewhat quicker iteration on modifications to bluestore intended to improve throttle behavior or IO latency variability.

@athanatos
Copy link
Contributor Author

retest this please.

@athanatos athanatos changed the title RFC: os/bluestore: expand lttng tracepoints, improve fio_ceph_objectstore backend os/bluestore: expand lttng tracepoints, improve fio_ceph_objectstore backend Aug 21, 2019
@athanatos athanatos requested review from ifed01 and aclamk and removed request for tchaikov, jdurgin and rzarzynski August 21, 2019 20:29
@athanatos
Copy link
Contributor Author

retest this please

@athanatos athanatos force-pushed the sjust/wip-fio-trace branch 2 times, most recently from f8582cc to 8f572d9 Compare August 27, 2019 18:21
@tchaikov
Copy link
Contributor

@athanatos needs rebase.

@athanatos
Copy link
Contributor Author

@tchaikov rebased

@athanatos
Copy link
Contributor Author

Ok, I think I've addressed your comments. For the cmake bit, I added a cmakedefine for EVENTTRACE, made it depend on LTTNG, and cleaned up all of the code and cmake conditionals to only check EVENTTRACE.

@athanatos
Copy link
Contributor Author

@tchaikov ^

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/librbd/CMakeLists.txt Outdated Show resolved Hide resolved
src/msg/async/ProtocolV1.cc Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

please note, some of my comments are folded by GitHub web UI.

@athanatos
Copy link
Contributor Author

Sorry, I did indeed miss those.

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

apart from 3 nits, LGTM

}

template<typename Target, typename T>
static uint64_t to_microseconds(T t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/uint64_t/Rep/

@@ -503,6 +503,20 @@ struct converts_to_timespec<Clock, std::void_t<decltype(
template <typename Clock>
constexpr bool converts_to_timespec_v = converts_to_timespec<Clock>::value;

template<typename Target, typename T>
static double to_seconds(T t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/double/Rep/

src/os/bluestore/BlueStore.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@aclamk aclamk left a comment

Choose a reason for hiding this comment

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

fio_ceph_objectstore looks good.

Option("bluestore_throttle_trace_rate", Option::TYPE_FLOAT, Option::LEVEL_ADVANCED)
.set_default(0)
.set_description(""),

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these options contain 'debug'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. I've added a description though.

src/test/fio/fio_ceph_objectstore.cc Show resolved Hide resolved
src/test/fio/fio_ceph_objectstore.cc Show resolved Hide resolved
src/test/fio/fio_ceph_objectstore.cc Show resolved Hide resolved
@athanatos athanatos force-pushed the sjust/wip-fio-trace branch 3 times, most recently from a00a5af to ae1eead Compare September 26, 2019 19:09
@athanatos
Copy link
Contributor Author

retest this please

1 similar comment
@athanatos
Copy link
Contributor Author

retest this please

…to file

Signed-off-by: Samuel Just <sjust@redhat.com>
Because EVENTTRACE now implies LTTNG, also cleanup EVENTTRACE
conditions to assume LTTNG.

Also add missing eventtrace dependencies to rbd and
test/objectstore.

Signed-off-by: Samuel Just <sjust@redhat.com>
By nature, these tend to be extremely high volume.

Signed-off-by: Samuel Just <sjust@redhat.com>
Signed-off-by: Samuel Just <sjust@redhat.com>
Otherwise, it will behave incorrectly if enabled dynamically.

Signed-off-by: Samuel Just <sjust@redhat.com>
Only implemented for rocksdb for now.

Signed-off-by: Samuel Just <sjust@redhat.com>
…Throttle

This will make it easier to have a single place to emit trace information.

Signed-off-by: Samuel Just <sjust@redhat.com>
This patch adds per-io bluestore specific tracepoints detailing the
throttle state at queue time as well as state latencies during
execution.  Additionally, bluestore_throttle_trace_rate will limit
the rate at which ios have tracepoints emitted.

Signed-off-by: Samuel Just <sjust@redhat.com>
…ence/size

Signed-off-by: Samuel Just <sjust@redhat.com>
Signed-off-by: Samuel Just <sjust@redhat.com>
@athanatos
Copy link
Contributor Author

@athanatos athanatos merged commit 7984735 into ceph:master Sep 30, 2019
@athanatos athanatos deleted the sjust/wip-fio-trace branch September 30, 2019 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants