Fix: allow get/put subbuf in loop for metadata stream

data_pending check can trigger the following sequence:

- get_next_subbuf
- put_subbuf
- get_next_subbuf
- put_subbuf
- get_next_subbuf
- put_subbuf
...

and then finally a thread would consume the data:

- get_next_subbuf
- put_next_subbuf

However, we don't want to populate data from the metadata cache into the
stream until put_next_subbuf is issued. Add a check to ensure that it is
not populated until required.

Also, disallow get_subbuf() ioctl on metadata channel: its random-access
semantic does not play well with serialization of the metadata cache on
demand.

Reviewed-by: Julien Desfossez <jdesfossez@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/lttng-abi.c b/lttng-abi.c
index 45d78d9750c3473b71b3aa9469ada486e78767e0..ba8362b167a3fd9a42612bc5c2631fedd4cb0bee 100644 (file)
--- a/lttng-abi.c
+++ b/lttng-abi.c
@@ -539,7 +539,7 @@ unsigned int lttng_metadata_ring_buffer_poll(struct file *filp,
                        mask |= POLLHUP;
 
                if (stream->metadata_cache->metadata_written >
-                               stream->metadata_cache_read)
+                               stream->metadata_out)
                        mask |= POLLIN;
        }
 
@@ -547,7 +547,7 @@ unsigned int lttng_metadata_ring_buffer_poll(struct file *filp,
 }
 
 static
-int lttng_metadata_ring_buffer_ioctl_get_subbuf(struct file *filp,
+int lttng_metadata_ring_buffer_ioctl_get_next_subbuf(struct file *filp,
                unsigned int cmd, unsigned long arg)
 {
        struct lttng_metadata_stream *stream = filp->private_data;
@@ -564,6 +564,15 @@ int lttng_metadata_ring_buffer_ioctl_get_subbuf(struct file *filp,
        return ret;
 }
 
+static
+void lttng_metadata_ring_buffer_ioctl_put_next_subbuf(struct file *filp,
+               unsigned int cmd, unsigned long arg)
+{
+       struct lttng_metadata_stream *stream = filp->private_data;
+
+       stream->metadata_out = stream->metadata_in;
+}
+
 static
 long lttng_metadata_ring_buffer_ioctl(struct file *filp,
                unsigned int cmd, unsigned long arg)
@@ -573,21 +582,41 @@ long lttng_metadata_ring_buffer_ioctl(struct file *filp,
        struct lib_ring_buffer *buf = stream->priv;
 
        switch (cmd) {
-       case RING_BUFFER_GET_SUBBUF:
        case RING_BUFFER_GET_NEXT_SUBBUF:
        {
-               ret = lttng_metadata_ring_buffer_ioctl_get_subbuf(filp,
+               ret = lttng_metadata_ring_buffer_ioctl_get_next_subbuf(filp,
                                cmd, arg);
                if (ret < 0)
                        goto err;
                break;
        }
+       case RING_BUFFER_GET_SUBBUF:
+       {
+               /*
+                * Random access is not allowed for metadata channel.
+                */
+               return -ENOSYS;
+       }
        default:
                break;
        }
+       /* PUT_SUBBUF is the one from lib ring buffer, unmodified. */
+
        /* Performing lib ring buffer ioctl after our own. */
-       return lib_ring_buffer_ioctl(filp, cmd, arg, buf);
+       ret = lib_ring_buffer_ioctl(filp, cmd, arg, buf);
+       if (ret < 0)
+               goto err;
 
+       switch (cmd) {
+       case RING_BUFFER_PUT_NEXT_SUBBUF:
+       {
+               lttng_metadata_ring_buffer_ioctl_put_next_subbuf(filp,
+                               cmd, arg);
+               break;
+       }
+       default:
+               break;
+       }
 err:
        return ret;
 }
@@ -602,21 +631,41 @@ long lttng_metadata_ring_buffer_compat_ioctl(struct file *filp,
        struct lib_ring_buffer *buf = stream->priv;
 
        switch (cmd) {
-       case RING_BUFFER_GET_SUBBUF:
        case RING_BUFFER_GET_NEXT_SUBBUF:
        {
-               ret = lttng_metadata_ring_buffer_ioctl_get_subbuf(filp,
+               ret = lttng_metadata_ring_buffer_ioctl_get_next_subbuf(filp,
                                cmd, arg);
                if (ret < 0)
                        goto err;
                break;
        }
+       case RING_BUFFER_GET_SUBBUF:
+       {
+               /*
+                * Random access is not allowed for metadata channel.
+                */
+               return -ENOSYS;
+       }
        default:
                break;
        }
+       /* PUT_SUBBUF is the one from lib ring buffer, unmodified. */
+
        /* Performing lib ring buffer ioctl after our own. */
-       return lib_ring_buffer_compat_ioctl(filp, cmd, arg, buf);
+       ret = lib_ring_buffer_compat_ioctl(filp, cmd, arg, buf);
+       if (ret < 0)
+               goto err;
 
+       switch (cmd) {
+       case RING_BUFFER_PUT_NEXT_SUBBUF:
+       {
+               lttng_metadata_ring_buffer_ioctl_put_next_subbuf(filp,
+                               cmd, arg);
+               break;
+       }
+       default:
+               break;
+       }
 err:
        return ret;
 }

diff --git a/lttng-events.c b/lttng-events.c
index 27a8f8644477be0bcf41c3732622e0e8c0f9fa57..567df65acea12345f51e46c937efa5f95cb15202 100644 (file)
--- a/lttng-events.c
+++ b/lttng-events.c
@@ -561,8 +561,16 @@ int lttng_metadata_output_channel(struct lttng_channel *chan,
        int ret = 0;
        size_t len, reserve_len;
 
+       /*
+        * Ensure we support mutiple get_next / put sequences followed
+        * by put_next.
+        */
+       WARN_ON(stream->metadata_in < stream->metadata_out);
+       if (stream->metadata_in != stream->metadata_out)
+               return 0;
+
        len = stream->metadata_cache->metadata_written -
-               stream->metadata_cache_read;
+               stream->metadata_in;
        if (!len)
                return 0;
        reserve_len = min_t(size_t,
@@ -579,10 +587,10 @@ int lttng_metadata_output_channel(struct lttng_channel *chan,
                goto end;
        }
        chan->ops->event_write(&ctx,
-                       stream->metadata_cache->data + stream->metadata_cache_read,
+                       stream->metadata_cache->data + stream->metadata_in,
                        reserve_len);
        chan->ops->event_commit(&ctx);
-       stream->metadata_cache_read += reserve_len;
+       stream->metadata_in += reserve_len;
        ret = reserve_len;
 
 end:

diff --git a/lttng-events.h b/lttng-events.h
index 46d80bcdb0edcd1a6d3599e7f630f240ecb28b0f..4c1f32288752e092612b791061006654ffcc66d9 100644 (file)
--- a/lttng-events.h
+++ b/lttng-events.h
@@ -278,7 +278,8 @@ struct lttng_channel {
 struct lttng_metadata_stream {
        void *priv;                     /* Ring buffer private data */
        struct lttng_metadata_cache *metadata_cache;
-       unsigned int metadata_cache_read; /* Bytes read from the cache */
+       unsigned int metadata_in;       /* Bytes read from the cache */
+       unsigned int metadata_out;      /* Bytes consumed from stream */
        int finalized;                  /* Has channel been finalized */
        wait_queue_head_t read_wait;    /* Reader buffer-level wait queue */
        struct list_head list;          /* Stream list */