All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot
       [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
@ 2018-09-11  0:09 ` Jonathan Rajotte
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0 Jonathan Rajotte
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 9+ messages in thread
From: Jonathan Rajotte @ 2018-09-11  0:09 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

From: Jérémie Galarneau <jeremie.galarneau@efficios.com>

The stream lock is not taken when interacting with the kernel
metadata stream that is created at the time a snapshot is taken.

This was noticed while reviewing the code for an unrelated reason,
so there is no known problem caused by this. Nevertheless, this
is incorrect as the stream is globally visible in the consumer.

Moreover, the stream was not cleaned-up which can cause a leak
whenever a metadata snapshot fails.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/common/kernel-consumer/kernel-consumer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 87ade12fa..3455f827b 100644
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -336,7 +336,7 @@ end:
  *
  * Returns 0 on success, < 0 on error
  */
-int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
+static int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 		uint64_t relayd_id, struct lttng_consumer_local_data *ctx)
 {
 	int ret, use_relayd = 0;
@@ -355,11 +355,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 	if (!metadata_channel) {
 		ERR("Kernel snapshot metadata not found for key %" PRIu64, key);
 		ret = -1;
-		goto error;
+		goto error_no_channel;
 	}
 
 	metadata_stream = metadata_channel->metadata_stream;
 	assert(metadata_stream);
+	pthread_mutex_lock(&metadata_stream->lock);
 
 	/* Flag once that we have a valid relayd for the stream. */
 	if (relayd_id != (uint64_t) -1ULL) {
@@ -369,7 +370,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 	if (use_relayd) {
 		ret = consumer_send_relayd_stream(metadata_stream, path);
 		if (ret < 0) {
-			goto error;
+			goto error_snapshot;
 		}
 	} else {
 		ret = utils_create_stream_file(path, metadata_stream->name,
@@ -377,7 +378,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 				metadata_stream->tracefile_count_current,
 				metadata_stream->uid, metadata_stream->gid, NULL);
 		if (ret < 0) {
-			goto error;
+			goto error_snapshot;
 		}
 		metadata_stream->out_fd = ret;
 	}
@@ -390,7 +391,8 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 			if (ret_read != -EAGAIN) {
 				ERR("Kernel snapshot reading metadata subbuffer (ret: %zd)",
 						ret_read);
-				goto error;
+				ret = ret_read;
+				goto error_snapshot;
 			}
 			/* ret_read is negative at this point so we will exit the loop. */
 			continue;
@@ -415,11 +417,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
 	}
 
 	ret = 0;
-
+error_snapshot:
+	pthread_mutex_unlock(&metadata_stream->lock);
 	cds_list_del(&metadata_stream->send_node);
 	consumer_stream_destroy(metadata_stream, NULL);
 	metadata_channel->metadata_stream = NULL;
-error:
+error_no_channel:
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.17.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0
       [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot Jonathan Rajotte
@ 2018-09-11  0:09 ` Jonathan Rajotte
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending Jonathan Rajotte
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 9+ messages in thread
From: Jonathan Rajotte @ 2018-09-11  0:09 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

A value of zero for the metadata key indicate that metadata was never
created/pushed to the consumer.

This can occur in scenario were a tracker is present since metadata
might never be created/pushed.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/bin/lttng-sessiond/ust-app.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
index 5cf904939..52d1da787 100644
--- a/src/bin/lttng-sessiond/ust-app.c
+++ b/src/bin/lttng-sessiond/ust-app.c
@@ -5975,6 +5975,11 @@ int ust_app_snapshot_record(struct ltt_ust_session *usess,
 			struct buffer_reg_channel *reg_chan;
 			struct consumer_socket *socket;
 
+			if (!reg->registry->reg.ust->metadata_key) {
+				/* Skip since no metadata is present */
+				continue;
+			}
+
 			/* Get consumer socket to use to push the metadata.*/
 			socket = consumer_find_socket_by_bitness(reg->bits_per_long,
 					usess->consumer);
-- 
2.17.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending
       [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot Jonathan Rajotte
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0 Jonathan Rajotte
@ 2018-09-11  0:09 ` Jonathan Rajotte
  2018-09-11  0:09 ` [PATCH lttng-tools] Perform local data pending before checking data pending with relayd Jonathan Rajotte
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 9+ messages in thread
From: Jonathan Rajotte @ 2018-09-11  0:09 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

The live timer can hold the stream lock while sending empty beacon. An empty beacon
does not mean that data is still pending for the stream.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/common/consumer/consumer.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 0cfbf5c7c..66f10c780 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -3668,34 +3668,6 @@ error_nosignal:
 	}
 }
 
-/*
- * Try to lock the stream mutex.
- *
- * On success, 1 is returned else 0 indicating that the mutex is NOT lock.
- */
-static int stream_try_lock(struct lttng_consumer_stream *stream)
-{
-	int ret;
-
-	assert(stream);
-
-	/*
-	 * Try to lock the stream mutex. On failure, we know that the stream is
-	 * being used else where hence there is data still being extracted.
-	 */
-	ret = pthread_mutex_trylock(&stream->lock);
-	if (ret) {
-		/* For both EBUSY and EINVAL error, the mutex is NOT locked. */
-		ret = 0;
-		goto end;
-	}
-
-	ret = 1;
-
-end:
-	return ret;
-}
-
 /*
  * Search for a relayd associated to the session id and return the reference.
  *
@@ -3779,11 +3751,7 @@ int consumer_data_pending(uint64_t id)
 			ht->hash_fct(&id, lttng_ht_seed),
 			ht->match_fct, &id,
 			&iter.iter, stream, node_session_id.node) {
-		/* If this call fails, the stream is being used hence data pending. */
-		ret = stream_try_lock(stream);
-		if (!ret) {
-			goto data_pending;
-		}
+		pthread_mutex_lock(&stream->lock);
 
 		/*
 		 * A removed node from the hash table indicates that the stream has
-- 
2.17.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH lttng-tools] Perform local data pending before checking data pending with relayd
       [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
                   ` (2 preceding siblings ...)
  2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending Jonathan Rajotte
@ 2018-09-11  0:09 ` Jonathan Rajotte
  2018-09-19 15:52 ` [PATCH lttng-tools] Fix: double put on error path Jérémie Galarneau
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 9+ messages in thread
From: Jonathan Rajotte @ 2018-09-11  0:09 UTC (permalink / raw)
  To: lttng-dev; +Cc: jgalar

Performing the data pending check in two phases, local and network,
reduce the total number network operation needed.

Doing the local check first enable early return in cases where data is
still pending locally.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/common/consumer/consumer.c | 52 ++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 66f10c780..5a32c7e8b 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -3734,19 +3734,6 @@ int consumer_data_pending(uint64_t id)
 	/* Ease our life a bit */
 	ht = consumer_data.stream_list_ht;
 
-	relayd = find_relayd_by_session_id(id);
-	if (relayd) {
-		/* Send init command for data pending. */
-		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
-		ret = relayd_begin_data_pending(&relayd->control_sock,
-				relayd->relayd_session_id);
-		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
-		if (ret < 0) {
-			/* Communication error thus the relayd so no data pending. */
-			goto data_not_pending;
-		}
-	}
-
 	cds_lfht_for_each_entry_duplicate(ht->ht,
 			ht->hash_fct(&id, lttng_ht_seed),
 			ht->match_fct, &id,
@@ -3769,9 +3756,27 @@ int consumer_data_pending(uint64_t id)
 			}
 		}
 
