From 73811eccc9599ebf62e5f5bee49039cecc25c3eb Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 28 May 2013 14:25:27 -0400 Subject: [PATCH] Fix: get consumer lock before closing/pushing metadata This is to ensure the validity of the metadata stream in the channel before closing or pushing the metadata on it. Fixes #536 Acked-by: Mathieu Desnoyers Signed-off-by: David Goulet --- src/common/consumer-metadata-cache.h | 2 ++ src/common/consumer.c | 7 ++++++ src/common/consumer.h | 1 + src/common/ust-consumer/ust-consumer.c | 33 ++++++++++++++++++++++---- 4 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/common/consumer-metadata-cache.h b/src/common/consumer-metadata-cache.h index 164f9eaae..b1a4dc2b7 100644 --- a/src/common/consumer-metadata-cache.h +++ b/src/common/consumer-metadata-cache.h @@ -43,6 +43,8 @@ struct consumer_metadata_cache { /* * Lock to update the metadata cache and push into the ring_buffer * (ustctl_write_metadata_to_channel). + * + * This is nested INSIDE the consumer_data lock. */ pthread_mutex_t lock; }; diff --git a/src/common/consumer.c b/src/common/consumer.c index b5fe98316..78b3f0799 100644 --- a/src/common/consumer.c +++ b/src/common/consumer.c @@ -1960,6 +1960,13 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream, } end: + /* + * Nullify the stream reference so it is not used after deletion. The + * consumer data lock MUST be acquired before being able to check for a + * NULL pointer value. + */ + stream->chan->metadata_stream = NULL; + pthread_mutex_unlock(&stream->lock); pthread_mutex_unlock(&consumer_data.lock); diff --git a/src/common/consumer.h b/src/common/consumer.h index 3726fd1e6..bf87609f3 100644 --- a/src/common/consumer.h +++ b/src/common/consumer.h @@ -222,6 +222,7 @@ struct lttng_consumer_stream { * Lock to use the stream FDs since they are used between threads. * * This is nested INSIDE the consumer_data lock. + * This is nested INSIDE the metadata cache lock. * This is nested OUTSIDE consumer_relayd_sock_pair lock. */ pthread_mutex_t lock; diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index e0280f148..652207297 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -558,6 +558,11 @@ int lttng_ustconsumer_push_metadata(struct lttng_consumer_channel *metadata, DBG("UST consumer writing metadata to channel %s", metadata->name); + if (!metadata->metadata_stream) { + ret = 0; + goto error; + } + assert(target_offset <= metadata->metadata_cache->max_offset); ret = ustctl_write_metadata_to_channel(metadata->uchan, metadata_str + target_offset, len); @@ -629,11 +634,17 @@ static int close_metadata(uint64_t chan_key) } pthread_mutex_lock(&consumer_data.lock); - if (!cds_lfht_is_node_deleted(&channel->node.node)) { - if (channel->switch_timer_enabled == 1) { - DBG("Deleting timer on metadata channel"); - consumer_timer_switch_stop(channel); - } + + if (cds_lfht_is_node_deleted(&channel->node.node)) { + goto error_unlock; + } + + if (channel->switch_timer_enabled == 1) { + DBG("Deleting timer on metadata channel"); + consumer_timer_switch_stop(channel); + } + + if (channel->metadata_stream) { ret = ustctl_stream_close_wakeup_fd(channel->metadata_stream->ustream); if (ret < 0) { ERR("UST consumer unable to close fd of metadata (ret: %d)", ret); @@ -728,6 +739,17 @@ int lttng_ustconsumer_recv_metadata(int sock, uint64_t key, uint64_t offset, goto end_free; } + /* + * XXX: The consumer data lock is acquired before calling metadata cache + * write which calls push metadata that MUST be protected by the consumer + * lock in order to be able to check the validity of the metadata stream of + * the channel. + * + * Note that this will be subject to change to better fine grained locking + * and ultimately try to get rid of this global consumer data lock. + */ + pthread_mutex_lock(&consumer_data.lock); + pthread_mutex_lock(&channel->metadata_cache->lock); ret = consumer_metadata_cache_write(channel, offset, len, metadata_str); if (ret < 0) { @@ -735,6 +757,7 @@ int lttng_ustconsumer_recv_metadata(int sock, uint64_t key, uint64_t offset, ret_code = LTTCOMM_CONSUMERD_ERROR_METADATA; } pthread_mutex_unlock(&channel->metadata_cache->lock); + pthread_mutex_unlock(&consumer_data.lock); while (consumer_metadata_cache_flushed(channel, offset + len)) { DBG("Waiting for metadata to be flushed"); -- 2.34.1