Fix: fd leak when closing metadata stream
authorDavid Goulet <dgoulet@efficios.com>
Thu, 6 Feb 2014 21:49:37 +0000 (16:49 -0500)
committerDavid Goulet <dgoulet@efficios.com>
Mon, 10 Feb 2014 21:13:41 +0000 (16:13 -0500)
Make sure to close the metadata poll pipe when the endpoint of the
stream dies out (ex: relayd stops unexpectedly). That was causing a fd
leak when we were destroying a metadata stream without using the close
metadata code path.

Signed-off-by: David Goulet <dgoulet@efficios.com>
src/common/consumer-stream.c
src/common/consumer.c
src/common/consumer.h
src/common/ust-consumer/ust-consumer.c
src/common/ust-consumer/ust-consumer.h

index 745de050d6dd4f7c805d37ec2f1221092fed17ac..5b52862c70a4a1d57fde1237e4f5e5ac6d1033cc 100644 (file)
@@ -122,7 +122,29 @@ void consumer_stream_close(struct lttng_consumer_stream *stream)
                break;
        case LTTNG_CONSUMER32_UST:
        case LTTNG_CONSUMER64_UST:
+       {
+               /*
+                * Special case for the metadata since the wait fd is an internal pipe
+                * polled in the metadata thread.
+                */
+               if (stream->metadata_flag && stream->chan->monitor) {
+                       int rpipe = stream->ust_metadata_poll_pipe[0];
+
+                       /*
+                        * This will stop the channel timer if one and close the write side
+                        * of the metadata poll pipe.
+                        */
+                       lttng_ustconsumer_close_metadata(stream->chan);
+                       if (rpipe >= 0) {
+                               ret = close(rpipe);
+                               if (ret < 0) {
+                                       PERROR("closing metadata pipe write side");
+                               }
+                               stream->ust_metadata_poll_pipe[0] = -1;
+                       }
+               }
                break;
+       }
        default:
                ERR("Unknown consumer_data type");
                assert(0);
@@ -195,9 +217,11 @@ void consumer_stream_delete(struct lttng_consumer_stream *stream,
 
        rcu_read_unlock();
 
-       /* Decrement the stream count of the global consumer data. */
-       assert(consumer_data.stream_count > 0);
-       consumer_data.stream_count--;
+       if (!stream->metadata_flag) {
+               /* Decrement the stream count of the global consumer data. */
+               assert(consumer_data.stream_count > 0);
+               consumer_data.stream_count--;
+       }
 }
 
 /*
index b942778b32e9a768f7da0029507b52af10478804..9d5a36970551fd19a5cb2288ca15026f1bf8dc70 100644 (file)
@@ -225,16 +225,6 @@ struct lttng_consumer_channel *consumer_find_channel(uint64_t key)
        return channel;
 }
 
-static void free_stream_rcu(struct rcu_head *head)
-{
-       struct lttng_ht_node_u64 *node =
-               caa_container_of(head, struct lttng_ht_node_u64, head);
-       struct lttng_consumer_stream *stream =
-               caa_container_of(node, struct lttng_consumer_stream, node);
-
-       free(stream);
-}
-
 static void free_channel_rcu(struct rcu_head *head)
 {
        struct lttng_ht_node_u64 *node =
@@ -1956,7 +1946,7 @@ int lttng_consumer_recv_cmd(struct lttng_consumer_local_data *ctx,
        }
 }
 
-void lttng_consumer_close_metadata(void)
+void lttng_consumer_close_all_metadata(void)
 {
        switch (consumer_data.type) {
        case LTTNG_CONSUMER_KERNEL:
@@ -1974,7 +1964,7 @@ void lttng_consumer_close_metadata(void)
                 * because at this point we are sure that the metadata producer is
                 * either dead or blocked.
                 */
-               lttng_ustconsumer_close_metadata(metadata_ht);
+               lttng_ustconsumer_close_all_metadata(metadata_ht);
                break;
        default:
                ERR("Unknown consumer_data type");
