Fix fd leak in RingBuffer; add fd tracking to find leaks (default off…

…; opt-in). (#154)

* Fix fd leak in RingBuffer; add fd tracking to find leaks (default off; opt-in).

* CR feedback (#154)

src/llfs/filesystem.cpp

@@ -9,6 +9,8 @@
 #include <llfs/filesystem.hpp>
 //
 
+#include <llfs/track_fds.hpp>
+
 #include <batteries/finally.hpp>
 #include <batteries/syscall_retry.hpp>
 
@@ -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<int> 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<int> 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<int> 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<int> 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));
 

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<int> open_file_read_only(std::string_view file_name,
                                   OpenRawIO open_raw_io = OpenRawIO{false});
 

src/llfs/ioring.test.cpp

@@ -69,7 +69,8 @@ TEST(IoRingTest, Test)
   StatusOr<IoRing> 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<char, 1023> buf;
@@ -125,8 +126,8 @@ TEST(IoRingTest, DISABLED_BlockDev)
   StatusOr<IoRing> 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};

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();
 }
 

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 <llfs/ioring_file.hpp>
+//
+#include <llfs/ioring_file.hpp>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <llfs/filesystem.hpp>
+#include <llfs/track_fds.hpp>
+
+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<llfs::ScopedIoRing> 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<std::set<int>> before_fds = llfs::get_open_fds();
+    ASSERT_TRUE(before_fds.ok()) << BATT_INSPECT(before_fds.status());
+    {
+      StatusOr<int> 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<std::set<int>> 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<std::set<int>> 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

src/llfs/ioring_file_runtime_options.cpp

@@ -13,6 +13,8 @@
 #include <llfs/ioring_file_runtime_options.hpp>
 //
 
+#include <llfs/filesystem.hpp>
+
 namespace llfs {
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
@@ -54,7 +56,7 @@ StatusOr<IoRing::File> 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));
 

src/llfs/ioring_impl.cpp

@@ -12,6 +12,7 @@
 #ifndef LLFS_DISABLE_IO_URING
 
 #include <llfs/logging.hpp>
+#include <llfs/track_fds.hpp>
 
 #include <batteries/finally.hpp>
 
@@ -57,7 +58,7 @@ StatusOr<i32> 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());

src/llfs/log_device_config.cpp

@@ -86,7 +86,7 @@ StatusOr<std::unique_ptr<IoRingLogDeviceFactory>> 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));
 

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));

src/llfs/ring_buffer.cpp

@@ -9,7 +9,9 @@
 #include <llfs/ring_buffer.hpp>
 //
 
+#include <llfs/filesystem.hpp>
 #include <llfs/status.hpp>
+#include <llfs/track_fds.hpp>
 
 #include <batteries/checked_cast.hpp>
 #include <batteries/env.hpp>
@@ -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;
+      }
     }
   }