-		/* Relayd check */
-		if (relayd) {
-			pthread_mutex_lock(&relayd->ctrl_sock_mutex);
+		pthread_mutex_unlock(&stream->lock);
+	}
+
+	relayd = find_relayd_by_session_id(id);
+	if (relayd) {
+		unsigned int is_data_inflight = 0;
+
+		/* Send init command for data pending. */
+		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
+		ret = relayd_begin_data_pending(&relayd->control_sock,
+				relayd->relayd_session_id);
+		if (ret < 0) {
+			pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
+			/* Communication error thus the relayd so no data pending. */
+			goto data_not_pending;
+		}
+
+		cds_lfht_for_each_entry_duplicate(ht->ht,
+				ht->hash_fct(&id, lttng_ht_seed),
+				ht->match_fct, &id,
+				&iter.iter, stream, node_session_id.node) {
 			if (stream->metadata_flag) {
 				ret = relayd_quiescent_control(&relayd->control_sock,
 						stream->relayd_stream_id);
@@ -3780,20 +3785,19 @@ int consumer_data_pending(uint64_t id)
 						stream->relayd_stream_id,
 						stream->next_net_seq_num - 1);
 			}
-			pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 			if (ret == 1) {
+				pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
 				pthread_mutex_unlock(&stream->lock);
 				goto data_pending;
 			}
+			if (ret < 0) {
+				pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
+				pthread_mutex_unlock(&stream->lock);
+				goto data_not_pending;
+			}
 		}
