[PATCH lttng-tools 2/2] Clean-up shm directory tree after freeing the channel

[PATCH lttng-tools 2/2] Clean-up shm directory tree after freeing the channel

[Jonathan Rajotte]

On NFS an unlinked file that is still mmaped or unclosed is kept around
by creating a .nfsXXXX file since an unlinked file can still be used.[1]

This prevent the effective cleanup of the shm tree directory because it happens
before the userspace consumer shm handles table cleanup [2].

Moving the tree removal to lttng_ustconsumer_free_channel ensure that the
cleanup is done when the files are considered completely closed/unmapped.

[1] http://nfs.sourceforge.net/ Look for "silly rename"
[2] See channel_free subcall to shm_object_table_destroy

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/common/ust-consumer/ust-consumer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
index fe7445b..a113ef1 100644
--- a/src/common/ust-consumer/ust-consumer.c
+++ b/src/common/ust-consumer/ust-consumer.c
@@ -1985,11 +1985,6 @@ void lttng_ustconsumer_del_channel(struct lttng_consumer_channel *chan)
 			}
 		}
 	}
-	/* Try to rmdir all directories under shm_path root. */
-	if (chan->root_shm_path[0]) {
-		(void) run_as_recursive_rmdir(chan->root_shm_path,
-				chan->uid, chan->gid);
-	}
 }
 
 void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
@@ -1999,6 +1994,11 @@ void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
 
 	consumer_metadata_cache_destroy(chan);
 	ustctl_destroy_channel(chan->uchan);
+	/* Try to rmdir all directories under shm_path root. */
+	if (chan->root_shm_path[0]) {
+		(void) run_as_recursive_rmdir(chan->root_shm_path,
+				chan->uid, chan->gid);
+	}
 	free(chan->stream_fds);
 }
 
-- 
2.7.0

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

Re: [PATCH lttng-tools 2/2] Clean-up shm directory tree after freeing the channel

[Jérémie Galarneau]

Is a first patch missing? I'm only seeing 2/2.

Thanks,
Jérémie

On Fri, Mar 18, 2016 at 1:20 PM, Jonathan Rajotte
<jonathan.rajotte-julien@efficios.com> wrote:
> On NFS an unlinked file that is still mmaped or unclosed is kept around
> by creating a .nfsXXXX file since an unlinked file can still be used.[1]
>
> This prevent the effective cleanup of the shm tree directory because it happens
> before the userspace consumer shm handles table cleanup [2].
>
> Moving the tree removal to lttng_ustconsumer_free_channel ensure that the
> cleanup is done when the files are considered completely closed/unmapped.
>
> [1] http://nfs.sourceforge.net/ Look for "silly rename"
> [2] See channel_free subcall to shm_object_table_destroy
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/ust-consumer/ust-consumer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
> index fe7445b..a113ef1 100644
> --- a/src/common/ust-consumer/ust-consumer.c
> +++ b/src/common/ust-consumer/ust-consumer.c
> @@ -1985,11 +1985,6 @@ void lttng_ustconsumer_del_channel(struct lttng_consumer_channel *chan)
>                         }
>                 }
>         }
> -       /* Try to rmdir all directories under shm_path root. */
> -       if (chan->root_shm_path[0]) {
> -               (void) run_as_recursive_rmdir(chan->root_shm_path,
> -                               chan->uid, chan->gid);
> -       }
>  }
>
>  void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
> @@ -1999,6 +1994,11 @@ void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
>
>         consumer_metadata_cache_destroy(chan);
>         ustctl_destroy_channel(chan->uchan);
> +       /* Try to rmdir all directories under shm_path root. */
> +       if (chan->root_shm_path[0]) {
> +               (void) run_as_recursive_rmdir(chan->root_shm_path,
> +                               chan->uid, chan->gid);
> +       }
>         free(chan->stream_fds);
>  }
>
> --
> 2.7.0
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools 2/2] Clean-up shm directory tree after freeing the channel

[Jonathan Rajotte Julien]

Should be there.