@@ -1988,10 +1978,7 @@ void lttng_consumer_close_metadata(void)
 void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
                struct lttng_ht *ht)
 {
-       int ret;
-       struct lttng_ht_iter iter;
        struct lttng_consumer_channel *free_chan = NULL;
-       struct consumer_relayd_sock_pair *relayd;
 
        assert(stream);
        /*
@@ -2002,96 +1989,17 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
 
        DBG3("Consumer delete metadata stream %d", stream->wait_fd);
 
-       if (ht == NULL) {
-               /* Means the stream was allocated but not successfully added */
-               goto free_stream_rcu;
-       }
-
        pthread_mutex_lock(&consumer_data.lock);
        pthread_mutex_lock(&stream->chan->lock);
        pthread_mutex_lock(&stream->lock);
 
-       switch (consumer_data.type) {
-       case LTTNG_CONSUMER_KERNEL:
-               if (stream->mmap_base != NULL) {
-                       ret = munmap(stream->mmap_base, stream->mmap_len);
-                       if (ret != 0) {
-                               PERROR("munmap metadata stream");
-                       }
-               }
-               if (stream->wait_fd >= 0) {
-                       ret = close(stream->wait_fd);
-                       if (ret < 0) {
-                               PERROR("close kernel metadata wait_fd");
-                       }
-               }
-               break;
-       case LTTNG_CONSUMER32_UST:
-       case LTTNG_CONSUMER64_UST:
-               if (stream->monitor) {
-                       /* close the write-side in close_metadata */
-                       ret = close(stream->ust_metadata_poll_pipe[0]);
-                       if (ret < 0) {
-                               PERROR("Close UST metadata read-side poll pipe");
-                       }
-               }
-               lttng_ustconsumer_del_stream(stream);
-               break;
-       default:
-               ERR("Unknown consumer_data type");
-               assert(0);
-               goto end;
-       }
-
-       rcu_read_lock();
-       iter.iter.node = &stream->node.node;
-       ret = lttng_ht_del(ht, &iter);
-       assert(!ret);
+       /* Remove any reference to that stream. */
+       consumer_stream_delete(stream, ht);
 
-       iter.iter.node = &stream->node_channel_id.node;
-       ret = lttng_ht_del(consumer_data.stream_per_chan_id_ht, &iter);
-       assert(!ret);
-
-       iter.iter.node = &stream->node_session_id.node;
-       ret = lttng_ht_del(consumer_data.stream_list_ht, &iter);
-       assert(!ret);
-       rcu_read_unlock();
-
-       if (stream->out_fd >= 0) {
-               ret = close(stream->out_fd);
-               if (ret) {
-                       PERROR("close");
-               }
-       }
-
-       /* Check and cleanup relayd */
-       rcu_read_lock();
-       relayd = consumer_find_relayd(stream->net_seq_idx);
-       if (relayd != NULL) {
-               uatomic_dec(&relayd->refcount);
-               assert(uatomic_read(&relayd->refcount) >= 0);
-
-               /* Closing streams requires to lock the control socket. */
-               pthread_mutex_lock(&relayd->ctrl_sock_mutex);
-               ret = relayd_send_close_stream(&relayd->control_sock,
-                               stream->relayd_stream_id, stream->next_net_seq_num - 1);
-               pthread_mutex_unlock(&relayd->ctrl_sock_mutex);
-               if (ret < 0) {
-                       DBG("Unable to close stream on the relayd. Continuing");
-                       /*
-                        * Continue here. There is nothing we can do for the relayd.
-                        * Chances are that the relayd has closed the socket so we just
-                        * continue cleaning up.
-                        */
-               }
-
-               /* Both conditions are met, we destroy the relayd. */
-               if (uatomic_read(&relayd->refcount) == 0 &&
-                               uatomic_read(&relayd->destroy_flag)) {
-                       consumer_destroy_relayd(relayd);
-               }
-       }
-       rcu_read_unlock();
+       /* Close down everything including the relayd if one. */
+       consumer_stream_close(stream);
+       /* Destroy tracer buffers of the stream. */
+       consumer_stream_destroy_buffers(stream);
 
        /* Atomically decrement channel refcount since other threads can use it. */
        if (!uatomic_sub_return(&stream->chan->refcount, 1)
@@ -2100,11 +2008,10 @@ void consumer_del_metadata_stream(struct lttng_consumer_stream *stream,
                free_chan = stream->chan;
        }
 
-end:
        /*
         * Nullify the stream reference so it is not used after deletion. The
-        * channel lock MUST be acquired before being able to check for
-        * a NULL pointer value.
+        * channel lock MUST be acquired before being able to check for a NULL
+        * pointer value.
         */
        stream->chan->metadata_stream = NULL;
 
@@ -2116,8 +2023,7 @@ end:
                consumer_del_channel(free_chan);
        }
 
