Re: [PATCH v2 24/87] trace-cmd library: Add local helper function for data compression

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 29 Jul 2021 08:08:56 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> The newly added helper functions read data from a file and compress it,
> before writing into the trace file. The trace data is comressed in
> chunks, which are page aligned. A new local define is introduced:
>   PAGES_IN_CHUNK
> which can be used to tune how big a compression chunk is.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-output.c | 70 ++++++++++++++++++++++++++++++++----
>  1 file changed, 64 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 6a44a99b..90625c4e 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -285,18 +285,26 @@ static unsigned long get_size(const char *file)
>  	return size;
>  }
>  
> -static tsize_t copy_file_fd(struct tracecmd_output *handle, int fd)
> +static tsize_t copy_file_fd(struct tracecmd_output *handle, int fd, unsigned long long max)
>  {
> +	tsize_t rsize = 0;
>  	tsize_t size = 0;
>  	char buf[BUFSIZ];
>  	stsize_t r;
>  
>  	do {
> -		r = read(fd, buf, BUFSIZ);
> +		if (max > 0 && (max - size) < BUFSIZ)
> +			rsize = (max - size);
> +		else
> +			rsize = BUFSIZ;
> +
> +		r = read(fd, buf, rsize);
>  		if (r > 0) {
>  			size += r;
>  			if (do_write_check(handle, buf, r))
>  				return 0;
> +			if (max > 0 && size >= max)
> +				break;
>  		}
>  	} while (r > 0);

I think this would be a bit cleaner:

static tsize_t copy_file_fd(struct tracecmd_output *handle, int fd, unsigned long long max)
{
	tsize_t rsize = BUFSIZ;
	tsize_t size = 0;
	char buf[BUFSIZ];
	stsize_t r;

	do {
		if (max && rsize > max)
			rsize = max;

		r = read(fd, buf, rsize);
		if (r > 0) {
			size += r;
			if (do_write_check(handle, buf, r))
				return 0;
			if (max) {
				max -= r;
				if (!max)
					break;
			}
		}
	} while (r > 0);

	return size;
}


>  
> @@ -314,12 +322,62 @@ static tsize_t copy_file(struct tracecmd_output *handle,
>  		tracecmd_warning("Can't read '%s'", file);
>  		return 0;
>  	}
> -	size = copy_file_fd(handle, fd);
> +	size = copy_file_fd(handle, fd, 0);
>  	close(fd);
>  
>  	return size;
>  }
>  
> +#define PAGES_IN_CHUNK 10
> +__hidden unsigned long long out_copy_fd_compress(struct tracecmd_output *handle,

The above name is also hard to understand. "out_copy_fd_compress"?
Would "copy_out_fd_compress()" be better?

Also, why is it __hidden and not static. it's not used outside this file.

If it gets used outside this file in a follow up patch, please convert
it from static to __hidden then.

-- Steve


> +						 int fd, unsigned long long max,
> +						 unsigned long long *write_size)
> +{
> +	unsigned long long rsize = 0;
> +	unsigned long long wsize = 0;
> +	unsigned long long size;
> +	int ret;
> +
> +	if (handle->compress) {
> +		rsize = max;
> +		ret = tracecmd_compress_copy_from(handle->compress, fd,
> +						  PAGES_IN_CHUNK * handle->page_size,
> +						  &rsize, &wsize);
> +		if (ret < 0)
> +			return 0;
> +
> +		size = rsize;
> +		if (write_size)
> +			*write_size = wsize;
> +	} else {
> +		size = copy_file_fd(handle, fd, max);
> +		if (write_size)
> +			*write_size = size;
> +	}
> +
> +	return size;
> +}
> +
> +static tsize_t copy_file_compress(struct tracecmd_output *handle,
> +				  const char *file, unsigned long long *write_size)
> +{
> +	int ret;
> +	int fd;
> +
> +	fd = open(file, O_RDONLY);
> +	if (fd < 0) {
> +		tracecmd_warning("Can't read '%s'", file);
> +		return 0;
> +	}
> +
> +	ret = out_copy_fd_compress(handle, fd, 0, write_size);
> +	if (!ret)
> +		tracecmd_warning("Can't compress '%s'", file);
> +
> +	close(fd);
> +	return ret;
> +}
> +
>  /*
>   * Finds the path to the debugfs/tracing
>   * Allocates the string and stores it.
> @@ -516,7 +574,7 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
>  	endian8 = convert_endian_8(handle, size);
>  	if (do_write_check(handle, &endian8, 8))
>  		goto out_close;
> -	check_size = copy_file_fd(handle, fd);
> +	check_size = copy_file_fd(handle, fd, 0);
>  	close(fd);
>  	if (size != check_size) {
>  		tracecmd_warning("wrong size for '%s' size=%lld read=%lld", path, size, check_size);
> @@ -542,7 +600,7 @@ static int read_header_files(struct tracecmd_output *handle, bool compress)
>  	endian8 = convert_endian_8(handle, size);
>  	if (do_write_check(handle, &endian8, 8))
>  		goto out_close;
> -	check_size = copy_file_fd(handle, fd);
> +	check_size = copy_file_fd(handle, fd, 0);
>  	close(fd);
>  	if (size != check_size) {
>  		tracecmd_warning("wrong size for '%s'", path);
> @@ -1984,7 +2042,7 @@ __hidden int out_write_cpu_data(struct tracecmd_output *handle,
>  		if (lseek64(data[i].fd, data[i].offset, SEEK_SET) == (off64_t)-1)
>  			goto out_free;
>  		if (data[i].size) {
> -			read_size = copy_file_fd(handle, data[i].fd);
> +			read_size = copy_file_fd(handle, data[i].fd, data[i].size);
>  			if (read_size != data_files[i].file_size) {
>  				errno = EINVAL;
>  				tracecmd_warning("did not match size of %lld to %lld",