Re: [RFC PATCH lttng-tools] Fix: metadata push -EPIPE should be recoverable

Re: [RFC PATCH lttng-tools] Fix: metadata push -EPIPE should be recoverable

[Aravind HT]

[-- Attachment #1.1: Type: text/plain, Size: 3621 bytes --]

In case of -EPIPE, im thinking if we can send LTTNG_ERR_UND or some other
error back to the consumer for it to do any relevant clean up.

On Mon, Jul 6, 2015 at 9:51 PM, Mathieu Desnoyers <
mathieu.desnoyers@efficios.com> wrote:

> This return value can be caused by application terminating concurrently
> (when using per-PID buffers), so it should not make the consumer
> management thread exit.
>
> CC: Aravind HT <aravind.ht@gmail.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  src/bin/lttng-sessiond/ust-app.c      | 13 ++++++++++---
>  src/bin/lttng-sessiond/ust-consumer.c |  7 +++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/src/bin/lttng-sessiond/ust-app.c
> b/src/bin/lttng-sessiond/ust-app.c
> index c30792c..6f032da 100644
> --- a/src/bin/lttng-sessiond/ust-app.c
> +++ b/src/bin/lttng-sessiond/ust-app.c
> @@ -431,6 +431,9 @@ void delete_ust_app_channel(int sock, struct
> ust_app_channel *ua_chan,
>   * Must be called with the registry lock held.
>   *
>   * On success, return the len of metadata pushed or else a negative value.
> + * Returning a -EPIPE return value means we could not send the metadata,
> + * but it can be caused by recoverable errors (e.g. the application has
> + * terminated concurrently).
>   */
>  ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
>                 struct consumer_socket *socket, int send_zero_data)
> @@ -454,9 +457,10 @@ ssize_t ust_app_push_metadata(struct
> ust_registry_session *registry,
>         /*
>          * On a push metadata error either the consumer is dead or the
>          * metadata channel has been destroyed because its endpoint
> -        * might have died (e.g: relayd). If so, the metadata closed
> -        * flag is set to 1 so we deny pushing metadata again which is
> -        * not valid anymore on the consumer side.
> +        * might have died (e.g: relayd), or because the application has
> +        * exited. If so, the metadata closed flag is set to 1 so we
> +        * deny pushing metadata again which is not valid anymore on the
> +        * consumer side.
>          */
>         if (registry->metadata_closed) {
>                 return -EPIPE;
> @@ -547,6 +551,9 @@ error_push:
>   * of socket throughout this function.
>   *
>   * Return 0 on success else a negative error.
> + * Returning a -EPIPE return value means we could not send the metadata,
> + * but it can be caused by recoverable errors (e.g. the application has
> + * terminated concurrently).
>   */
>  static int push_metadata(struct ust_registry_session *registry,
>                 struct consumer_output *consumer)
> diff --git a/src/bin/lttng-sessiond/ust-consumer.c
> b/src/bin/lttng-sessiond/ust-consumer.c
> index 78e50df..ad076e3 100644
> --- a/src/bin/lttng-sessiond/ust-consumer.c
> +++ b/src/bin/lttng-sessiond/ust-consumer.c
> @@ -511,12 +511,15 @@ int ust_consumer_metadata_request(struct
> consumer_socket *socket)
>         pthread_mutex_lock(&ust_reg->lock);
>         ret_push = ust_app_push_metadata(ust_reg, socket, 1);
>         pthread_mutex_unlock(&ust_reg->lock);
> -       if (ret_push < 0) {
> +       if (ret_push == -EPIPE) {
> +               DBG("Application or relay closed while pushing metadata");
> +       } else if (ret_push < 0) {
>                 ERR("Pushing metadata");
>                 ret = -1;
>                 goto end;
> +       } else {
> +               DBG("UST Consumer metadata pushed successfully");
>         }
> -       DBG("UST Consumer metadata pushed successfully");
>         ret = 0;
>
>  end:
> --
> 2.1.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 4598 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH lttng-tools] Fix: metadata push -EPIPE should be recoverable

[Mathieu Desnoyers]

[-- Attachment #1.1: Type: text/plain, Size: 3978 bytes --]

We should not have to send any error back to the consumer because the 
app disappearing will eventually close all file descriptors related to the 
communication and notification pipes (sessiond vs apps), as well as all 
stream file descriptors (consumerd vs apps). So the consumerd will 
eventually be notified that the application disappeared, and will therefore 
close the associated streams (in per-pid buffers) in due time. 

Thanks, 

Mathieu 

----- On Jul 6, 2015, at 12:42 PM, HT, Aravind <aravind.ht@gmail.com> wrote: 

> In case of -EPIPE, im thinking if we can send LTTNG_ERR_UND or some other error
> back to the consumer for it to do any relevant clean up.

> On Mon, Jul 6, 2015 at 9:51 PM, Mathieu Desnoyers <
> mathieu.desnoyers@efficios.com > wrote:

>> This return value can be caused by application terminating concurrently
>> (when using per-PID buffers), so it should not make the consumer
>> management thread exit.

>> CC: Aravind HT < aravind.ht@gmail.com >
>> Signed-off-by: Mathieu Desnoyers < mathieu.desnoyers@efficios.com >
>> ---
>> src/bin/lttng-sessiond/ust-app.c | 13 ++++++++++---
>> src/bin/lttng-sessiond/ust-consumer.c | 7 +++++--
>> 2 files changed, 15 insertions(+), 5 deletions(-)

>> diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
>> index c30792c..6f032da 100644
>> --- a/src/bin/lttng-sessiond/ust-app.c
>> +++ b/src/bin/lttng-sessiond/ust-app.c
>> @@ -431,6 +431,9 @@ void delete_ust_app_channel(int sock, struct ust_app_channel
>> *ua_chan,
>> * Must be called with the registry lock held.
>> *
>> * On success, return the len of metadata pushed or else a negative value.
>> + * Returning a -EPIPE return value means we could not send the metadata,
>> + * but it can be caused by recoverable errors (e.g. the application has
>> + * terminated concurrently).
>> */
>> ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
>> struct consumer_socket *socket, int send_zero_data)
>> @@ -454,9 +457,10 @@ ssize_t ust_app_push_metadata(struct ust_registry_session
>> *registry,
>> /*
>> * On a push metadata error either the consumer is dead or the
>> * metadata channel has been destroyed because its endpoint
>> - * might have died (e.g: relayd). If so, the metadata closed
>> - * flag is set to 1 so we deny pushing metadata again which is
>> - * not valid anymore on the consumer side.
>> + * might have died (e.g: relayd), or because the application has
>> + * exited. If so, the metadata closed flag is set to 1 so we
>> + * deny pushing metadata again which is not valid anymore on the
>> + * consumer side.
>> */
>> if (registry->metadata_closed) {
>> return -EPIPE;
>> @@ -547,6 +551,9 @@ error_push:
>> * of socket throughout this function.
>> *
>> * Return 0 on success else a negative error.
>> + * Returning a -EPIPE return value means we could not send the metadata,
>> + * but it can be caused by recoverable errors (e.g. the application has
>> + * terminated concurrently).
>> */
>> static int push_metadata(struct ust_registry_session *registry,
>> struct consumer_output *consumer)
>> diff --git a/src/bin/lttng-sessiond/ust-consumer.c
>> b/src/bin/lttng-sessiond/ust-consumer.c
>> index 78e50df..ad076e3 100644
>> --- a/src/bin/lttng-sessiond/ust-consumer.c
>> +++ b/src/bin/lttng-sessiond/ust-consumer.c
>> @@ -511,12 +511,15 @@ int ust_consumer_metadata_request(struct consumer_socket
>> *socket)
>> pthread_mutex_lock(&ust_reg->lock);
>> ret_push = ust_app_push_metadata(ust_reg, socket, 1);
>> pthread_mutex_unlock(&ust_reg->lock);
>> - if (ret_push < 0) {
>> + if (ret_push == -EPIPE) {
>> + DBG("Application or relay closed while pushing metadata");
>> + } else if (ret_push < 0) {
>> ERR("Pushing metadata");
>> ret = -1;
>> goto end;
>> + } else {
>> + DBG("UST Consumer metadata pushed successfully");
>> }
>> - DBG("UST Consumer metadata pushed successfully");
>> ret = 0;

>> end:
>> --
>> 2.1.4

-- 
Mathieu Desnoyers 
EfficiOS Inc. 
http://www.efficios.com 

[-- Attachment #1.2: Type: text/html, Size: 6650 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH lttng-tools] Fix: metadata push -EPIPE should be recoverable

[Jérémie Galarneau]

[-- Attachment #1.1: Type: text/plain, Size: 3706 bytes --]

Merged in master, stable-2.6 and stable-2.5. Thanks!

Jérémie

On Mon, Jul 6, 2015 at 12:21 PM, Mathieu Desnoyers <
mathieu.desnoyers@efficios.com> wrote:

> This return value can be caused by application terminating concurrently
> (when using per-PID buffers), so it should not make the consumer
> management thread exit.
>
> CC: Aravind HT <aravind.ht@gmail.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  src/bin/lttng-sessiond/ust-app.c      | 13 ++++++++++---
>  src/bin/lttng-sessiond/ust-consumer.c |  7 +++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/src/bin/lttng-sessiond/ust-app.c
> b/src/bin/lttng-sessiond/ust-app.c
> index c30792c..6f032da 100644
> --- a/src/bin/lttng-sessiond/ust-app.c
> +++ b/src/bin/lttng-sessiond/ust-app.c
> @@ -431,6 +431,9 @@ void delete_ust_app_channel(int sock, struct
> ust_app_channel *ua_chan,
>   * Must be called with the registry lock held.
>   *
>   * On success, return the len of metadata pushed or else a negative value.
> + * Returning a -EPIPE return value means we could not send the metadata,
> + * but it can be caused by recoverable errors (e.g. the application has
> + * terminated concurrently).
>   */
>  ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
>                 struct consumer_socket *socket, int send_zero_data)
> @@ -454,9 +457,10 @@ ssize_t ust_app_push_metadata(struct
> ust_registry_session *registry,
>         /*
>          * On a push metadata error either the consumer is dead or the
>          * metadata channel has been destroyed because its endpoint
> -        * might have died (e.g: relayd). If so, the metadata closed
> -        * flag is set to 1 so we deny pushing metadata again which is
> -        * not valid anymore on the consumer side.
> +        * might have died (e.g: relayd), or because the application has
> +        * exited. If so, the metadata closed flag is set to 1 so we
> +        * deny pushing metadata again which is not valid anymore on the
> +        * consumer side.
>          */
>         if (registry->metadata_closed) {
>                 return -EPIPE;
> @@ -547,6 +551,9 @@ error_push:
>   * of socket throughout this function.
>   *
>   * Return 0 on success else a negative error.
> + * Returning a -EPIPE return value means we could not send the metadata,
> + * but it can be caused by recoverable errors (e.g. the application has
> + * terminated concurrently).
>   */
>  static int push_metadata(struct ust_registry_session *registry,
>                 struct consumer_output *consumer)
> diff --git a/src/bin/lttng-sessiond/ust-consumer.c
> b/src/bin/lttng-sessiond/ust-consumer.c
> index 78e50df..ad076e3 100644
> --- a/src/bin/lttng-sessiond/ust-consumer.c
> +++ b/src/bin/lttng-sessiond/ust-consumer.c
> @@ -511,12 +511,15 @@ int ust_consumer_metadata_request(struct
> consumer_socket *socket)
>         pthread_mutex_lock(&ust_reg->lock);
>         ret_push = ust_app_push_metadata(ust_reg, socket, 1);
>         pthread_mutex_unlock(&ust_reg->lock);
> -       if (ret_push < 0) {
> +       if (ret_push == -EPIPE) {
> +               DBG("Application or relay closed while pushing metadata");
> +       } else if (ret_push < 0) {
>                 ERR("Pushing metadata");
>                 ret = -1;
>                 goto end;
> +       } else {
> +               DBG("UST Consumer metadata pushed successfully");
>         }
> -       DBG("UST Consumer metadata pushed successfully");
>         ret = 0;
>
>  end:
> --
> 2.1.4
>
>


-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

[-- Attachment #1.2: Type: text/html, Size: 4682 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[RFC PATCH lttng-tools] Fix: metadata push -EPIPE should be recoverable

[Mathieu Desnoyers]

This return value can be caused by application terminating concurrently
(when using per-PID buffers), so it should not make the consumer
management thread exit.

CC: Aravind HT <aravind.ht@gmail.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 src/bin/lttng-sessiond/ust-app.c      | 13 ++++++++++---
 src/bin/lttng-sessiond/ust-consumer.c |  7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
index c30792c..6f032da 100644
--- a/src/bin/lttng-sessiond/ust-app.c
+++ b/src/bin/lttng-sessiond/ust-app.c
@@ -431,6 +431,9 @@ void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan,
  * Must be called with the registry lock held.
  *
  * On success, return the len of metadata pushed or else a negative value.
+ * Returning a -EPIPE return value means we could not send the metadata,
+ * but it can be caused by recoverable errors (e.g. the application has
+ * terminated concurrently).
  */
 ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
 		struct consumer_socket *socket, int send_zero_data)
@@ -454,9 +457,10 @@ ssize_t ust_app_push_metadata(struct ust_registry_session *registry,
 	/*
 	 * On a push metadata error either the consumer is dead or the
 	 * metadata channel has been destroyed because its endpoint
-	 * might have died (e.g: relayd). If so, the metadata closed
-	 * flag is set to 1 so we deny pushing metadata again which is
-	 * not valid anymore on the consumer side.
+	 * might have died (e.g: relayd), or because the application has
+	 * exited. If so, the metadata closed flag is set to 1 so we
+	 * deny pushing metadata again which is not valid anymore on the
+	 * consumer side.
 	 */
 	if (registry->metadata_closed) {
 		return -EPIPE;
@@ -547,6 +551,9 @@ error_push:
  * of socket throughout this function.
  *
  * Return 0 on success else a negative error.
+ * Returning a -EPIPE return value means we could not send the metadata,
+ * but it can be caused by recoverable errors (e.g. the application has
+ * terminated concurrently).
  */
 static int push_metadata(struct ust_registry_session *registry,
 		struct consumer_output *consumer)
diff --git a/src/bin/lttng-sessiond/ust-consumer.c b/src/bin/lttng-sessiond/ust-consumer.c
index 78e50df..ad076e3 100644
--- a/src/bin/lttng-sessiond/ust-consumer.c
+++ b/src/bin/lttng-sessiond/ust-consumer.c
@@ -511,12 +511,15 @@ int ust_consumer_metadata_request(struct consumer_socket *socket)
 	pthread_mutex_lock(&ust_reg->lock);
 	ret_push = ust_app_push_metadata(ust_reg, socket, 1);
 	pthread_mutex_unlock(&ust_reg->lock);
-	if (ret_push < 0) {
+	if (ret_push == -EPIPE) {
+		DBG("Application or relay closed while pushing metadata");
+	} else if (ret_push < 0) {
 		ERR("Pushing metadata");
 		ret = -1;
 		goto end;
+	} else {
+		DBG("UST Consumer metadata pushed successfully");
 	}
-	DBG("UST Consumer metadata pushed successfully");
 	ret = 0;
 
 end:
-- 
2.1.4