-		pthread_mutex_unlock(&stream->lock);
-	}
 
-	if (relayd) {
-		unsigned int is_data_inflight = 0;
-
-		/* Send init command for data pending. */
-		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
+		/* Send end command for data pending. */
 		ret = relayd_end_data_pending(&relayd->control_sock,
 				relayd->relayd_session_id, &is_data_inflight);
 		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
-- 
2.17.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH lttng-tools] Fix: double put on error path
       [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
                   ` (3 preceding siblings ...)
  2018-09-11  0:09 ` [PATCH lttng-tools] Perform local data pending before checking data pending with relayd Jonathan Rajotte
@ 2018-09-19 15:52 ` Jérémie Galarneau
       [not found] ` <20180911000915.29177-5-jonathan.rajotte-julien@efficios.com>
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 9+ messages in thread
From: Jérémie Galarneau @ 2018-09-19 15:52 UTC (permalink / raw)
  To: Jonathan Rajotte; +Cc: lttng-dev, jgalar

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks!
Jérémie


On Mon, Sep 10, 2018 at 08:09:11PM -0400, Jonathan Rajotte wrote:
> Let relay_index_try_flush be responsible for the self-reference put on
> error path.
> 
> Code flow of relay_index_try_flush is a bit tricky but the only error
> flow (via relay_index_file_write) will always mark the index as flushed
> and perform the self-reference put.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/bin/lttng-relayd/main.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/bin/lttng-relayd/main.c b/src/bin/lttng-relayd/main.c
> index 5a3bcb43f..2e2f5839f 100644
> --- a/src/bin/lttng-relayd/main.c
> +++ b/src/bin/lttng-relayd/main.c
> @@ -2318,8 +2318,11 @@ static int relay_recv_index(const struct lttcomm_relayd_hdr *recv_hdr,
>  		/* no flush. */
>  		ret = 0;
>  	} else {
> +		/*
> +		 * relay_index_try_flush is responsible for the self-reference
> +		 * put of the index object on error.
> +		 */
>  		ERR("relay_index_try_flush error %d", ret);
> -		relay_index_put(index);
>  		ret = -1;
>  	}
>  
> @@ -3217,9 +3220,11 @@ static int handle_index_data(struct relay_stream *stream, uint64_t net_seq_num,
>  		/* No flush. */
>  		ret = 0;
>  	} else {
> -		/* Put self-ref for this index due to error. */
> -		relay_index_put(index);
> -		index = NULL;
> +		/*
> +		 * relay_index_try_flush is responsible for the self-reference
> +		 * put of the index object on error.
> +		 */
> +		ERR("relay_index_try_flush error %d", ret);
>  		ret = -1;
>  	}
>  end:
> -- 
> 2.17.1
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH lttng-tools] Perform local data pending before checking data pending with relayd
       [not found] ` <20180911000915.29177-5-jonathan.rajotte-julien@efficios.com>
@ 2018-09-19 15:52   ` Jérémie Galarneau
  0 siblings, 0 replies; 9+ messages in thread
From: Jérémie Galarneau @ 2018-09-19 15:52 UTC (permalink / raw)
  To: Jonathan Rajotte; +Cc: lttng-dev, jgalar

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks,
Jérémie

