From: Jérémie Galarneau Date: Wed, 12 Jan 2022 20:48:00 +0000 (-0500) Subject: Fix: relayd: rotation failure for multi-domain session X-Git-Url: http://drtracing.org/?a=commitdiff_plain;h=c5c79321eb1937f3d208210365c512f4a186ec2a;p=deliverable%2Flttng-tools.git Fix: relayd: rotation failure for multi-domain session Observed issue ============== Rotating a multi-domain streaming session results in the following error: $ lttng rotate Waiting for rotation to complete... Error: Failed to retrieve rotation state. Meanwhile, the relay daemon logs indicate the following: DBG1 - 14:56:04.213163667 [265774/265778]: lttng_trace_chunk_rename_path from .tmp_new_chunk to (null) (in lttng_trace_chunk_rename_path_no_lock() at trace-chunk.cpp:759) PERROR - 14:56:04.213242941 [265774/265778]: Failed to move trace chunk directory ".tmp_new_chunk" to "20220112T145604-0500-1": No such file or directory (in lttng_trace_chunk_rename_path_no_lock() at trace-chunk.cpp:799) DBG1 - 14:56:04.213396931 [265774/265778]: aborting session 2 (in session_abort() at session.cpp:588) DBG1 - 14:56:04.213512198 [265774/265778]: Control connection closed with 22 (in relay_thread_close_connection() at main.cpp:3874) The 'abort' of session 2 here causes the kernel consumer to fail to consume subbuffers: Error: Relayd send index failed. Cleaning up relayd 3. Error: Error consuming subbuffer: (0) [...] Cause ===== Following the flow of execution in the relay daemon shows that different trace chunks are used by the two relay sessions that result from the streaming of a single multi-domain session. Both trace chunks "own" the same output directory. When a rotation is performed, the first trace chunk to be closed will move the directory. Then, the second trace chunk to be closed will attempt to do the same, failing to do so as seen in the relay daemon log. Solution ======== Using different trace chunk instances for relay sessions belonging to a single sessiond session goes against the intended use of the sessiond trace chunk registry. A sessiond trace chunk registry allows the relay daemon to share trace chunks used by different "relay sessions" when they were created for the same user-visible session daemon session. Tracing multiple domains (e.g. ust and kernel) results in per-domain relay sessions being created. Sharing trace chunks, and their output directory more specifically, is essential to properly implement session rotations. The sharing of output directory handles allows directory renames to be performed once and without races that would stem from from multiple renames. The reason why sessiond trace chunk registry returns different trace chunk instances for two relay sessions is that the wrong session `id` is used to publish trace chunks. The `id` that must be used to share trace chunks accross the relay sessions that belong to the same sessiond session is `id_sessiond`. `id_sessiond` is optional as it is only provided by consumers v2.11+. Otherwise, it is fine to use the relay session `id`: it is a unique id for a given session daemon instance and those consumers will not issue a session rotation (or clear) as the feature didn't exist. A reference counting bug revealed by this change is also fixed in the implementation of the sessiond trace chunk registry. When the trace chunk is first published, two references to the published chunks exist. One is taken by the registry while the other is being returned to the caller. In the use case of the relay daemon, the reference held by the registry itself is undesirable. We want the trace chunk to be removed from the registry as soon as it is not being used by the relay daemon (through a session or a stream). This differs from the behaviour of the consumer daemon which relies on an explicit command from the session daemon to release the registry's reference. In cases where the trace chunk had already been published, the reference belonging to the sessiond trace chunk registry instance has already been 'put' by the firt publication. We must simply return the published trace chunk with a reference taken on behalf of the caller. Known drawbacks =============== None. Change-Id: Ic33443b114a87574a1b26ac5ccb022e47f886ddd Signed-off-by: Jérémie Galarneau --- diff --git a/src/bin/lttng-relayd/main.cpp b/src/bin/lttng-relayd/main.cpp index d3f8b3c9e..942bfa4d5 100644 --- a/src/bin/lttng-relayd/main.cpp +++ b/src/bin/lttng-relayd/main.cpp @@ -2663,7 +2663,10 @@ static int relay_rotate_session_streams( */ next_trace_chunk = sessiond_trace_chunk_registry_get_chunk( sessiond_trace_chunk_registry, - session->sessiond_uuid, session->id, + session->sessiond_uuid, + conn->session->id_sessiond.is_set ? + conn->session->id_sessiond.value : + conn->session->id, rotate_streams.new_chunk_id.value); if (!next_trace_chunk) { char uuid_str[LTTNG_UUID_STR_LEN]; @@ -2885,7 +2888,9 @@ static int relay_create_trace_chunk(const struct lttcomm_relayd_hdr *recv_hdr, published_chunk = sessiond_trace_chunk_registry_publish_chunk( sessiond_trace_chunk_registry, conn->session->sessiond_uuid, - conn->session->id, + conn->session->id_sessiond.is_set ? + conn->session->id_sessiond.value : + conn->session->id, chunk); if (!published_chunk) { char uuid_str[LTTNG_UUID_STR_LEN]; @@ -2991,7 +2996,9 @@ static int relay_close_trace_chunk(const struct lttcomm_relayd_hdr *recv_hdr, chunk = sessiond_trace_chunk_registry_get_chunk( sessiond_trace_chunk_registry, conn->session->sessiond_uuid, - conn->session->id, + conn->session->id_sessiond.is_set ? + conn->session->id_sessiond.value : + conn->session->id, chunk_id); if (!chunk) { char uuid_str[LTTNG_UUID_STR_LEN]; diff --git a/src/bin/lttng-relayd/sessiond-trace-chunks.cpp b/src/bin/lttng-relayd/sessiond-trace-chunks.cpp index e0c46e34a..18ef586d0 100644 --- a/src/bin/lttng-relayd/sessiond-trace-chunks.cpp +++ b/src/bin/lttng-relayd/sessiond-trace-chunks.cpp @@ -360,6 +360,7 @@ struct lttng_trace_chunk *sessiond_trace_chunk_registry_publish_chunk( char uuid_str[LTTNG_UUID_STR_LEN]; char chunk_id_str[MAX_INT_DEC_LEN(typeof(chunk_id))] = "-1"; struct lttng_trace_chunk *published_chunk = NULL; + bool trace_chunk_already_published; lttng_uuid_to_str(sessiond_uuid, uuid_str); lttng_uuid_copy(key.sessiond_uuid, sessiond_uuid); @@ -394,20 +395,29 @@ struct lttng_trace_chunk *sessiond_trace_chunk_registry_publish_chunk( } published_chunk = lttng_trace_chunk_registry_publish_chunk( - element->trace_chunk_registry, session_id, new_chunk); + element->trace_chunk_registry, session_id, new_chunk, + &trace_chunk_already_published); /* - * At this point, two references to the published chunks exist. One - * is taken by the registry while the other is being returned to the - * caller. In the use case of the relay daemon, the reference held - * by the registry itself is undesirable. + * When the trace chunk is first published, two references to the + * published chunks exist. One is taken by the registry while the other + * is being returned to the caller. In the use case of the relay daemon, + * the reference held by the registry itself is undesirable. * * We want the trace chunk to be removed from the registry as soon * as it is not being used by the relay daemon (through a session * or a stream). This differs from the behaviour of the consumer * daemon which relies on an explicit command from the session * daemon to release the registry's reference. + * + * In cases where the trace chunk had already been published, + * the reference belonging to the sessiond trace chunk + * registry instance has already been 'put'. We simply return + * the published trace chunk with a reference taken on behalf of the + * caller. */ - lttng_trace_chunk_put(published_chunk); + if (!trace_chunk_already_published) { + lttng_trace_chunk_put(published_chunk); + } end: trace_chunk_registry_ht_element_put(element); return published_chunk; diff --git a/src/common/trace-chunk-registry.h b/src/common/trace-chunk-registry.h index 0eee0c21c..5442d7997 100644 --- a/src/common/trace-chunk-registry.h +++ b/src/common/trace-chunk-registry.h @@ -57,6 +57,29 @@ void lttng_trace_chunk_registry_destroy( struct lttng_trace_chunk *lttng_trace_chunk_registry_publish_chunk( struct lttng_trace_chunk_registry *registry, uint64_t session_id, struct lttng_trace_chunk *chunk); +/* + * Adds the `previously_published` parameter which allows the caller + * to know if a trace chunk equivalent to `chunk` was previously published. + * + * The registry holds a reference to the published trace chunks it contains. + * Trace chunks automatically unpublish themselves from their registry on + * destruction. + * + * This information is necessary to drop the reference of newly published + * chunks when a user doesn't wish to explicitly maintain all references + * to a given trace chunk. + * + * For instance, the relay daemon doesn't need the registry to hold a + * reference since it controls the lifetime of its trace chunks. + * Conversely, the consumer daemons rely on the session daemon to inform + * them of the end of life of a trace chunk and the trace chunks don't + * belong to a specific top-level object: they are always retrieved from + * the registry by `id`. + */ +struct lttng_trace_chunk *lttng_trace_chunk_registry_publish_chunk( + struct lttng_trace_chunk_registry *registry, + uint64_t session_id, struct lttng_trace_chunk *chunk, + bool *previously_published); /* * Look-up a trace chunk by session_id and chunk_id. diff --git a/src/common/trace-chunk.cpp b/src/common/trace-chunk.cpp index eed3d558b..ad7a983fb 100644 --- a/src/common/trace-chunk.cpp +++ b/src/common/trace-chunk.cpp @@ -1984,7 +1984,20 @@ end: struct lttng_trace_chunk * lttng_trace_chunk_registry_publish_chunk( struct lttng_trace_chunk_registry *registry, - uint64_t session_id, struct lttng_trace_chunk *chunk) + uint64_t session_id, + struct lttng_trace_chunk *chunk) +{ + bool unused; + + return lttng_trace_chunk_registry_publish_chunk( + registry, session_id, chunk, &unused); +} + +struct lttng_trace_chunk * +lttng_trace_chunk_registry_publish_chunk( + struct lttng_trace_chunk_registry *registry, + uint64_t session_id, struct lttng_trace_chunk *chunk, + bool *previously_published) { struct lttng_trace_chunk_registry_element *element; unsigned long element_hash; @@ -2019,6 +2032,7 @@ lttng_trace_chunk_registry_publish_chunk( element->registry = registry; /* Acquire a reference for the caller. */ if (lttng_trace_chunk_get(&element->chunk)) { + *previously_published = false; break; } else { /* @@ -2045,6 +2059,7 @@ lttng_trace_chunk_registry_publish_chunk( if (lttng_trace_chunk_get(published_chunk)) { lttng_trace_chunk_put(&element->chunk); element = published_element; + *previously_published = true; break; } /*