-free_stream_rcu:
-       call_rcu(&stream->node.head, free_stream_rcu);
+       consumer_stream_free(stream);
 }
 
 /*
@@ -2354,7 +2260,7 @@ restart:
 
                                        /* Add metadata stream to the global poll events list */
                                        lttng_poll_add(&events, stream->wait_fd,
-                                                       LPOLLIN | LPOLLPRI);
+                                                       LPOLLIN | LPOLLPRI | LPOLLHUP);
                                }
 
                                /* Handle other stream */
@@ -3174,7 +3080,7 @@ end:
         *
         * NOTE: for now, this only applies to the UST tracer.
         */
-       lttng_consumer_close_metadata();
+       lttng_consumer_close_all_metadata();
 
        /*
         * when all fds have hung up, the polling thread
index a7517a34083db737d9be2b2b51b61ddb3a60eb36..84ef2719d47e55eb7abc8626cdf37b3a11da525c 100644 (file)
@@ -622,6 +622,8 @@ ssize_t lttng_consumer_on_read_subbuffer_splice(
 int lttng_consumer_take_snapshot(struct lttng_consumer_stream *stream);
 int lttng_consumer_get_produced_snapshot(struct lttng_consumer_stream *stream,
                unsigned long *pos);
+int lttng_ustconsumer_get_wakeup_fd(struct lttng_consumer_stream *stream);
+int lttng_ustconsumer_close_wakeup_fd(struct lttng_consumer_stream *stream);
 void *consumer_thread_metadata_poll(void *data);
 void *consumer_thread_data_poll(void *data);
 void *consumer_thread_sessiond_poll(void *data);
index bdabb5e49fcda0e44d6c65018efc0b4f296c6730..34344f4518eeff43b6bd73675bf906635f0531b5 100644 (file)
@@ -629,45 +629,6 @@ error:
        rcu_read_unlock();
        return ret;
 }
-/*
- * Close metadata stream wakeup_fd using the given key to retrieve the channel.
- * RCU read side lock MUST be acquired before calling this function.
- *
- * NOTE: This function does NOT take any channel nor stream lock.
- *
- * Return 0 on success else LTTng error code.
- */
-static int _close_metadata(struct lttng_consumer_channel *channel)
-{
-       int ret = LTTCOMM_CONSUMERD_SUCCESS;
-
-       assert(channel);
-       assert(channel->type == CONSUMER_CHANNEL_TYPE_METADATA);
-
-       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);
-                       ret = LTTCOMM_CONSUMERD_ERROR_METADATA;
-               }
-
-               if (channel->monitor) {
-                       /* Close the read-side in consumer_del_metadata_stream */
-                       ret = close(channel->metadata_stream->ust_metadata_poll_pipe[1]);
-                       if (ret < 0) {
-                               PERROR("Close UST metadata write-side poll pipe");
-                               ret = LTTCOMM_CONSUMERD_ERROR_METADATA;
-                       }
-               }
-       }
-
-       return ret;
-}
 
 /*
  * Close metadata stream wakeup_fd using the given key to retrieve the channel.
@@ -702,7 +663,7 @@ static int close_metadata(uint64_t chan_key)
                goto error_unlock;
        }
 
-       ret = _close_metadata(channel);
+       lttng_ustconsumer_close_metadata(channel);
 
 error_unlock:
        pthread_mutex_unlock(&channel->lock);
@@ -1729,6 +1690,22 @@ void lttng_ustconsumer_del_stream(struct lttng_consumer_stream *stream)
        ustctl_destroy_stream(stream->ustream);
 }
 
+int lttng_ustconsumer_get_wakeup_fd(struct lttng_consumer_stream *stream)
+{
+       assert(stream);
+       assert(stream->ustream);
+
+       return ustctl_stream_get_wakeup_fd(stream->ustream);
+}
+
+int lttng_ustconsumer_close_wakeup_fd(struct lttng_consumer_stream *stream)
+{
+       assert(stream);
+       assert(stream->ustream);
+
+       return ustctl_stream_close_wakeup_fd(stream->ustream);
+}
+
 /*
  * Populate index values of a UST stream. Values are set in big endian order.
  *
@@ -2134,6 +2111,45 @@ end:
        return ret;
 }
 
+/*
+ * Stop a given metadata channel timer if enabled and close the wait fd which
+ * is the poll pipe of the metadata stream.
+ *
+ * This MUST be called with the metadata channel acquired.
+ */
+void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata)
+{
+       int ret;
+
+       assert(metadata);
+       assert(metadata->type == CONSUMER_CHANNEL_TYPE_METADATA);
+
+       DBG("Closing metadata channel key %" PRIu64, metadata->key);
+
+       if (metadata->switch_timer_enabled == 1) {
+               consumer_timer_switch_stop(metadata);
+       }
+
+       if (!metadata->metadata_stream) {
+               goto end;
+       }
+
+       /*
+        * Closing write side so the thread monitoring the stream wakes up if any
+        * and clean the metadata stream.
+        */
+       if (metadata->metadata_stream->ust_metadata_poll_pipe[1] >= 0) {
+               ret = close(metadata->metadata_stream->ust_metadata_poll_pipe[1]);
+               if (ret < 0) {
+                       PERROR("closing metadata pipe write side");
+               }
+               metadata->metadata_stream->ust_metadata_poll_pipe[1] = -1;
+       }
+
+end:
+       return;
+}
+
 /*
  * Close every metadata stream wait fd of the metadata hash table. This
  * function MUST be used very carefully so not to run into a race between the
@@ -2143,7 +2159,7 @@ end:
  * producer so calling this is safe because we are assured that no state change
  * can occur in the metadata thread for the streams in the hash table.
  */