On 2016-03-18 01:27 PM, Jérémie Galarneau wrote:
> Is a first patch missing? I'm only seeing 2/2.
>
> Thanks,
> Jérémie
>
> On Fri, Mar 18, 2016 at 1:20 PM, Jonathan Rajotte
> <jonathan.rajotte-julien@efficios.com> wrote:
>> On NFS an unlinked file that is still mmaped or unclosed is kept around
>> by creating a .nfsXXXX file since an unlinked file can still be used.[1]
>>
>> This prevent the effective cleanup of the shm tree directory because it happens
>> before the userspace consumer shm handles table cleanup [2].
>>
>> Moving the tree removal to lttng_ustconsumer_free_channel ensure that the
>> cleanup is done when the files are considered completely closed/unmapped.
>>
>> [1] http://nfs.sourceforge.net/ Look for "silly rename"
>> [2] See channel_free subcall to shm_object_table_destroy
>>
>> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
>> ---
>>   src/common/ust-consumer/ust-consumer.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
>> index fe7445b..a113ef1 100644
>> --- a/src/common/ust-consumer/ust-consumer.c
>> +++ b/src/common/ust-consumer/ust-consumer.c
>> @@ -1985,11 +1985,6 @@ void lttng_ustconsumer_del_channel(struct lttng_consumer_channel *chan)
>>                          }
>>                  }
>>          }
>> -       /* Try to rmdir all directories under shm_path root. */
>> -       if (chan->root_shm_path[0]) {
>> -               (void) run_as_recursive_rmdir(chan->root_shm_path,
>> -                               chan->uid, chan->gid);
>> -       }
>>   }
>>
>>   void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
>> @@ -1999,6 +1994,11 @@ void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
>>
>>          consumer_metadata_cache_destroy(chan);
>>          ustctl_destroy_channel(chan->uchan);
>> +       /* Try to rmdir all directories under shm_path root. */
>> +       if (chan->root_shm_path[0]) {
>> +               (void) run_as_recursive_rmdir(chan->root_shm_path,
>> +                               chan->uid, chan->gid);
>> +       }
>>          free(chan->stream_fds);
>>   }
>>
>> --
>> 2.7.0
>>
>
>

-- 
Jonathan R. Julien
Efficios

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

Re: [PATCH lttng-tools 1/2] Fix: d_type validity is not guaranteed on all nfs versions

[Jonathan Rajotte Julien]

Those two patches can also be backported to stable-2.7 with the 
condition that commit
c4b765f0d3892d57bb7e8828c324d304e9c4d5e9 [1] is also backported since it 
fix the
d_type problem in src/common/utils.c.

Thanks

[1]
commit c4b765f0d3892d57bb7e8828c324d304e9c4d5e9
Author: Michael Jeanson <mjeanson@efficios.com>
Date:   Wed Oct 14 16:02:45 2015 -0400

     Port: Replace dirent->d_type by stat

     dirent->d_type is Linux specific while 'stat' is part of POSIX

     Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
     Signed-off-by: J<C3><A9>r<C3><A9>mie Galarneau 
<jeremie.galarneau@efficios.com>



