The `do_sync_metadata` function is invoked everytime a data sub-buffer
is consumed in live mode.
In the user space case, lttng_ustconsumer_sync_metadata() returns
EAGAIN (positive) when there is new metadata to consume. This causes the
"metadata rendez-vous" synchronization to take place. However, the
kernel variant of this function returns 0 when there is new data to
consume, causing the "rendez-vous" to be skipped.
I have not observed an issue caused by this first-hand, but the check
appears bogus and skips over an essential synchronization step.
This check has been in place since at least 2013, although the callees
and their return values may have changed at some point in the past.
Solution
--------
The user space and kernel code paths mix various return code conventions
(negative errno, positive errno, 0/-1) which makes it difficult to
understand the final return codes and most likely lead to this confusion
in the first place.
Moreover, returning EAGAIN to indicate that data is ready to be consumed
is not appropriate in view of the existing conventions in the code base.
An explicit `enum sync_metadata_status` is returned by the domains'
sync_metadata operations which allows the common code to handle the
various conditions in a straight-forward manner and for the
"rendez-vous" to take place in the kernel case.
Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: Ib022eee97054c0b376853dd05593e3b94bc9a8ca
struct lttng_consumer_local_data *ctx)
{
int ret;
struct lttng_consumer_local_data *ctx)
{
int ret;
+ enum sync_metadata_status status;
assert(metadata);
assert(metadata->metadata_flag);
assert(metadata);
assert(metadata->metadata_flag);
/*
* Empty the metadata cache and flush the current stream.
*/
/*
* Empty the metadata cache and flush the current stream.
*/
- ret = lttng_kconsumer_sync_metadata(metadata);
+ status = lttng_kconsumer_sync_metadata(metadata);
break;
case LTTNG_CONSUMER32_UST:
case LTTNG_CONSUMER64_UST:
break;
case LTTNG_CONSUMER32_UST:
case LTTNG_CONSUMER64_UST:
* Ask the sessiond if we have new metadata waiting and update the
* consumer metadata cache.
*/
* Ask the sessiond if we have new metadata waiting and update the
* consumer metadata cache.
*/
- ret = lttng_ustconsumer_sync_metadata(ctx, metadata);
+ status = lttng_ustconsumer_sync_metadata(ctx, metadata);
- assert(0);
- ret = -1;
- break;
- /*
- * Error or no new metadata, we exit here.
- */
- if (ret <= 0 || ret == ENODATA) {
+
+ switch (status) {
+ case SYNC_METADATA_STATUS_NEW_DATA:
+ break;
+ case SYNC_METADATA_STATUS_NO_DATA:
+ ret = 0;
+ case SYNC_METADATA_STATUS_ERROR:
+ ret = -1;
+ goto end_unlock_mutex;
+ default:
+ abort();
*/
pthread_cond_wait(&metadata->metadata_rdv, &metadata->metadata_rdv_lock);
pthread_mutex_unlock(&metadata->metadata_rdv_lock);
*/
pthread_cond_wait(&metadata->metadata_rdv, &metadata->metadata_rdv_lock);
pthread_mutex_unlock(&metadata->metadata_rdv_lock);
- } while (ret == EAGAIN);
+ } while (status == SYNC_METADATA_STATUS_NEW_DATA);
CONSUMER_CHANNEL_TYPE_DATA = 1,
};
CONSUMER_CHANNEL_TYPE_DATA = 1,
};
+enum sync_metadata_status {
+ SYNC_METADATA_STATUS_NEW_DATA,
+ SYNC_METADATA_STATUS_NO_DATA,
+ SYNC_METADATA_STATUS_ERROR,
+};
+
extern struct lttng_consumer_global_data consumer_data;
struct stream_list {
extern struct lttng_consumer_global_data consumer_data;
struct stream_list {
* metadata thread can consumer them.
*
* Metadata stream lock MUST be acquired.
* metadata thread can consumer them.
*
* Metadata stream lock MUST be acquired.
- *
- * Return 0 if new metadatda is available, EAGAIN if the metadata stream
- * is empty or a negative value on error.
-int lttng_kconsumer_sync_metadata(struct lttng_consumer_stream *metadata)
+enum sync_metadata_status lttng_kconsumer_sync_metadata(
+ struct lttng_consumer_stream *metadata)
+ enum sync_metadata_status status;
assert(metadata);
ret = kernctl_buffer_flush(metadata->wait_fd);
if (ret < 0) {
ERR("Failed to flush kernel stream");
assert(metadata);
ret = kernctl_buffer_flush(metadata->wait_fd);
if (ret < 0) {
ERR("Failed to flush kernel stream");
+ status = SYNC_METADATA_STATUS_ERROR;
goto end;
}
ret = kernctl_snapshot(metadata->wait_fd);
if (ret < 0) {
goto end;
}
ret = kernctl_snapshot(metadata->wait_fd);
if (ret < 0) {
+ if (errno == EAGAIN) {
+ /* No new metadata, exit. */
+ DBG("Sync metadata, no new kernel metadata");
+ status = SYNC_METADATA_STATUS_NO_DATA;
+ } else {
ERR("Sync metadata, taking kernel snapshot failed.");
ERR("Sync metadata, taking kernel snapshot failed.");
+ status = SYNC_METADATA_STATUS_ERROR;
- DBG("Sync metadata, no new kernel metadata");
- /* No new metadata, exit. */
- ret = ENODATA;
- goto end;
+ } else {
+ status = SYNC_METADATA_STATUS_NEW_DATA;
int sock, struct pollfd *consumer_sockpoll);
int lttng_kconsumer_on_recv_stream(struct lttng_consumer_stream *stream);
int lttng_kconsumer_data_pending(struct lttng_consumer_stream *stream);
int sock, struct pollfd *consumer_sockpoll);
int lttng_kconsumer_on_recv_stream(struct lttng_consumer_stream *stream);
int lttng_kconsumer_data_pending(struct lttng_consumer_stream *stream);
-int lttng_kconsumer_sync_metadata(struct lttng_consumer_stream *metadata);
+enum sync_metadata_status lttng_kconsumer_sync_metadata(
+ struct lttng_consumer_stream *metadata);
#endif /* _LTTNG_KCONSUMER_H */
#endif /* _LTTNG_KCONSUMER_H */
/*
* Write up to one packet from the metadata cache to the channel.
*
/*
* Write up to one packet from the metadata cache to the channel.
*
- * Returns the number of bytes pushed in the cache, or a negative value
- * on error.
+ * Returns the number of bytes pushed from the cache into the ring buffer, or a
+ * negative value on error.
*/
static
int commit_one_metadata_packet(struct lttng_consumer_stream *stream)
*/
static
int commit_one_metadata_packet(struct lttng_consumer_stream *stream)
* awaiting on metadata to be pushed out.
*
* The RCU read side lock must be held by the caller.
* awaiting on metadata to be pushed out.
*
* The RCU read side lock must be held by the caller.
- *
- * Return 0 if new metadatda is available, EAGAIN if the metadata stream
- * is empty or a negative value on error.
-int lttng_ustconsumer_sync_metadata(struct lttng_consumer_local_data *ctx,
+enum sync_metadata_status lttng_ustconsumer_sync_metadata(
+ struct lttng_consumer_local_data *ctx,
struct lttng_consumer_stream *metadata_stream)
{
int ret;
struct lttng_consumer_stream *metadata_stream)
{
int ret;
+ enum sync_metadata_status status;
struct lttng_consumer_channel *metadata_channel;
assert(ctx);
struct lttng_consumer_channel *metadata_channel;
assert(ctx);
ret = lttng_ustconsumer_request_metadata(ctx, metadata_channel, 0, 0);
pthread_mutex_lock(&metadata_stream->lock);
if (ret < 0) {
ret = lttng_ustconsumer_request_metadata(ctx, metadata_channel, 0, 0);
pthread_mutex_lock(&metadata_stream->lock);
if (ret < 0) {
+ status = SYNC_METADATA_STATUS_ERROR;
if (consumer_stream_is_deleted(metadata_stream)) {
DBG("Metadata stream %" PRIu64 " was deleted during the metadata synchronization",
metadata_stream->key);
if (consumer_stream_is_deleted(metadata_stream)) {
DBG("Metadata stream %" PRIu64 " was deleted during the metadata synchronization",
metadata_stream->key);
+ status = SYNC_METADATA_STATUS_NO_DATA;
goto end;
}
ret = commit_one_metadata_packet(metadata_stream);
goto end;
}
ret = commit_one_metadata_packet(metadata_stream);
+ if (ret < 0) {
+ status = SYNC_METADATA_STATUS_ERROR;
goto end;
} else if (ret > 0) {
goto end;
} else if (ret > 0) {
+ status = SYNC_METADATA_STATUS_NEW_DATA;
+ } else /* ret == 0 */ {
+ status = SYNC_METADATA_STATUS_NO_DATA;
+ goto end;
}
ret = ustctl_snapshot(metadata_stream->ustream);
if (ret < 0) {
}
ret = ustctl_snapshot(metadata_stream->ustream);
if (ret < 0) {
- if (errno != EAGAIN) {
- ERR("Sync metadata, taking UST snapshot");
- goto end;
- }
- DBG("No new metadata when syncing them.");
- /* No new metadata, exit. */
- ret = ENODATA;
+ ERR("Failed to take a snapshot of the metadata ring-buffer positions, ret = %d", ret);
+ status = SYNC_METADATA_STATUS_ERROR;
- /*
- * After this flush, we still need to extract metadata.
- */
- if (retry) {
- ret = EAGAIN;
- }
-
struct lttng_consumer_channel *channel, int timer, int wait);
int lttng_ustconsumer_request_metadata(struct lttng_consumer_local_data *ctx,
struct lttng_consumer_channel *channel, int timer, int wait);
struct lttng_consumer_channel *channel, int timer, int wait);
int lttng_ustconsumer_request_metadata(struct lttng_consumer_local_data *ctx,
struct lttng_consumer_channel *channel, int timer, int wait);
-int lttng_ustconsumer_sync_metadata(struct lttng_consumer_local_data *ctx,
+enum sync_metadata_status lttng_ustconsumer_sync_metadata(
+ struct lttng_consumer_local_data *ctx,
struct lttng_consumer_stream *metadata);
void lttng_ustconsumer_flush_buffer(struct lttng_consumer_stream *stream,
int producer);
struct lttng_consumer_stream *metadata);
void lttng_ustconsumer_flush_buffer(struct lttng_consumer_stream *stream,
int producer);
return -ENOSYS;
}
static inline
return -ENOSYS;
}
static inline
-int lttng_ustconsumer_sync_metadata(struct lttng_consumer_local_data *ctx,
+enum sync_metadata_status lttng_ustconsumer_sync_metadata(struct lttng_consumer_local_data *ctx,
struct lttng_consumer_stream *metadata)
{
struct lttng_consumer_stream *metadata)
{
+ return SYNC_METADATA_STATUS_ERROR;
}
static inline
void lttng_ustconsumer_flush_buffer(struct lttng_consumer_stream *stream,
}
static inline
void lttng_ustconsumer_flush_buffer(struct lttng_consumer_stream *stream,