From 671e39d79a1ad9c3f13c4784a26710a5b1f14237 Mon Sep 17 00:00:00 2001 From: Michael Jeanson Date: Tue, 17 Jan 2023 17:49:42 -0500 Subject: [PATCH] Remove fcntl wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Replace the questionnable sync_file_range wrapper by more descriptive util functions. Remove the unimplemented splice wrapper, I'd rather have a build failure / adjust the build system on some platforms than have hard to diagnose runtime issues. Change-Id: I4114d0d9765ae3d95a1488c945e5d66a20c2029d Signed-off-by: Michael Jeanson Signed-off-by: Jérémie Galarneau --- src/bin/lttng-sessiond/agent-thread.cpp | 2 + src/bin/lttng-sessiond/client.cpp | 1 + src/bin/lttng-sessiond/health.cpp | 1 + src/bin/lttng-sessiond/manage-apps.cpp | 2 + src/bin/lttng-sessiond/manage-consumer.cpp | 1 + src/bin/lttng-sessiond/manage-kernel.cpp | 2 + src/bin/lttng-sessiond/notify-apps.cpp | 2 + src/bin/lttng-sessiond/register.cpp | 1 + src/bin/lttng-sessiond/rotation-thread.cpp | 1 + src/bin/lttng-sessiond/save.cpp | 1 + src/bin/lttng-sessiond/thread-utils.cpp | 1 + src/common/Makefile.am | 4 +- src/common/compat/compat-fcntl.cpp | 25 --- src/common/compat/fcntl.hpp | 72 -------- src/common/compat/poll.hpp | 1 - src/common/consumer/consumer-stream.cpp | 1 + src/common/consumer/consumer.cpp | 36 +--- src/common/consumer/consumer.hpp | 1 - src/common/io-hint.cpp | 165 ++++++++++++++++++ src/common/io-hint.hpp | 21 +++ .../kernel-consumer/kernel-consumer.cpp | 2 +- src/common/ust-consumer/ust-consumer.cpp | 2 +- tests/unit/test_payload.cpp | 2 +- tests/unit/test_unix_socket.cpp | 2 +- 24 files changed, 214 insertions(+), 135 deletions(-) delete mode 100644 src/common/compat/compat-fcntl.cpp delete mode 100644 src/common/compat/fcntl.hpp create mode 100644 src/common/io-hint.cpp create mode 100644 src/common/io-hint.hpp diff --git a/src/bin/lttng-sessiond/agent-thread.cpp b/src/bin/lttng-sessiond/agent-thread.cpp index f641ed78e..23a26a806 100644 --- a/src/bin/lttng-sessiond/agent-thread.cpp +++ b/src/bin/lttng-sessiond/agent-thread.cpp @@ -22,6 +22,8 @@ #include #include +#include + namespace { struct thread_notifiers { struct lttng_pipe *quit_pipe; diff --git a/src/bin/lttng-sessiond/client.cpp b/src/bin/lttng-sessiond/client.cpp index 6c3f696f0..d6d894cfb 100644 --- a/src/bin/lttng-sessiond/client.cpp +++ b/src/bin/lttng-sessiond/client.cpp @@ -40,6 +40,7 @@ #include #include +#include #include #include #include diff --git a/src/bin/lttng-sessiond/health.cpp b/src/bin/lttng-sessiond/health.cpp index dd3d9426d..4aa427562 100644 --- a/src/bin/lttng-sessiond/health.cpp +++ b/src/bin/lttng-sessiond/health.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include diff --git a/src/bin/lttng-sessiond/manage-apps.cpp b/src/bin/lttng-sessiond/manage-apps.cpp index 27fa6ce85..68e70676a 100644 --- a/src/bin/lttng-sessiond/manage-apps.cpp +++ b/src/bin/lttng-sessiond/manage-apps.cpp @@ -13,6 +13,8 @@ #include "thread.hpp" #include "utils.hpp" +#include + namespace { struct thread_notifiers { struct lttng_pipe *quit_pipe; diff --git a/src/bin/lttng-sessiond/manage-consumer.cpp b/src/bin/lttng-sessiond/manage-consumer.cpp index e1d67f5bb..b7eb10800 100644 --- a/src/bin/lttng-sessiond/manage-consumer.cpp +++ b/src/bin/lttng-sessiond/manage-consumer.cpp @@ -17,6 +17,7 @@ #include #include +#include #include namespace { diff --git a/src/bin/lttng-sessiond/manage-kernel.cpp b/src/bin/lttng-sessiond/manage-kernel.cpp index 74bcb1322..fe7f21c6f 100644 --- a/src/bin/lttng-sessiond/manage-kernel.cpp +++ b/src/bin/lttng-sessiond/manage-kernel.cpp @@ -19,6 +19,8 @@ #include #include +#include + namespace { struct thread_notifiers { struct lttng_pipe *quit_pipe; diff --git a/src/bin/lttng-sessiond/notify-apps.cpp b/src/bin/lttng-sessiond/notify-apps.cpp index a73e9df58..03ac5866c 100644 --- a/src/bin/lttng-sessiond/notify-apps.cpp +++ b/src/bin/lttng-sessiond/notify-apps.cpp @@ -18,6 +18,8 @@ #include #include +#include + namespace { struct thread_notifiers { struct lttng_pipe *quit_pipe; diff --git a/src/bin/lttng-sessiond/register.cpp b/src/bin/lttng-sessiond/register.cpp index 3740b380a..0191adc94 100644 --- a/src/bin/lttng-sessiond/register.cpp +++ b/src/bin/lttng-sessiond/register.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include diff --git a/src/bin/lttng-sessiond/rotation-thread.cpp b/src/bin/lttng-sessiond/rotation-thread.cpp index 1f82176f8..9c92f66ab 100644 --- a/src/bin/lttng-sessiond/rotation-thread.cpp +++ b/src/bin/lttng-sessiond/rotation-thread.cpp @@ -44,6 +44,7 @@ #include #include +#include #include #include #include diff --git a/src/bin/lttng-sessiond/save.cpp b/src/bin/lttng-sessiond/save.cpp index e8205bf7f..b4ca4c080 100644 --- a/src/bin/lttng-sessiond/save.cpp +++ b/src/bin/lttng-sessiond/save.cpp @@ -22,6 +22,7 @@ #include +#include #include #include #include diff --git a/src/bin/lttng-sessiond/thread-utils.cpp b/src/bin/lttng-sessiond/thread-utils.cpp index 3231d8acf..c861e0f52 100644 --- a/src/bin/lttng-sessiond/thread-utils.cpp +++ b/src/bin/lttng-sessiond/thread-utils.cpp @@ -12,6 +12,7 @@ #include +#include #include /* diff --git a/src/common/Makefile.am b/src/common/Makefile.am index 5e5f6d5e7..e04b38347 100644 --- a/src/common/Makefile.am +++ b/src/common/Makefile.am @@ -89,6 +89,8 @@ libcommon_lgpl_la_SOURCES = \ file-descriptor.hpp file-descriptor.cpp \ fd-handle.cpp fd-handle.hpp\ format.hpp \ + io-hint.cpp \ + io-hint.hpp \ kernel-probe.cpp \ location.cpp \ locked-reference.hpp \ @@ -169,13 +171,11 @@ libcommon_gpl_la_LIBADD = \ # libcompat noinst_LTLIBRARIES += libcompat.la libcompat_la_SOURCES = \ - compat/compat-fcntl.cpp \ compat/directory-handle.cpp \ compat/directory-handle.hpp \ compat/dirent.hpp \ compat/endian.hpp \ compat/errno.hpp \ - compat/fcntl.hpp \ compat/getenv.hpp \ compat/mman.hpp \ compat/netdb.hpp \ diff --git a/src/common/compat/compat-fcntl.cpp b/src/common/compat/compat-fcntl.cpp deleted file mode 100644 index 18b6fa382..000000000 --- a/src/common/compat/compat-fcntl.cpp +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright (C) 2011 David Goulet - * - * SPDX-License-Identifier: LGPL-2.1-only - * - */ - -#define _LGPL_SOURCE -#include -#include - -#include - -#ifdef __linux__ - -int compat_sync_file_range(int fd, off_t offset, off_t nbytes, unsigned int flags) -{ -#ifdef HAVE_SYNC_FILE_RANGE - return sync_file_range(fd, offset, nbytes, flags); -#else - return fdatasync(fd); -#endif -} - -#endif /* __linux__ */ diff --git a/src/common/compat/fcntl.hpp b/src/common/compat/fcntl.hpp deleted file mode 100644 index ba8f05a95..000000000 --- a/src/common/compat/fcntl.hpp +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright (C) 2011 David Goulet - * - * SPDX-License-Identifier: LGPL-2.1-only - * - */ - -#ifndef _COMPAT_FCNTL_H -#define _COMPAT_FCNTL_H - -#include - -#include -#include - -static_assert(sizeof(off_t) == sizeof(int64_t), - "Build system is misconfigured, off_t must be 64-bit wide"); - -#if (defined(__FreeBSD__) || defined(__sun__)) -typedef off64_t loff_t; -#endif - -#ifdef __linux__ -extern int compat_sync_file_range(int fd, off_t offset, off_t nbytes, unsigned int flags); -#define lttng_sync_file_range(fd, offset, nbytes, flags) \ - compat_sync_file_range(fd, offset, nbytes, flags) - -#endif /* __linux__ */ - -#if (defined(__FreeBSD__) || defined(__CYGWIN__) || defined(__sun__)) -/* - * Possible flags under Linux. Simply nullify them and avoid wrapper. - */ -#define SYNC_FILE_RANGE_WAIT_AFTER 0 -#define SYNC_FILE_RANGE_WAIT_BEFORE 0 -#define SYNC_FILE_RANGE_WRITE 0 - -static inline int lttng_sync_file_range(int fd __attribute__((unused)), - off_t offset __attribute__((unused)), - off_t nbytes __attribute__((unused)), - unsigned int flags __attribute__((unused))) -{ - return -ENOSYS; -} -#endif - -#if (defined(__FreeBSD__) || defined(__CYGWIN__) || defined(__sun__)) -/* - * Possible flags under Linux. Simply nullify them and avoid wrappers. - */ -#define SPLICE_F_MOVE 0 -#define SPLICE_F_NONBLOCK 0 -#define SPLICE_F_MORE 0 -#define SPLICE_F_GIFT 0 - -static inline ssize_t splice(int fd_in __attribute__((unused)), - loff_t *off_in __attribute__((unused)), - int fd_out __attribute__((unused)), - loff_t *off_out __attribute__((unused)), - size_t len __attribute__((unused)), - unsigned int flags __attribute__((unused))) -{ - return -ENOSYS; -} -#endif - -#if !(defined(__linux__) || defined(__FreeBSD__) || defined(__CYGWIN__) || defined(__sun__) || \ - defined(__APPLE__)) -#error "Please add support for your OS." -#endif /* __linux__ , __FreeBSD__, __CYGWIN__, __sun__, __APPLE__ */ - -#endif /* _COMPAT_FCNTL_H */ diff --git a/src/common/compat/poll.hpp b/src/common/compat/poll.hpp index bfb3da82b..da3391106 100644 --- a/src/common/compat/poll.hpp +++ b/src/common/compat/poll.hpp @@ -25,7 +25,6 @@ static inline void __lttng_poll_free(void *events) * epoll(7) implementation. */ #ifdef HAVE_EPOLL -#include #include #include diff --git a/src/common/consumer/consumer-stream.cpp b/src/common/consumer/consumer-stream.cpp index ab9b39111..0742c8652 100644 --- a/src/common/consumer/consumer-stream.cpp +++ b/src/common/consumer/consumer-stream.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include diff --git a/src/common/consumer/consumer.cpp b/src/common/consumer/consumer.cpp index 5f3dc4e62..01845871b 100644 --- a/src/common/consumer/consumer.cpp +++ b/src/common/consumer/consumer.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -34,6 +35,7 @@ #include #include +#include #include #include #include @@ -1336,7 +1338,6 @@ void lttng_consumer_should_exit(struct lttng_consumer_local_data *ctx) */ static void lttng_consumer_sync_trace_file(struct lttng_consumer_stream *stream, off_t orig_offset) { - int ret; int outfd = stream->out_fd; /* @@ -1348,31 +1349,8 @@ static void lttng_consumer_sync_trace_file(struct lttng_consumer_stream *stream, if (orig_offset < stream->max_sb_size) { return; } - lttng_sync_file_range(outfd, - orig_offset - stream->max_sb_size, - stream->max_sb_size, - SYNC_FILE_RANGE_WAIT_BEFORE | SYNC_FILE_RANGE_WRITE | - SYNC_FILE_RANGE_WAIT_AFTER); - /* - * Give hints to the kernel about how we access the file: - * POSIX_FADV_DONTNEED : we won't re-access data in a near future after - * we write it. - * - * We need to call fadvise again after the file grows because the - * kernel does not seem to apply fadvise to non-existing parts of the - * file. - * - * Call fadvise _after_ having waited for the page writeback to - * complete because the dirty page writeback semantic is not well - * defined. So it can be expected to lead to lower throughput in - * streaming. - */ - ret = posix_fadvise( - outfd, orig_offset - stream->max_sb_size, stream->max_sb_size, POSIX_FADV_DONTNEED); - if (ret && ret != -ENOSYS) { - errno = ret; - PERROR("posix_fadvise on fd %i", outfd); - } + lttng::io::hint_flush_range_dont_need_sync( + outfd, orig_offset - stream->max_sb_size, stream->max_sb_size); } /* @@ -1733,8 +1711,7 @@ ssize_t lttng_consumer_on_read_subbuffer_mmap(struct lttng_consumer_stream *stre /* This call is useless on a socket so better save a syscall. */ if (!relayd) { /* This won't block, but will start writeout asynchronously */ - lttng_sync_file_range( - outfd, stream->out_fd_offset, write_len, SYNC_FILE_RANGE_WRITE); + lttng::io::hint_flush_range_async(outfd, stream->out_fd_offset, write_len); stream->out_fd_offset += write_len; lttng_consumer_sync_trace_file(stream, orig_offset); } @@ -1933,8 +1910,7 @@ ssize_t lttng_consumer_on_read_subbuffer_splice(struct lttng_consumer_local_data /* This call is useless on a socket so better save a syscall. */ if (!relayd) { /* This won't block, but will start writeout asynchronously */ - lttng_sync_file_range( - outfd, stream->out_fd_offset, ret_splice, SYNC_FILE_RANGE_WRITE); + lttng::io::hint_flush_range_async(outfd, stream->out_fd_offset, ret_splice); stream->out_fd_offset += ret_splice; } stream->output_written += ret_splice; diff --git a/src/common/consumer/consumer.hpp b/src/common/consumer/consumer.hpp index 1efbc4eee..053744904 100644 --- a/src/common/consumer/consumer.hpp +++ b/src/common/consumer/consumer.hpp @@ -12,7 +12,6 @@ #define LIB_CONSUMER_H #include -#include #include #include #include diff --git a/src/common/io-hint.cpp b/src/common/io-hint.cpp new file mode 100644 index 000000000..506f15de6 --- /dev/null +++ b/src/common/io-hint.cpp @@ -0,0 +1,165 @@ +/* + * Copyright (C) 2023 Michael Jeanson + * + * SPDX-License-Identifier: LGPL-2.1-only + * + */ + +#include +#include +#include + +#include +#include +#include +#include + +/* + * Use sync_file_range when available. + */ +#ifdef HAVE_SYNC_FILE_RANGE + +#include + +namespace { +int flush_range(int fd, off_t offset, off_t nbytes, unsigned int flags) +{ + int ret; + + ret = sync_file_range(fd, offset, nbytes, flags); + if (ret) { + PERROR("Failed to sync file range: fd=%i, offset=%" PRIu64 ", nbytes=%" PRIu64 + ", flags=%i", + fd, + static_cast(offset), + static_cast(nbytes), + flags); + } + + return ret; +} + +int flush_range_sync(int fd, off_t offset, off_t nbytes) +{ + return flush_range(fd, + offset, + nbytes, + SYNC_FILE_RANGE_WAIT_BEFORE | SYNC_FILE_RANGE_WRITE | + SYNC_FILE_RANGE_WAIT_AFTER); +} + +int flush_range_async(int fd, off_t offset, off_t nbytes) +{ + return flush_range(fd, offset, nbytes, SYNC_FILE_RANGE_WRITE); +} +} /* namespace */ + +#else + +namespace { +/* + * Use a memory mapping with msync() to emulate sync_file_range(). + */ +int flush_range(int fd, off_t offset, off_t nbytes, int flags) +{ + void *mapped_region = mmap(NULL, nbytes, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + if (mapped_region == MAP_FAILED) { + PERROR("Failed to mmap region to flush range: fd=%i, offset=%" PRIu64 + ", nbytes=%" PRIu64 ", flags=%i", + fd, + static_cast(offset), + static_cast(nbytes), + flags); + return -1; + } + + const auto munmap_on_exit = lttng::make_scope_exit([&]() noexcept { + const auto munmap_ret = munmap(mapped_region, nbytes); + if (munmap_ret) { + PERROR("Failed to munmap region while flushing range: fd=%i, offset=%" PRIu64 + ", nbytes=%" PRIu64 ", flags=%i", + fd, + static_cast(offset), + static_cast(nbytes), + flags); + } + }); + + const auto msync_ret = msync(mapped_region, nbytes, flags); + if (msync_ret) { + PERROR("Failed to msync region while flushing range: fd=%i, offset=%" PRIu64 + ", nbytes=%" PRIu64 ", flags=%i", + fd, + static_cast(offset), + static_cast(nbytes), + flags); + return -1; + } + + return 0; +} + +int flush_range_sync(int fd, off_t offset, off_t nbytes) +{ + return flush_range(fd, offset, nbytes, MS_SYNC); +} + +int flush_range_async(int fd, off_t offset, off_t nbytes) +{ + return flush_range(fd, offset, nbytes, MS_ASYNC); +} +} /* namespace */ +#endif + +/* + * Give a hint to the kernel that we won't need the data at the specified range + * so it can be dropped from the page cache and wait for it to be flushed to + * disk. + */ +void lttng::io::hint_flush_range_dont_need_sync(int fd, off_t offset, off_t nbytes) +{ + /* Waited for the page writeback to complete. */ + flush_range_sync(fd, offset, nbytes); + + /* + * Give hints to the kernel about how we access the file: + * POSIX_FADV_DONTNEED : we won't re-access data in a near future after + * we write it. + * + * We need to call fadvise again after the file grows because the + * kernel does not seem to apply fadvise to non-existing parts of the + * file. + * + * Call fadvise _after_ having waited for the page writeback to + * complete because the dirty page writeback semantic is not well + * defined. So it can be expected to lead to lower throughput in + * streaming. + */ + const int ret = posix_fadvise(fd, offset, nbytes, POSIX_FADV_DONTNEED); + if (ret && ret != -ENOSYS) { + PERROR("Failed to mark region as DONTNEED with posix_fadvise: fd=%i, offset=%" PRIu64 + ", nbytes=%" PRIu64, + fd, + static_cast(offset), + static_cast(nbytes)); + errno = ret; + } +} + +/* + * Give a hint to the kernel that the data at the specified range should be + * flushed to disk and wait for it to complete. + */ +void lttng::io::hint_flush_range_sync(int fd, off_t offset, off_t nbytes) +{ + flush_range_sync(fd, offset, nbytes); +} + +/* + * Give a hint to the kernel that the data at the specified range should be + * flushed to disk and return immediatly. + */ +void lttng::io::hint_flush_range_async(int fd, off_t offset, off_t nbytes) +{ + flush_range_async(fd, offset, nbytes); +} diff --git a/src/common/io-hint.hpp b/src/common/io-hint.hpp new file mode 100644 index 000000000..24fde6bbe --- /dev/null +++ b/src/common/io-hint.hpp @@ -0,0 +1,21 @@ +/* + * Copyright (C) 2023 Michael Jeanson + * + * SPDX-License-Identifier: LGPL-2.1-only + * + */ + +#ifndef _LTTNG_IO_HINT_H +#define _LTTNG_IO_HINT_H + +namespace lttng { +namespace io { + +void hint_flush_range_dont_need_sync(int fd, off_t offset, off_t nbytes); +void hint_flush_range_sync(int fd, off_t offset, off_t nbytes); +void hint_flush_range_async(int fd, off_t offset, off_t nbytes); + +} /* namespace io */ +} /* namespace lttng */ + +#endif diff --git a/src/common/kernel-consumer/kernel-consumer.cpp b/src/common/kernel-consumer/kernel-consumer.cpp index b0aa99ec1..7e8de03f6 100644 --- a/src/common/kernel-consumer/kernel-consumer.cpp +++ b/src/common/kernel-consumer/kernel-consumer.cpp @@ -13,7 +13,6 @@ #include #include #include -#include #include #include #include @@ -29,6 +28,7 @@ #include #include +#include #include #include #include diff --git a/src/common/ust-consumer/ust-consumer.cpp b/src/common/ust-consumer/ust-consumer.cpp index 48a01a352..6274cdcc5 100644 --- a/src/common/ust-consumer/ust-consumer.cpp +++ b/src/common/ust-consumer/ust-consumer.cpp @@ -12,7 +12,6 @@ #include #include -#include #include #include #include @@ -29,6 +28,7 @@ #include #include +#include #include #include #include diff --git a/tests/unit/test_payload.cpp b/tests/unit/test_payload.cpp index 2d1e01696..419fc893d 100644 --- a/tests/unit/test_payload.cpp +++ b/tests/unit/test_payload.cpp @@ -5,10 +5,10 @@ * */ -#include #include #include +#include #include #include diff --git a/tests/unit/test_unix_socket.cpp b/tests/unit/test_unix_socket.cpp index ea02c2ec2..e0af6e296 100644 --- a/tests/unit/test_unix_socket.cpp +++ b/tests/unit/test_unix_socket.cpp @@ -5,7 +5,6 @@ * */ -#include #include #include #include @@ -17,6 +16,7 @@ #include #include +#include #include #include #include -- 2.34.1