On Thu, 29 Jul 2021 08:08:56 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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",
|