diff --git a/src/llfs/filesystem.cpp b/src/llfs/filesystem.cpp index 4cca971..172bdd7 100644 --- a/src/llfs/filesystem.cpp +++ b/src/llfs/filesystem.cpp @@ -9,6 +9,8 @@ #include // +#include + #include #include @@ -21,6 +23,24 @@ using ::batt::syscall_retry; namespace { +} //namespace + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +int system_open2(const char* path, int flags) +{ + return maybe_track_fd(::open(path, flags)); +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +int system_open3(const char* path, int flags, int mode) +{ + return maybe_track_fd(::open(path, flags, mode)); +} + +namespace { + //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // int build_open_flags(OpenForRead open_for_read, OpenForWrite open_for_write, @@ -63,7 +83,7 @@ StatusOr open_file_read_only(std::string_view file_name, OpenRawIO open_raw open_raw_io); const int fd = syscall_retry([&] { - return ::open(std::string(file_name).c_str(), flags); + return system_open2(std::string(file_name).c_str(), flags); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); @@ -82,7 +102,7 @@ StatusOr open_file_read_write(std::string_view file_name, OpenForAppend ope open_raw_io); const int fd = syscall_retry([&] { - return ::open(std::string(file_name).c_str(), flags); + return system_open2(std::string(file_name).c_str(), flags); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); @@ -100,7 +120,7 @@ StatusOr create_file_read_write(std::string_view file_name, OpenForAppend o | O_CREAT | O_EXCL | O_TRUNC; const int fd = syscall_retry([&] { - return ::open(std::string(file_name).c_str(), flags, /*mode=*/0644); + return system_open3(std::string(file_name).c_str(), flags, /*mode=*/0644); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); @@ -112,7 +132,7 @@ StatusOr create_file_read_write(std::string_view file_name, OpenForAppend o Status truncate_file(std::string_view file_name, u64 size) { const int fd = syscall_retry([&] { - return ::open(std::string(file_name).c_str(), O_RDWR | O_CREAT, 0644); + return system_open3(std::string(file_name).c_str(), O_RDWR | O_CREAT, 0644); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); diff --git a/src/llfs/filesystem.hpp b/src/llfs/filesystem.hpp index 62aab05..c58f92d 100644 --- a/src/llfs/filesystem.hpp +++ b/src/llfs/filesystem.hpp @@ -28,6 +28,16 @@ BATT_STRONG_TYPEDEF(bool, OpenForWrite); BATT_STRONG_TYPEDEF(bool, OpenForAppend); BATT_STRONG_TYPEDEF(bool, OpenRawIO); +/** \brief Wrapper for all calls to system open; this allows us to track open file descriptors and + * where they were opened, to detect leaks. + */ +int system_open2(const char* path, int flags); + +/** \brief Wrapper for all calls to system open; this allows us to track open file descriptors and + * where they were opened, to detect leaks. + */ +int system_open3(const char* path, int flags, int mode); + StatusOr open_file_read_only(std::string_view file_name, OpenRawIO open_raw_io = OpenRawIO{false}); diff --git a/src/llfs/ioring.test.cpp b/src/llfs/ioring.test.cpp index 18b4b90..1c5b04c 100644 --- a/src/llfs/ioring.test.cpp +++ b/src/llfs/ioring.test.cpp @@ -69,7 +69,8 @@ TEST(IoRingTest, Test) StatusOr io = IoRing::make_new(llfs::MaxQueueDepth{64}); ASSERT_TRUE(io.ok()) << io.status(); - int fd = open("/tmp/llfs_ioring_test_file", O_CREAT | O_RDWR | O_DIRECT | O_SYNC, /*mode=*/0644); + int fd = llfs::system_open3("/tmp/llfs_ioring_test_file", O_CREAT | O_RDWR | O_DIRECT | O_SYNC, + /*mode=*/0644); ASSERT_GE(fd, 0) << std::strerror(errno); std::array buf; @@ -125,8 +126,8 @@ TEST(IoRingTest, DISABLED_BlockDev) StatusOr io = IoRing::make_new(llfs::MaxQueueDepth{64}); ASSERT_TRUE(io.ok()) << io.status(); - int fd = open("/dev/nvme3n1", O_RDWR | O_DIRECT | O_SYNC); - int fd2 = open("/dev/nvme3n1", O_RDWR | O_DIRECT | O_SYNC); + int fd = llfs::system_open2("/dev/nvme3n1", O_RDWR | O_DIRECT | O_SYNC); + int fd2 = llfs::system_open2("/dev/nvme3n1", O_RDWR | O_DIRECT | O_SYNC); ASSERT_GE(fd, 0) << std::strerror(errno); @@ -179,7 +180,7 @@ TEST(IoRingTest, MultipleThreads) const auto file_path = "/tmp/llfs_ioring_test_file"; - int fd = open(file_path, O_CREAT | O_RDWR, /*mode=*/0644); + int fd = llfs::system_open3(file_path, O_CREAT | O_RDWR, /*mode=*/0644); ASSERT_GE(fd, 0) << std::strerror(errno); IoRing::File f{scoped_io_ring->get_io_ring(), fd}; diff --git a/src/llfs/ioring_file.cpp b/src/llfs/ioring_file.cpp index 16a1c50..d5548e0 100644 --- a/src/llfs/ioring_file.cpp +++ b/src/llfs/ioring_file.cpp @@ -50,15 +50,7 @@ auto IoRing::File::operator=(File&& that) noexcept -> File& // IoRing::File::~File() noexcept { - if (this->fd_ != -1) { - if (this->registered_fd_ != -1) { - this->unregister_fd().IgnoreError(); - } - - batt::syscall_retry([&] { - return ::close(this->fd_); - }); - } + this->close().IgnoreError(); } //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - @@ -139,10 +131,7 @@ Status IoRing::File::read_all_fixed(i64 offset, MutableBuffer buffer, int buf_in // int IoRing::File::release() { - if (this->registered_fd_ != -1) { - this->unregister_fd().IgnoreError(); - this->registered_fd_ = -1; - } + this->unregister_fd().IgnoreError(); int released = -1; std::swap(released, this->fd_); @@ -176,11 +165,12 @@ Status IoRing::File::unregister_fd() return OkStatus(); } - Status status = this->io_ring_impl_->unregister_fd(this->registered_fd_); - BATT_REQUIRE_OK(status); - + const int local_registered_fd = this->registered_fd_; this->registered_fd_ = -1; + Status status = this->io_ring_impl_->unregister_fd(local_registered_fd); + BATT_REQUIRE_OK(status) << batt::LogLevel::kError; + return OkStatus(); } diff --git a/src/llfs/ioring_file.test.cpp b/src/llfs/ioring_file.test.cpp new file mode 100644 index 0000000..e0333f4 --- /dev/null +++ b/src/llfs/ioring_file.test.cpp @@ -0,0 +1,66 @@ +//#=##=##=#==#=#==#===#+==#+==========+==+=+=+=+=+=++=+++=+++++=-++++=-+++++++++++ +// +// Part of the LLFS Project, under Apache License v2.0. +// See https://www.apache.org/licenses/LICENSE-2.0 for license information. +// SPDX short identifier: Apache-2.0 +// +//+++++++++++-+-+--+----- --- -- - - - - + +#include +// +#include + +#include +#include + +#include +#include + +namespace { + +using namespace llfs::int_types; + +using llfs::Status; +using llfs::StatusOr; + +TEST(IoringFileTest, FdLeakTest) +{ + bool saved = llfs::set_fd_tracking_enabled(true); + auto on_scope_exit = batt::finally([&] { + llfs::set_fd_tracking_enabled(saved); + }); + + StatusOr scoped_io_ring = + llfs::ScopedIoRing::make_new(llfs::MaxQueueDepth{32}, llfs::ThreadPoolSize{1}); + + ASSERT_TRUE(scoped_io_ring.ok()) << BATT_INSPECT(scoped_io_ring.status()); + + for (usize i = 0; i < 1000; ++i) { + StatusOr> before_fds = llfs::get_open_fds(); + ASSERT_TRUE(before_fds.ok()) << BATT_INSPECT(before_fds.status()); + { + StatusOr fd = llfs::open_file_read_only(__FILE__); + ASSERT_TRUE(fd.ok()) << BATT_INSPECT(fd.status()); + { + llfs::IoRing::File file{scoped_io_ring->get_io_ring(), *fd}; + + Status register_status = file.register_fd(); + + EXPECT_TRUE(register_status.ok()) << BATT_INSPECT(register_status); + + StatusOr> after_fds = llfs::get_open_fds(); + + ASSERT_TRUE(after_fds.ok()) << BATT_INSPECT(after_fds.status()); + EXPECT_NE(before_fds, after_fds) + << BATT_INSPECT_RANGE(*before_fds) << BATT_INSPECT_RANGE(*after_fds); + } + } + StatusOr> after_fds = llfs::get_open_fds(); + + ASSERT_TRUE(after_fds.ok()) << BATT_INSPECT(after_fds.status()); + EXPECT_EQ(before_fds, after_fds) + << BATT_INSPECT_RANGE(*before_fds) << BATT_INSPECT_RANGE(*after_fds) << BATT_INSPECT(i); + } +} + +} // namespace diff --git a/src/llfs/ioring_file_runtime_options.cpp b/src/llfs/ioring_file_runtime_options.cpp index 3be3d74..4167fef 100644 --- a/src/llfs/ioring_file_runtime_options.cpp +++ b/src/llfs/ioring_file_runtime_options.cpp @@ -13,6 +13,8 @@ #include // +#include + namespace llfs { //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - @@ -54,7 +56,7 @@ StatusOr open_ioring_file(const std::string& file_name, } int fd = batt::syscall_retry([&] { - return ::open(file_name.c_str(), flags); + return system_open2(file_name.c_str(), flags); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); diff --git a/src/llfs/ioring_impl.cpp b/src/llfs/ioring_impl.cpp index ebea724..58fa9d5 100644 --- a/src/llfs/ioring_impl.cpp +++ b/src/llfs/ioring_impl.cpp @@ -12,6 +12,7 @@ #ifndef LLFS_DISABLE_IO_URING #include +#include #include @@ -57,7 +58,7 @@ StatusOr status_or_i32_from_uring_retval(int retval) BATT_CHECK_EQ(impl->event_fd_, -1); LLFS_VLOG(1) << "Creating eventfd"; - impl->event_fd_.store(eventfd(0, /*flags=*/EFD_SEMAPHORE)); + impl->event_fd_.store(maybe_track_fd(eventfd(0, /*flags=*/EFD_SEMAPHORE))); if (impl->event_fd_ < 0) { LLFS_LOG_ERROR() << "failed to create eventfd: " << std::strerror(errno); return batt::status_from_retval(impl->event_fd_.load()); diff --git a/src/llfs/log_device_config.cpp b/src/llfs/log_device_config.cpp index 2334894..500dc2e 100644 --- a/src/llfs/log_device_config.cpp +++ b/src/llfs/log_device_config.cpp @@ -86,7 +86,7 @@ StatusOr> recover_storage_object( const int flags = O_DIRECT | O_SYNC | O_RDWR; int fd = batt::syscall_retry([&] { - return ::open(file_name.c_str(), flags); + return system_open2(file_name.c_str(), flags); }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); diff --git a/src/llfs/raw_block_file_impl.cpp b/src/llfs/raw_block_file_impl.cpp index 028478e..5985c78 100644 --- a/src/llfs/raw_block_file_impl.cpp +++ b/src/llfs/raw_block_file_impl.cpp @@ -33,9 +33,9 @@ namespace llfs { { const int fd = batt::syscall_retry([&] { if (mode) { - return ::open(file_name, flags, *mode); + return system_open3(file_name, flags, *mode); } else { - return ::open(file_name, flags); + return system_open2(file_name, flags); } }); BATT_REQUIRE_OK(batt::status_from_retval(fd)); diff --git a/src/llfs/ring_buffer.cpp b/src/llfs/ring_buffer.cpp index b914750..d3e0979 100644 --- a/src/llfs/ring_buffer.cpp +++ b/src/llfs/ring_buffer.cpp @@ -9,7 +9,9 @@ #include // +#include #include +#include #include #include @@ -93,13 +95,12 @@ auto RingBuffer::ImplPool::allocate(const Params& params) noexcept -> Impl const i64 id = counter.fetch_add(1); - FILE* const fp = nullptr; - - const int fd = memfd_create(Impl::memfd_name_from_id(id).c_str(), MFD_CLOEXEC); + const int fd = + maybe_track_fd(memfd_create(Impl::memfd_name_from_id(id).c_str(), MFD_CLOEXEC)); return Impl{FileDescriptor{ .fd = fd, - .fp = fp, + .fp = nullptr, .byte_size = p.byte_size, .byte_offset = 0, .truncate = true, @@ -121,7 +122,7 @@ auto RingBuffer::ImplPool::allocate(const Params& params) noexcept -> Impl flags |= O_TRUNC; } return Impl{FileDescriptor{ - .fd = ::open(p.file_name.c_str(), flags, S_IRWXU), + .fd = system_open3(p.file_name.c_str(), flags, S_IRWXU), .fp = nullptr, .byte_size = p.byte_size, .byte_offset = p.byte_offset, @@ -261,7 +262,11 @@ RingBuffer::Impl::~Impl() noexcept int local_fd = -1; std::swap(this->fd_, local_fd); - ::close(this->fd_); + Status close_status = close_fd(local_fd); + if (!close_status.ok()) { + LLFS_LOG_ERROR() << "Failed to close RingBuffer::Impl fd=" << local_fd + << "; status=" << close_status; + } } } diff --git a/src/llfs/track_fds.cpp b/src/llfs/track_fds.cpp new file mode 100644 index 0000000..f8d822e --- /dev/null +++ b/src/llfs/track_fds.cpp @@ -0,0 +1,178 @@ +//#=##=##=#==#=#==#===#+==#+==========+==+=+=+=+=+=++=+++=+++++=-++++=-+++++++++++ +// +// Part of the LLFS Project, under Apache License v2.0. +// See https://www.apache.org/licenses/LICENSE-2.0 for license information. +// SPDX short identifier: Apache-2.0 +// +//+++++++++++-+-+--+----- --- -- - - - - + +#include +// + +#include + +#include +#include + +#include +#include +#include +#include + +#ifdef __linux__ +// +#include +#include +// +#endif + +namespace llfs { + +namespace { + +const char* kVarName = "LLFS_TRACK_FDS"; + +struct TrackFdState { + std::atomic enabled{batt::getenv_as(kVarName).value_or(0) != 0}; + std::mutex mutex; + std::unordered_map> traces; + + //+++++++++++-+-+--+----- --- -- - - - - + + static TrackFdState& instance() noexcept + { + // Intentionally leaked, since if this is being deleted the process is going away anyhow, the + // destructor of std::unordered_map in this case doesn't have any side-effects external to the + // process, so it's better to keep the process tear-down as simple as possible. + // + static TrackFdState* const instance_ = new TrackFdState{}; + + return *instance_; + } +}; + +} //namespace + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +StatusOr> get_open_fds() +{ + std::set result; + const char* dirpath = "/proc/self/fd"; + + //+++++++++++-+-+--+----- --- -- - - - - +#ifdef __linux__ + // TODO [tastolfi 2024-07-01] Make Windows compatible. + // + DIR* p_dir = opendir(dirpath); + if (!p_dir) { + return batt::status_from_errno(errno); + } + + if (!p_dir) { + LLFS_LOG_ERROR() << "Could not open " << dirpath; + } else { + auto on_scope_exit = batt::finally([&] { + closedir(p_dir); + }); + + const int p_dir_fd = dirfd(p_dir); + + for (;;) { + errno = 0; + struct dirent* p_entry = readdir(p_dir); + + // If p_entry is null, we are done! + // + if (!p_entry) { + if (errno != 0) { + return batt::status_from_errno(errno); + } + break; + } + + // If the entry name parses as an integer *and* it is not the open directory we are scanning, + // then add it to the result set. + // + std::optional fd = batt::from_string(p_entry->d_name); + if (fd && *fd != p_dir_fd) { + result.emplace(*fd); + } + } + } + + //+++++++++++-+-+--+----- --- -- - - - - +#else + std::error_code ec; + std::filesystem::directory_iterator iter{dirpath, ec}; + if (ec) { + LLFS_VLOG(1) << "failed to open '/proc/self/fd': " << ec.value() << ":" << ec.message(); + return {}; + } + LLFS_VLOG(1) << "open succeeded; gathering fds"; + + for (const std::filesystem::directory_entry& entry : iter) { + LLFS_VLOG(1) << BATT_INSPECT(entry.path().stem()); + std::optional fd = batt::from_string(entry.path().stem().string()); + if (!fd) { + continue; + } + result.emplace(*fd); + } +#endif + //+++++++++++-+-+--+----- --- -- - - - - + + return result; +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +int maybe_track_fd(int fd) +{ + if (fd >= 0 && is_fd_tracking_enabled()) { + set_trace_for_fd(fd, boost::stacktrace::stacktrace{}); + } + return fd; +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +bool is_fd_tracking_enabled() +{ + return TrackFdState::instance().enabled.load(); +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +bool set_fd_tracking_enabled(bool on) +{ + return TrackFdState::instance().enabled.exchange(on); +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +Optional get_trace_for_fd(int fd) +{ + TrackFdState& state = TrackFdState::instance(); + std::unique_lock lock{state.mutex}; + auto iter = state.traces.find(fd); + if (iter == state.traces.end() || iter->second == nullptr) { + return None; + } + return *iter->second; +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +void set_trace_for_fd(int fd, Optional trace) +{ + TrackFdState& state = TrackFdState::instance(); + std::unique_lock lock{state.mutex}; + if (!trace) { + state.traces.erase(fd); + } else { + state.traces[fd] = std::make_unique(*trace); + } +} + +} //namespace llfs diff --git a/src/llfs/track_fds.hpp b/src/llfs/track_fds.hpp new file mode 100644 index 0000000..25fffb4 --- /dev/null +++ b/src/llfs/track_fds.hpp @@ -0,0 +1,53 @@ +//#=##=##=#==#=#==#===#+==#+==========+==+=+=+=+=+=++=+++=+++++=-++++=-+++++++++++ +// +// Part of the LLFS Project, under Apache License v2.0. +// See https://www.apache.org/licenses/LICENSE-2.0 for license information. +// SPDX short identifier: Apache-2.0 +// +//+++++++++++-+-+--+----- --- -- - - - - + +#pragma once +#ifndef LLFS_TRACK_FDS_HPP +#define LLFS_TRACK_FDS_HPP + +#include +// +#include +#include + +#include + +#include + +namespace llfs { + +/** \brief Returns the set of currently open file descriptors for this process. + */ +StatusOr> get_open_fds(); + +/** \brief If tracking is enabled and fd is not invalid, adds it to the tracked set. + * \return the passed fd + */ +int maybe_track_fd(int fd); + +/** \brief Returns whether fd tracking is currently enabled for this process. + */ +bool is_fd_tracking_enabled(); + +/** \brief Sets whether fd tracking is currently enabled for this process. + * \return the previous enabled status + */ +bool set_fd_tracking_enabled(bool on); + +/** \brief Returns the stack trace for the most recent open of the passed file descriptor, or None + * if no such trace exists. + */ +Optional get_trace_for_fd(int fd); + +/** \brief Sets the stack trace for the given file descriptor. + */ +void set_trace_for_fd(int fd, Optional trace); + +} //namespace llfs + +#endif // LLFS_TRACK_FDS_HPP diff --git a/src/llfs/track_fds.test.cpp b/src/llfs/track_fds.test.cpp new file mode 100644 index 0000000..76e606c --- /dev/null +++ b/src/llfs/track_fds.test.cpp @@ -0,0 +1,35 @@ +//#=##=##=#==#=#==#===#+==#+==========+==+=+=+=+=+=++=+++=+++++=-++++=-+++++++++++ +// +// Part of the LLFS Project, under Apache License v2.0. +// See https://www.apache.org/licenses/LICENSE-2.0 for license information. +// SPDX short identifier: Apache-2.0 +// +//+++++++++++-+-+--+----- --- -- - - - - + +#include +// +#include + +#include +#include + +#include + +#include + +namespace { + +TEST(TrackFdsTest, Test) +{ + llfs::StatusOr> fds = llfs::get_open_fds(); + ASSERT_TRUE(fds.ok()) << BATT_INSPECT(fds.status()); + + EXPECT_GE(fds->size(), 3u); + EXPECT_EQ(fds->count(0), 1u); + EXPECT_EQ(fds->count(1), 1u); + EXPECT_EQ(fds->count(2), 1u); + + LLFS_LOG_INFO() << batt::dump_range(*fds); +} + +} // namespace