From d299d5531ff234d4a9c986126b872aac4ffc4dd8 Mon Sep 17 00:00:00 2001 From: Morph <39850852+Morph1984@users.noreply.github.com> Date: Mon, 5 Jul 2021 12:05:35 -0400 Subject: [PATCH 1/3] common: fs: file: Flush the file in GetSize This ensures that GetSize always retrieves the correct file size after a write operation. --- src/common/fs/file.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/common/fs/file.cpp b/src/common/fs/file.cpp index 077f34995..022780e4e 100644 --- a/src/common/fs/file.cpp +++ b/src/common/fs/file.cpp @@ -347,6 +347,9 @@ u64 IOFile::GetSize() const { return 0; } + // Flush any unwritten buffered data into the file prior to retrieving the file size. + std::fflush(file); + std::error_code ec; const auto file_size = fs::file_size(file_path, ec); From 14ab50defb2c6b06aec9a1401154fc57e662fc9d Mon Sep 17 00:00:00 2001 From: Morph <39850852+Morph1984@users.noreply.github.com> Date: Mon, 5 Jul 2021 12:32:14 -0400 Subject: [PATCH 2/3] common: fs: file: Revert Flush to its previous behavior and add Commit It became apparent that logging can continuously spam errors that trigger file flushing. Since committing the files to disk is an expensive operation, this causes unnecessarily high disk usage. As such, we will revert Flush() to the previous behavior and add a Commit() member function in the event that this behavior is needed. --- src/common/fs/file.cpp | 26 ++++++++++++++++++++++++-- src/common/fs/file.h | 11 ++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/common/fs/file.cpp b/src/common/fs/file.cpp index 022780e4e..274f57659 100644 --- a/src/common/fs/file.cpp +++ b/src/common/fs/file.cpp @@ -306,9 +306,9 @@ bool IOFile::Flush() const { errno = 0; #ifdef _WIN32 - const auto flush_result = std::fflush(file) == 0 && _commit(fileno(file)) == 0; + const auto flush_result = std::fflush(file) == 0; #else - const auto flush_result = std::fflush(file) == 0 && fsync(fileno(file)) == 0; + const auto flush_result = std::fflush(file) == 0; #endif if (!flush_result) { @@ -320,6 +320,28 @@ bool IOFile::Flush() const { return flush_result; } +bool IOFile::Commit() const { + if (!IsOpen()) { + return false; + } + + errno = 0; + +#ifdef _WIN32 + const auto commit_result = std::fflush(file) == 0 && _commit(fileno(file)) == 0; +#else + const auto commit_result = std::fflush(file) == 0 && fsync(fileno(file)) == 0; +#endif + + if (!commit_result) { + const auto ec = std::error_code{errno, std::generic_category()}; + LOG_ERROR(Common_Filesystem, "Failed to commit the file at path={}, ec_message={}", + PathToUTF8String(file_path), ec.message()); + } + + return commit_result; +} + bool IOFile::SetSize(u64 size) const { if (!IsOpen()) { return false; diff --git a/src/common/fs/file.h b/src/common/fs/file.h index 588fe619d..2c4ab4332 100644 --- a/src/common/fs/file.h +++ b/src/common/fs/file.h @@ -396,12 +396,21 @@ public: [[nodiscard]] size_t WriteString(std::span string) const; /** - * Attempts to flush any unwritten buffered data into the file and flush the file into the disk. + * Attempts to flush any unwritten buffered data into the file. * * @returns True if the flush was successful, false otherwise. */ bool Flush() const; + /** + * Attempts to commit the file into the disk. + * Note that this is an expensive operation as this forces the operating system to write + * the contents of the file associated with the file descriptor into the disk. + * + * @returns True if the commit was successful, false otherwise. + */ + bool Commit() const; + /** * Resizes the file to a given size. * If the file is resized to a smaller size, the remainder of the file is discarded. From a59ae5e702a9fed1626e8815b2208acc7f373e22 Mon Sep 17 00:00:00 2001 From: Morph <39850852+Morph1984@users.noreply.github.com> Date: Mon, 5 Jul 2021 12:54:06 -0400 Subject: [PATCH 3/3] common: logging: backend: Close the file after exceeding the write limit There's no point in keeping the file open after the write limit is exceeded. This allows the file to be committed to the disk shortly after it is closed and avoids redundantly checking whether or not the write limit is exceeded. --- src/common/logging/backend.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/common/logging/backend.cpp b/src/common/logging/backend.cpp index b6fa4affb..61dddab3f 100644 --- a/src/common/logging/backend.cpp +++ b/src/common/logging/backend.cpp @@ -171,19 +171,22 @@ FileBackend::FileBackend(const std::filesystem::path& filename) { FileBackend::~FileBackend() = default; void FileBackend::Write(const Entry& entry) { - using namespace Common::Literals; - // prevent logs from going over the maximum size (in case its spamming and the user doesn't - // know) - constexpr std::size_t MAX_BYTES_WRITTEN = 100_MiB; - constexpr std::size_t MAX_BYTES_WRITTEN_EXTENDED = 1_GiB; - if (!file->IsOpen()) { return; } - if (Settings::values.extended_logging && bytes_written > MAX_BYTES_WRITTEN_EXTENDED) { - return; - } else if (!Settings::values.extended_logging && bytes_written > MAX_BYTES_WRITTEN) { + using namespace Common::Literals; + // Prevent logs from exceeding a set maximum size in the event that log entries are spammed. + constexpr std::size_t MAX_BYTES_WRITTEN = 100_MiB; + constexpr std::size_t MAX_BYTES_WRITTEN_EXTENDED = 1_GiB; + + const bool write_limit_exceeded = + bytes_written > MAX_BYTES_WRITTEN_EXTENDED || + (bytes_written > MAX_BYTES_WRITTEN && !Settings::values.extended_logging); + + // Close the file after the write limit is exceeded. + if (write_limit_exceeded) { + file->Close(); return; }