-void lttng_ustconsumer_close_metadata(struct lttng_ht *metadata_ht)
+void lttng_ustconsumer_close_all_metadata(struct lttng_ht *metadata_ht)
 {
        struct lttng_ht_iter iter;
        struct lttng_consumer_stream *stream;
@@ -2160,13 +2176,7 @@ void lttng_ustconsumer_close_metadata(struct lttng_ht *metadata_ht)
                health_code_update();
 
                pthread_mutex_lock(&stream->chan->lock);
-               /*
-                * Whatever returned value, we must continue to try to close everything
-                * so ignore it.
-                */
-               (void) _close_metadata(stream->chan);
-               DBG("Metadata wait fd %d and poll pipe fd %d closed", stream->wait_fd,
-                               stream->ust_metadata_poll_pipe[1]);
+               lttng_ustconsumer_close_metadata(stream->chan);
                pthread_mutex_unlock(&stream->chan->lock);
 
        }
index 2dfd2e43c3dcf47c8ab73f3728e343bc8038de93..ac29857b35cf7c681dbb10a22a6facd9ff7cc76f 100644 (file)
@@ -51,7 +51,8 @@ int lttng_ustctl_get_mmap_read_offset(struct lttng_consumer_stream *stream,
                unsigned long *off);
 void *lttng_ustctl_get_mmap_base(struct lttng_consumer_stream *stream);
 int lttng_ustconsumer_data_pending(struct lttng_consumer_stream *stream);
-void lttng_ustconsumer_close_metadata(struct lttng_ht *ht);
+void lttng_ustconsumer_close_all_metadata(struct lttng_ht *ht);
+void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata);
 void lttng_ustconsumer_close_stream_wakeup(struct lttng_consumer_stream *stream);
 int lttng_ustconsumer_recv_metadata(int sock, uint64_t key, uint64_t offset,
                uint64_t len, struct lttng_consumer_channel *channel,
@@ -162,7 +163,11 @@ void *lttng_ustctl_get_mmap_base(struct lttng_consumer_stream *stream)
        return NULL;
 }
 static inline
-void lttng_ustconsumer_close_metadata(struct lttng_ht *ht)
+void lttng_ustconsumer_close_all_metadata(struct lttng_ht *ht)
+{
+}
+static inline
+void lttng_ustconsumer_close_metadata(struct lttng_consumer_channel *metadata)
 {
 }
 static inline
This page took 0.033128 seconds and 5 git commands to generate.