On Mon, Sep 10, 2018 at 08:09:15PM -0400, Jonathan Rajotte wrote:
> Performing the data pending check in two phases, local and network,
> reduce the total number network operation needed.
> 
> Doing the local check first enable early return in cases where data is
> still pending locally.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/consumer/consumer.c | 52 ++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
> index 66f10c780..5a32c7e8b 100644
> --- a/src/common/consumer/consumer.c
> +++ b/src/common/consumer/consumer.c
> @@ -3734,19 +3734,6 @@ int consumer_data_pending(uint64_t id)
>  	/* Ease our life a bit */
>  	ht = consumer_data.stream_list_ht;
>  
> -	relayd = find_relayd_by_session_id(id);
> -	if (relayd) {
> -		/* Send init command for data pending. */
> -		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
> -		ret = relayd_begin_data_pending(&relayd->control_sock,
> -				relayd->relayd_session_id);
> -		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
> -		if (ret < 0) {
> -			/* Communication error thus the relayd so no data pending. */
> -			goto data_not_pending;
> -		}
> -	}
> -
>  	cds_lfht_for_each_entry_duplicate(ht->ht,
>  			ht->hash_fct(&id, lttng_ht_seed),
>  			ht->match_fct, &id,
> @@ -3769,9 +3756,27 @@ int consumer_data_pending(uint64_t id)
>  			}
>  		}
>  
> -		/* Relayd check */
> -		if (relayd) {
> -			pthread_mutex_lock(&relayd->ctrl_sock_mutex);
> +		pthread_mutex_unlock(&stream->lock);
> +	}
> +
> +	relayd = find_relayd_by_session_id(id);
> +	if (relayd) {
> +		unsigned int is_data_inflight = 0;
> +
> +		/* Send init command for data pending. */
> +		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
> +		ret = relayd_begin_data_pending(&relayd->control_sock,
> +				relayd->relayd_session_id);
> +		if (ret < 0) {
> +			pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
> +			/* Communication error thus the relayd so no data pending. */
> +			goto data_not_pending;
> +		}
> +
> +		cds_lfht_for_each_entry_duplicate(ht->ht,
> +				ht->hash_fct(&id, lttng_ht_seed),
> +				ht->match_fct, &id,
> +				&iter.iter, stream, node_session_id.node) {
>  			if (stream->metadata_flag) {
>  				ret = relayd_quiescent_control(&relayd->control_sock,
>  						stream->relayd_stream_id);
> @@ -3780,20 +3785,19 @@ int consumer_data_pending(uint64_t id)
>  						stream->relayd_stream_id,
>  						stream->next_net_seq_num - 1);
>  			}
> -			pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
>  			if (ret == 1) {
> +				pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
>  				pthread_mutex_unlock(&stream->lock);
>  				goto data_pending;
>  			}
> +			if (ret < 0) {
> +				pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
> +				pthread_mutex_unlock(&stream->lock);
> +				goto data_not_pending;
> +			}
>  		}
> -		pthread_mutex_unlock(&stream->lock);
> -	}
>  
> -	if (relayd) {
> -		unsigned int is_data_inflight = 0;
> -
> -		/* Send init command for data pending. */
> -		pthread_mutex_lock(&relayd->ctrl_sock_mutex);
> +		/* Send end command for data pending. */
>  		ret = relayd_end_data_pending(&relayd->control_sock,
>  				relayd->relayd_session_id, &is_data_inflight);
>  		pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
> -- 
> 2.17.1
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending
       [not found] ` <20180911000915.29177-4-jonathan.rajotte-julien@efficios.com>
@ 2018-09-19 15:53   ` Jérémie Galarneau
  0 siblings, 0 replies; 9+ messages in thread
From: Jérémie Galarneau @ 2018-09-19 15:53 UTC (permalink / raw)
  To: Jonathan Rajotte; +Cc: lttng-dev, jgalar

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks,
Jérémie

On Mon, Sep 10, 2018 at 08:09:14PM -0400, Jonathan Rajotte wrote:
> The live timer can hold the stream lock while sending empty beacon. An empty beacon
> does not mean that data is still pending for the stream.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/consumer/consumer.c | 34 +---------------------------------
>  1 file changed, 1 insertion(+), 33 deletions(-)
> 
> diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
> index 0cfbf5c7c..66f10c780 100644
> --- a/src/common/consumer/consumer.c
> +++ b/src/common/consumer/consumer.c
> @@ -3668,34 +3668,6 @@ error_nosignal:
>  	}
>  }
>  
> -/*
> - * Try to lock the stream mutex.
> - *
> - * On success, 1 is returned else 0 indicating that the mutex is NOT lock.
> - */
> -static int stream_try_lock(struct lttng_consumer_stream *stream)
> -{
> -	int ret;
> -
> -	assert(stream);
> -
> -	/*
> -	 * Try to lock the stream mutex. On failure, we know that the stream is
> -	 * being used else where hence there is data still being extracted.
> -	 */
> -	ret = pthread_mutex_trylock(&stream->lock);
> -	if (ret) {
> -		/* For both EBUSY and EINVAL error, the mutex is NOT locked. */
> -		ret = 0;
> -		goto end;
> -	}
> -
> -	ret = 1;
> -
> -end:
> -	return ret;
> -}
> -
>  /*
>   * Search for a relayd associated to the session id and return the reference.
>   *
> @@ -3779,11 +3751,7 @@ int consumer_data_pending(uint64_t id)
>  			ht->hash_fct(&id, lttng_ht_seed),
>  			ht->match_fct, &id,
>  			&iter.iter, stream, node_session_id.node) {
> -		/* If this call fails, the stream is being used hence data pending. */
> -		ret = stream_try_lock(stream);
> -		if (!ret) {
> -			goto data_pending;
> -		}
> +		pthread_mutex_lock(&stream->lock);
>  
>  		/*
>  		 * A removed node from the hash table indicates that the stream has
> -- 
> 2.17.1
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0
       [not found] ` <20180911000915.29177-3-jonathan.rajotte-julien@efficios.com>
@ 2018-09-19 15:53   ` Jérémie Galarneau
  0 siblings, 0 replies; 9+ messages in thread
From: Jérémie Galarneau @ 2018-09-19 15:53 UTC (permalink / raw)
  To: Jonathan Rajotte; +Cc: lttng-dev, jgalar

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks,
Jérémie

On Mon, Sep 10, 2018 at 08:09:13PM -0400, Jonathan Rajotte wrote:
> A value of zero for the metadata key indicate that metadata was never
> created/pushed to the consumer.
> 
> This can occur in scenario were a tracker is present since metadata
> might never be created/pushed.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/bin/lttng-sessiond/ust-app.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
> index 5cf904939..52d1da787 100644
> --- a/src/bin/lttng-sessiond/ust-app.c
> +++ b/src/bin/lttng-sessiond/ust-app.c
> @@ -5975,6 +5975,11 @@ int ust_app_snapshot_record(struct ltt_ust_session *usess,
>  			struct buffer_reg_channel *reg_chan;
>  			struct consumer_socket *socket;
>  
> +			if (!reg->registry->reg.ust->metadata_key) {
> +				/* Skip since no metadata is present */
> +				continue;
> +			}
> +
>  			/* Get consumer socket to use to push the metadata.*/
>  			socket = consumer_find_socket_by_bitness(reg->bits_per_long,
>  					usess->consumer);
> -- 
> 2.17.1
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot
       [not found] ` <20180911000915.29177-2-jonathan.rajotte-julien@efficios.com>
@ 2018-09-19 15:54   ` Jérémie Galarneau
  0 siblings, 0 replies; 9+ messages in thread
