Fix: coherent state not changed atomically with metadata written

commit 122c63cb4310 ("Fix: Implement RING_BUFFER_GET_NEXT_SUBBUF_METADATA_CHECK")
introduces a new ioctl which returns a flag indicating whether the
metadata is in consistent state at the end of the sub-buffer.

That commit is meant to address metadata consistency issues observable
in live sessions.

However, the "consistent" state is false as soon as a producer is
active (between an outermost metadata_begin/end pair). Unfortunately,
if the last "RING_BUFFER_GET_NEXT_SUBBUF_METADATA_CHECK" operation is
done between the last metadata printf and "end" of the transaction, the
last consistency state will be false, and the consumer daemon will never
send metadata to the relay daemon. This in turn causes a live viewer to
wait for metadata endlessly.

This issue can be reproduced by running lttng-tools:
tests/regression/tools/live/test_kernel

as root in a loop.

We observe two things:
1) the poll operation blocks when there is no more metadata to send,
   which means there is no mean to unblock when the consistency state
   changes back to "true" without producing additional metadata,

2) Even if (1) was fixed, the expectation from an ABI perspective is
   that the "coherent" state is only populated when
   RING_BUFFER_GET_NEXT_SUBBUF_METADATA_CHECK succeeds. Therefore,
   there is no way to let user-space know about conherency transition
   unless additional metadata is generated.

Fixing this requires to hold the metadata cache lock across the entire
production of a coherent metadata transaction. This simpler scheme is
possible because the metadata is generated in a reallocated memory area
and not directly into a ring buffer anymore. This was not the case in
earlier lttng-modules versions, when the metadata was generated directly
into a ring buffer, which explains why this simpler scheme was not
implemented.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/include/lttng/events.h b/include/lttng/events.h
index 605a48a5d3bdd16c7023678797ddefa0ae48b77f..4c376b395da8f9e22769ef5e17176aa23321d254 100644 (file)
--- a/include/lttng/events.h
+++ b/include/lttng/events.h
@@ -530,7 +530,7 @@ struct lttng_metadata_cache {
        char *data;                     /* Metadata cache */
        unsigned int cache_alloc;       /* Metadata allocated size (bytes) */
        unsigned int metadata_written;  /* Number of bytes written in metadata cache */
-       int producing;                  /* Metadata being produced (incomplete) */
+       atomic_t producing;             /* Metadata being produced (incomplete) */
        struct kref refcount;           /* Metadata cache usage */
        struct list_head metadata_stream;       /* Metadata stream list */
        uuid_le uuid;                   /* Trace session unique ID (copy) */

diff --git a/src/lttng-events.c b/src/lttng-events.c
index 3b9b907beb85196b36301cbb6df6456ab195b23b..2b72621cf2d6fccacaed54e3c8dde1159500ddb5 100644 (file)
--- a/src/lttng-events.c
+++ b/src/lttng-events.c
@@ -1673,7 +1673,7 @@ int lttng_metadata_output_channel(struct lttng_metadata_stream *stream,
                        reserve_len);
        stream->transport->ops.event_commit(&ctx);
        stream->metadata_in += reserve_len;
-       if (reserve_len < len || stream->metadata_cache->producing != 0)
+       if (reserve_len < len)
                stream->coherent = false;
        else
                stream->coherent = true;
@@ -1689,18 +1689,21 @@ end:
 static
 void lttng_metadata_begin(struct lttng_session *session)
 {
-       mutex_lock(&session->metadata_cache->lock);
-       session->metadata_cache->producing++;
-       mutex_unlock(&session->metadata_cache->lock);
+       if (atomic_inc_return(&session->metadata_cache->producing) == 1)
+               mutex_lock(&session->metadata_cache->lock);
 }
 
 static
 void lttng_metadata_end(struct lttng_session *session)
 {
-       mutex_lock(&session->metadata_cache->lock);
-       WARN_ON_ONCE(!session->metadata_cache->producing);
-       session->metadata_cache->producing--;
-       mutex_unlock(&session->metadata_cache->lock);
+       WARN_ON_ONCE(!atomic_read(&session->metadata_cache->producing));
+       if (atomic_dec_return(&session->metadata_cache->producing) == 0) {
+               struct lttng_metadata_stream *stream;
+
+               mutex_unlock(&session->metadata_cache->lock);
+               list_for_each_entry(stream, &session->metadata_cache->metadata_stream, list)
+                       wake_up_interruptible(&stream->read_wait);
+       }
 }
 
 /*
@@ -1717,7 +1720,6 @@ int lttng_metadata_printf(struct lttng_session *session,
        char *str;
        size_t len;
        va_list ap;
-       struct lttng_metadata_stream *stream;
 
        WARN_ON_ONCE(!READ_ONCE(session->active));
 
@@ -1728,8 +1730,7 @@ int lttng_metadata_printf(struct lttng_session *session,
                return -ENOMEM;
 
        len = strlen(str);
-       mutex_lock(&session->metadata_cache->lock);
-       session->metadata_cache->producing++;
+       WARN_ON_ONCE(!atomic_read(&session->metadata_cache->producing));
        if (session->metadata_cache->metadata_written + len >
                        session->metadata_cache->cache_alloc) {
                char *tmp_cache_realloc;
@@ -1755,18 +1756,11 @@ int lttng_metadata_printf(struct lttng_session *session,
                        session->metadata_cache->metadata_written,
                        str, len);
        session->metadata_cache->metadata_written += len;
-       session->metadata_cache->producing--;
-       mutex_unlock(&session->metadata_cache->lock);
        kfree(str);
 
-       list_for_each_entry(stream, &session->metadata_cache->metadata_stream, list)
-               wake_up_interruptible(&stream->read_wait);
-
        return 0;
 
 err:
-       session->metadata_cache->producing--;
-       mutex_unlock(&session->metadata_cache->lock);
        kfree(str);
        return -ENOMEM;
 }