From: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Date: Fri, 22 Nov 2019 20:08:15 +0000 (-0500)
Subject: Fix: consumerd: assert on null trace chunk on session restart
X-Git-Url: https://git.efficios.com/?a=commitdiff_plain;h=ce1aa6fe88ec86bb5e7910bfa59e8f6873647ed4;p=deliverable%2Flttng-tools.git

Fix: consumerd: assert on null trace chunk on session restart

The consumer daemon asserts on
`stream->net_seq_idx != (uint64_t) 1ULL || stream>trace_chunk'
when a session is re-started following a rotation.

Steps to reproduce, while an instrumented application is running:
$ lttng create
$ lttng enable-event -u -a
$ lttng start
$ lttng stop
$ lttng rotate
$ lttng start
-> assertion fails

The trace chunk of a stream can be null in this scenario as the
"create trace chunk" consumer command immediately sets the streams'
current trace chunk when they are transitioning from a "null" trace
chunk.

Since 1f4962443, a session restart will cause a rotation to occur from
"null" to the streams' new trace chunk. When this rotation occurs, the
channel and their streams are seen to be in the same trace chunk.
This is unexpected as this should only occur when transitioning from a
trace chunk to the "null" trace chunk (no output). In that case, the
streams are considered to have reached the point where they should
discard their current trace chunk to enter the "null trace chunk"
state (having no current output). This is exactly the opposite of
the situation here as streams are transitioning from a null to a
non-null trace chunk.

Hence, the immediate assignation of the trace chunk to the streams
should no longer be performed on creation of a trace chunk on the
consumer daemon. The streams transition from the "null" trace chunk to
a new trace chunk through the "rotate channel" command as is done for
all other rotation operations. Removing this bypass also ensures that
the rotation behaviour of both the consumer and relay daemons match.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 09f0f0983..1ea682f02 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -1014,13 +1014,6 @@ int lttng_consumer_channel_set_trace_chunk(
 		struct lttng_consumer_channel *channel,
 		struct lttng_trace_chunk *new_trace_chunk)
 {
-	int ret = 0;
-	const bool is_local_trace = channel->relayd_id == -1ULL;
-	bool update_stream_trace_chunk;
-	struct cds_lfht_iter iter;
-	struct lttng_consumer_stream *stream;
-	unsigned long channel_hash;
-
 	pthread_mutex_lock(&channel->lock);
 	if (channel->is_deleted) {
 		/*
@@ -1032,24 +1025,6 @@ int lttng_consumer_channel_set_trace_chunk(
 		 */
 		goto end;
 	}
-	/*
-	 * A stream can transition to a state where it and its channel
-	 * no longer belong to a trace chunk. For instance, this happens when
-	 * a session is rotated while it is inactive. After the rotation
-	 * of an inactive session completes, the channel and its streams no
-	 * longer belong to a trace chunk.
-	 *
-	 * However, if a session is stopped, rotated, and started again,
-	 * the session daemon will create a new chunk and send it to its peers.
-	 * In that case, the streams' transition to a new chunk can be performed
-	 * immediately.
-	 *
-	 * This trace chunk transition could also be performed lazily when
-	 * a buffer is consumed. However, creating the files here allows the
-	 * consumer daemon to report any creation error to the session daemon
-	 * and cause the start of the tracing session to fail.
-	 */
-	update_stream_trace_chunk = !channel->trace_chunk && new_trace_chunk;
 
 	/*
 	 * The acquisition of the reference cannot fail (barring
@@ -1065,59 +1040,9 @@ int lttng_consumer_channel_set_trace_chunk(
 
 	lttng_trace_chunk_put(channel->trace_chunk);
 	channel->trace_chunk = new_trace_chunk;
-	if (!is_local_trace || !new_trace_chunk) {
-		/* Not an error. */
-		goto end;
-	}
-
-	if (!update_stream_trace_chunk) {
-		goto end;
-	}
-
-	channel_hash = consumer_data.stream_per_chan_id_ht->hash_fct(
-			&channel->key, lttng_ht_seed);
-	rcu_read_lock();
-	cds_lfht_for_each_entry_duplicate(consumer_data.stream_per_chan_id_ht->ht,
-			channel_hash,
-			consumer_data.stream_per_chan_id_ht->match_fct,
-			&channel->key, &iter, stream, node_channel_id.node) {
-		bool acquired_reference, should_regenerate_metadata = false;
-
-		acquired_reference = lttng_trace_chunk_get(channel->trace_chunk);
-		assert(acquired_reference);
-
-		pthread_mutex_lock(&stream->lock);
-
-		/*
-		 * On a transition from "no-chunk" to a new chunk, a metadata
-		 * stream's content must be entirely dumped. This must occcur
-		 * _after_ the creation of the metadata stream's output files
-		 * as the consumption thread (not necessarily the one executing
-		 * this) may start to consume during the call to
-		 * consumer_metadata_stream_dump().
-		 */
-		should_regenerate_metadata =
-			stream->metadata_flag &&
-			!stream->trace_chunk && channel->trace_chunk;
-		stream->trace_chunk = channel->trace_chunk;
-		ret = consumer_stream_create_output_files(stream, true);
-		if (ret) {
-			pthread_mutex_unlock(&stream->lock);
-			goto end_rcu_unlock;
-		}
-		if (should_regenerate_metadata) {
-			ret = consumer_metadata_stream_dump(stream);
-		}
-		pthread_mutex_unlock(&stream->lock);
-		if (ret) {
-			goto end_rcu_unlock;
-		}
-	}
-end_rcu_unlock:
-	rcu_read_unlock();
 end:
 	pthread_mutex_unlock(&channel->lock);
-	return ret;
+	return 0;
 }
 
 /*