From: Jérémie Galarneau @ 2018-09-19 15:54 UTC (permalink / raw)
  To: Jonathan Rajotte; +Cc: lttng-dev, jgalar

Merged in master, stable-2.11, stable-2.10, and stable-2.9.

Thanks,
Jérémie

On Mon, Sep 10, 2018 at 08:09:12PM -0400, Jonathan Rajotte wrote:
> From: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> 
> The stream lock is not taken when interacting with the kernel
> metadata stream that is created at the time a snapshot is taken.
> 
> This was noticed while reviewing the code for an unrelated reason,
> so there is no known problem caused by this. Nevertheless, this
> is incorrect as the stream is globally visible in the consumer.
> 
> Moreover, the stream was not cleaned-up which can cause a leak
> whenever a metadata snapshot fails.
> 
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/kernel-consumer/kernel-consumer.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
> index 87ade12fa..3455f827b 100644
> --- a/src/common/kernel-consumer/kernel-consumer.c
> +++ b/src/common/kernel-consumer/kernel-consumer.c
> @@ -336,7 +336,7 @@ end:
>   *
>   * Returns 0 on success, < 0 on error
>   */
> -int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
> +static int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  		uint64_t relayd_id, struct lttng_consumer_local_data *ctx)
>  {
>  	int ret, use_relayd = 0;
> @@ -355,11 +355,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	if (!metadata_channel) {
>  		ERR("Kernel snapshot metadata not found for key %" PRIu64, key);
>  		ret = -1;
> -		goto error;
> +		goto error_no_channel;
>  	}
>  
>  	metadata_stream = metadata_channel->metadata_stream;
>  	assert(metadata_stream);
> +	pthread_mutex_lock(&metadata_stream->lock);
>  
>  	/* Flag once that we have a valid relayd for the stream. */
>  	if (relayd_id != (uint64_t) -1ULL) {
> @@ -369,7 +370,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	if (use_relayd) {
>  		ret = consumer_send_relayd_stream(metadata_stream, path);
>  		if (ret < 0) {
> -			goto error;
> +			goto error_snapshot;
>  		}
>  	} else {
>  		ret = utils_create_stream_file(path, metadata_stream->name,
> @@ -377,7 +378,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  				metadata_stream->tracefile_count_current,
>  				metadata_stream->uid, metadata_stream->gid, NULL);
>  		if (ret < 0) {
> -			goto error;
> +			goto error_snapshot;
>  		}
>  		metadata_stream->out_fd = ret;
>  	}
> @@ -390,7 +391,8 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  			if (ret_read != -EAGAIN) {
>  				ERR("Kernel snapshot reading metadata subbuffer (ret: %zd)",
>  						ret_read);
> -				goto error;
> +				ret = ret_read;
> +				goto error_snapshot;
>  			}
>  			/* ret_read is negative at this point so we will exit the loop. */
>  			continue;
> @@ -415,11 +417,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
>  	}
>  
>  	ret = 0;
> -
> +error_snapshot:
> +	pthread_mutex_unlock(&metadata_stream->lock);
>  	cds_list_del(&metadata_stream->send_node);
>  	consumer_stream_destroy(metadata_stream, NULL);
>  	metadata_channel->metadata_stream = NULL;
> -error:
> +error_no_channel:
>  	rcu_read_unlock();
>  	return ret;
>  }
> -- 
> 2.17.1
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-09-19 15:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20180911000915.29177-1-jonathan.rajotte-julien@efficios.com>
2018-09-11  0:09 ` [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot Jonathan Rajotte
2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0 Jonathan Rajotte
2018-09-11  0:09 ` [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending Jonathan Rajotte
2018-09-11  0:09 ` [PATCH lttng-tools] Perform local data pending before checking data pending with relayd Jonathan Rajotte
2018-09-19 15:52 ` [PATCH lttng-tools] Fix: double put on error path Jérémie Galarneau
     [not found] ` <20180911000915.29177-5-jonathan.rajotte-julien@efficios.com>
2018-09-19 15:52   ` [PATCH lttng-tools] Perform local data pending before checking data pending with relayd Jérémie Galarneau
     [not found] ` <20180911000915.29177-4-jonathan.rajotte-julien@efficios.com>
2018-09-19 15:53   ` [PATCH lttng-tools] Fix: Holding the stream lock does not equate to having data pending Jérémie Galarneau
     [not found] ` <20180911000915.29177-3-jonathan.rajotte-julien@efficios.com>
2018-09-19 15:53   ` [PATCH lttng-tools] Fix: Skip uid registry when metadata key is 0 Jérémie Galarneau
     [not found] ` <20180911000915.29177-2-jonathan.rajotte-julien@efficios.com>
2018-09-19 15:54   ` [PATCH lttng-tools] Fix: acquire stream lock during kernel metadata snapshot Jérémie Galarneau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.