On 2016-03-18 01:20 PM, Jonathan Rajotte wrote:
> When using lttng-crash on files mounted on a nfs mount the d_type from the dirent
> struct is not necessarily set and result in DT_UNKNOWN d_type.
>
> stat() provide the valid information on an nfs mount.
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>   src/bin/lttng-crash/lttng-crash.c | 80 +++++++++++++++++++++++++++++----------
>   1 file changed, 61 insertions(+), 19 deletions(-)
>
> diff --git a/src/bin/lttng-crash/lttng-crash.c b/src/bin/lttng-crash/lttng-crash.c
> index 653e4d0..8eddbd9 100644
> --- a/src/bin/lttng-crash/lttng-crash.c
> +++ b/src/bin/lttng-crash/lttng-crash.c
> @@ -976,6 +976,7 @@ int extract_trace_recursive(const char *output_path,
>   	DIR *dir;
>   	int dir_fd, ret = 0, closeret;
>   	struct dirent *entry;
> +	size_t path_len;
>   	int has_warning = 0;
>   
>   	/* Open directory */
> @@ -984,6 +985,9 @@ int extract_trace_recursive(const char *output_path,
>   		PERROR("Cannot open '%s' path", input_path);
>   		return -1;
>   	}
> +
> +	path_len = strlen(input_path);
> +
>   	dir_fd = dirfd(dir);
>   	if (dir_fd < 0) {
>   		PERROR("dirfd");
> @@ -991,13 +995,34 @@ int extract_trace_recursive(const char *output_path,
>   	}
>   
>   	while ((entry = readdir(dir))) {
> +		struct stat st;
> +		size_t name_len;
> +		char filename[PATH_MAX];
> +
>   		if (!strcmp(entry->d_name, ".")
>   				|| !strcmp(entry->d_name, "..")) {
>   			continue;
>   		}
> -		switch (entry->d_type) {
> -		case DT_DIR:
> -		{
> +
> +		name_len = strlen(entry->d_name);
> +		if (path_len + name_len + 2 > sizeof(filename)) {
> +			ERR("Failed to remove file: path name too long (%s/%s)",
> +				input_path, entry->d_name);
> +			continue;
> +		}
> +
> +		if (snprintf(filename, sizeof(filename), "%s/%s",
> +				input_path, entry->d_name) < 0) {
> +			ERR("Failed to format path.");
> +			continue;
> +		}
> +
> +		if (stat(filename, &st)) {
> +			PERROR("stat");
> +			continue;
> +		}
> +
> +		if (S_ISDIR(st.st_mode)) {
>   			char output_subpath[PATH_MAX];
>   			char input_subpath[PATH_MAX];
>   
> @@ -1029,22 +1054,17 @@ int extract_trace_recursive(const char *output_path,
>   			if (ret) {
>   				has_warning = 1;
>   			}
> -			break;
> -		}
> -		case DT_REG:
> -		case DT_LNK:
> +		} else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
>   			if (!strcmp(entry->d_name, "metadata")) {
>   				ret = extract_one_trace(output_path,
>   					input_path);
>   				if (ret) {
>   					WARN("Error extracting trace '%s', continuing anyway.",
>   						input_path);
> -					has_warning = 1;
> +					has_warning = 2;
>   				}
>   			}
> -			/* Ignore other files */
> -			break;
> -		default:
> +		} else {
>   			has_warning = 1;
>   			goto end;
>   		}
> @@ -1062,6 +1082,7 @@ int delete_dir_recursive(const char *path)
>   {
>   	DIR *dir;
>   	int dir_fd, ret = 0, closeret;
> +	size_t path_len;
>   	struct dirent *entry;
>   
>   	/* Open trace directory */
> @@ -1071,6 +1092,9 @@ int delete_dir_recursive(const char *path)
>   		ret = -errno;
>   	        goto end;
>   	}
> +
> +	path_len = strlen(path);
> +
>   	dir_fd = dirfd(dir);
>   	if (dir_fd < 0) {
>   		PERROR("dirfd");
> @@ -1079,13 +1103,34 @@ int delete_dir_recursive(const char *path)
>   	}
>   
>   	while ((entry = readdir(dir))) {
> +		struct stat st;
> +		size_t name_len;
> +		char filename[PATH_MAX];
> +
>   		if (!strcmp(entry->d_name, ".")
>   				|| !strcmp(entry->d_name, "..")) {
>   			continue;
>   		}
> -		switch (entry->d_type) {
> -		case DT_DIR:
> -		{
> +
> +		name_len = strlen(entry->d_name);
> +		if (path_len + name_len + 2 > sizeof(filename)) {
> +			ERR("Failed to remove file: path name too long (%s/%s)",
> +				path, entry->d_name);
> +			continue;
> +		}
> +
> +		if (snprintf(filename, sizeof(filename), "%s/%s",
> +				path, entry->d_name) < 0) {
> +			ERR("Failed to format path.");
> +			continue;
> +		}
> +
> +		if (stat(filename, &st)) {
> +			PERROR("stat");
> +			continue;
> +		}
> +
> +		if (S_ISDIR(st.st_mode)) {
>   			char *subpath = zmalloc(PATH_MAX);
>   
>   			if (!subpath) {
> @@ -1106,16 +1151,13 @@ int delete_dir_recursive(const char *path)
>   				/* Error occured, abort traversal. */
>   				goto end;
>   			}
> -			break;
> -		}
> -		case DT_REG:
> +		} else if (S_ISREG(st.st_mode)) {
>   			ret = unlinkat(dir_fd, entry->d_name, 0);
>   			if (ret) {
>   				PERROR("Unlinking '%s'", entry->d_name);
>   				goto end;
>   			}
> -			break;
> -		default:
> +		} else {
>   			ret = -EINVAL;
>   			goto end;
>   		}

-- 
Jonathan R. Julien
Efficios

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

Re: [PATCH lttng-tools 2/2] Clean-up shm directory tree after freeing the channel

[Jérémie Galarneau]

Merged in master, stable-2.8 and stable-2.7, thanks!
I'll wait for a v2 of the first patch.

Jérémie

On Fri, Mar 18, 2016 at 1:20 PM, Jonathan Rajotte
<jonathan.rajotte-julien@efficios.com> wrote:
> On NFS an unlinked file that is still mmaped or unclosed is kept around
> by creating a .nfsXXXX file since an unlinked file can still be used.[1]
>
> This prevent the effective cleanup of the shm tree directory because it happens
> before the userspace consumer shm handles table cleanup [2].
>
> Moving the tree removal to lttng_ustconsumer_free_channel ensure that the
> cleanup is done when the files are considered completely closed/unmapped.
>
> [1] http://nfs.sourceforge.net/ Look for "silly rename"
> [2] See channel_free subcall to shm_object_table_destroy
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/ust-consumer/ust-consumer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c
> index fe7445b..a113ef1 100644
> --- a/src/common/ust-consumer/ust-consumer.c
> +++ b/src/common/ust-consumer/ust-consumer.c
> @@ -1985,11 +1985,6 @@ void lttng_ustconsumer_del_channel(struct lttng_consumer_channel *chan)
>                         }
>                 }
>         }
> -       /* Try to rmdir all directories under shm_path root. */
> -       if (chan->root_shm_path[0]) {
> -               (void) run_as_recursive_rmdir(chan->root_shm_path,
> -                               chan->uid, chan->gid);
> -       }
>  }
>
>  void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
> @@ -1999,6 +1994,11 @@ void lttng_ustconsumer_free_channel(struct lttng_consumer_channel *chan)
>
>         consumer_metadata_cache_destroy(chan);
>         ustctl_destroy_channel(chan->uchan);
> +       /* Try to rmdir all directories under shm_path root. */
> +       if (chan->root_shm_path[0]) {
> +               (void) run_as_recursive_rmdir(chan->root_shm_path,
> +                               chan->uid, chan->gid);
> +       }
>         free(chan->stream_fds);
>  }
>
> --
> 2.7.0
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev