this was intended to have its own feature bit, rather than share a bit with other flags. the idea was to allow endpoints to interoperate if one was built with blkin support, and the other was not. but i'm not sure that's actually useful - if we're including blkin as a submodule, we might as well enable and package it by default

so i think we should get rid of the conditional CEPH_FEATURES_BLKIN stuff below, and just update the value of this bit. i'm guessing that we want to overlap with the KRAKEN bit (can @athanatos confirm?):

#define CEPH_FEATURE_BLKIN_TRACING CEPH_FEATURE_SERVER_KRAKEN

We can include this with the kraken feature bit if blkin will always be enabled. However, if we make blkin optional in the build there are potential issues with upgrades when some endpoints have blkin compiled in and others do not. E.g. a post-kraken osd without blkin and a kraken osd with blkin - one sends no tracing info, the other assumes tracing info is there based on the KRAKEN bit and can't decode the non-blkin message.

Keeping it as a separate feature bit that's only turned on in connections where it's compiled in, like this diff shows, seems to avoid this kind of problem.

@jdurgin okay, thanks!

@vears91 could you please update the value of CEPH_FEATURE_BLKIN_TRACING here to the first unused bit you can find?

should we move this under the ceph github org? I can create the repo there

https://github.com/ceph/blkin and https://github.com/ceph/babeltrace-zipkin are available now

This breaks the current API/ABI (e.g. an older app linked against this would not have put a null pointer on the stack). Same comment below.

Could it be fixed by adding an aio_operate_traced function in librados, and calling it from AioObjectRequest if there is trace information, or else defaulting to the regular aio_operate?

Since aio_operate is already overloaded many times over, you could just add a new overload which librbd would always call.

this journal_trace.init() should be conditional on if (osd_op->store_trace) instead of just if (osd_op), or we'll create separate traces that aren't attached to a parent

(the call to osd_op->mark_event() should remain outside of if (osd_op->store_trace))

as @dillaman pointed out, it's not safe to hold on to the trace info pointers like this

take a look at how Objecter and Op work for an example; they pass optional traces around by ZTracer::Trace*, and store them by value as ZTracer::Trace. we should only be using blkin_trace_info in our public interface (anything in src/include/rados/), then turning it into a Trace object as soon as we can

I wondered why ZTracer::Trace wasn't being used but I was told to pass the trace info to send_write_op. I will try to change it, can you please explain more about the issue with the trace info pointers? What problems can it cause?

looking at your aio_write_test_data_traced() test as an example, it's starting the rbd write and waiting for the completion at the same time:

void aio_write_test_data_traced(...) {
  struct blkin_trace_info trace_info;
  rbd_aio_write_traced(image, off, len, test_data, comp, &trace_info);
  rbd_aio_wait_for_complete(comp);

we're passing a pointer to a blkin_trace_info that's on the stack here. but it's not a problem, because the stack stays valid until the operation completes - this operation is basically synchronous. but consider an asynchronous case, where we start the operation in one function, and wait for it later in another:

void start_write(...) {
  struct blkin_trace_info trace_info;
  rbd_aio_write_traced(image, off, len, test_data, comp, &trace_info);
}
void wait_for_write(...) {
  rbd_aio_wait_for_complete(comp);
}

again we pass a pointer to blkin_trace_info on the stack, but this time it goes out of scope right after it starts the write. so if rbd holds on to this pointer and tries it dereference it later, it won't be pointing to valid memory. that's why we need to store a copy of the trace info instead of just the pointer