From 84a1e209eadf96e213a82bcb2a592978ce36ffa8 Mon Sep 17 00:00:00 2001 From: Stenzek Date: Tue, 3 Dec 2024 17:17:00 +1000 Subject: [PATCH] OpenGLDevice: Lock pipeline cache on Linux Prevents multiple processes from trampling on one another. --- src/util/opengl_device.h | 12 ++++- src/util/opengl_pipeline.cpp | 88 ++++++++++++++++++++++++++---------- 2 files changed, 75 insertions(+), 25 deletions(-) diff --git a/src/util/opengl_device.h b/src/util/opengl_device.h index ba4ca9277..fa1cae9c9 100644 --- a/src/util/opengl_device.h +++ b/src/util/opengl_device.h @@ -11,11 +11,18 @@ #include "opengl_pipeline.h" #include "opengl_texture.h" -#include +#include "common/file_system.h" + #include #include #include +// Unix doesn't prevent concurrent write access, need to explicitly lock the pipeline cache. +// Don't worry about Android, it's not like you can run one more than one instance of the app there... +#if !defined(_WIN32) && !defined(__ANDROID__) +#define OPENGL_PIPELINE_CACHE_NEEDS_LOCK 1 +#endif + class OpenGLPipeline; class OpenGLStreamBuffer; class OpenGLTexture; @@ -232,6 +239,9 @@ private: bool m_timestamp_query_started = false; std::FILE* m_pipeline_disk_cache_file = nullptr; +#ifdef OPENGL_PIPELINE_CACHE_NEEDS_LOCK + FileSystem::POSIXLock m_pipeline_disk_cache_file_lock; +#endif u32 m_pipeline_disk_cache_data_end = 0; bool m_pipeline_disk_cache_changed = false; diff --git a/src/util/opengl_pipeline.cpp b/src/util/opengl_pipeline.cpp index 4f36aba73..ec31f4147 100644 --- a/src/util/opengl_pipeline.cpp +++ b/src/util/opengl_pipeline.cpp @@ -761,12 +761,22 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) { DebugAssert(!m_pipeline_disk_cache_file); - std::FILE* fp = FileSystem::OpenCFile(path.c_str(), "r+b", error); + auto fp = FileSystem::OpenManagedCFile(path.c_str(), "r+b", error); if (!fp) return false; +#ifdef OPENGL_PIPELINE_CACHE_NEEDS_LOCK + // Unix doesn't prevent concurrent write access, need to explicitly lock it. + FileSystem::POSIXLock fp_lock(fp.get(), true, error); + if (!fp_lock.IsLocked()) + { + Error::AddPrefix(error, "Failed to lock cache file: "); + return false; + } +#endif + // Read footer. - const s64 size = FileSystem::FSize64(fp); + const s64 size = FileSystem::FSize64(fp.get()); if (size < static_cast(sizeof(PipelineDiskCacheFooter)) || size >= static_cast(std::numeric_limits::max())) { @@ -775,11 +785,10 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) } PipelineDiskCacheFooter file_footer; - if (FileSystem::FSeek64(fp, size - sizeof(PipelineDiskCacheFooter), SEEK_SET) != 0 || - std::fread(&file_footer, sizeof(file_footer), 1, fp) != 1) + if (FileSystem::FSeek64(fp.get(), size - sizeof(PipelineDiskCacheFooter), SEEK_SET) != 0 || + std::fread(&file_footer, sizeof(file_footer), 1, fp.get()) != 1) { Error::SetStringView(error, "Invalid cache file footer."); - std::fclose(fp); return false; } @@ -795,16 +804,15 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) 0) { Error::SetStringView(error, "Cache does not match expected driver/version."); - std::fclose(fp); return false; } m_pipeline_disk_cache_data_end = static_cast(size) - sizeof(PipelineDiskCacheFooter) - (sizeof(PipelineDiskCacheIndexEntry) * file_footer.num_programs); - if (m_pipeline_disk_cache_data_end < 0 || FileSystem::FSeek64(fp, m_pipeline_disk_cache_data_end, SEEK_SET) != 0) + if (m_pipeline_disk_cache_data_end < 0 || + FileSystem::FSeek64(fp.get(), m_pipeline_disk_cache_data_end, SEEK_SET) != 0) { Error::SetStringView(error, "Failed to seek to start of index entries."); - std::fclose(fp); return false; } @@ -812,12 +820,11 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) for (u32 i = 0; i < file_footer.num_programs; i++) { PipelineDiskCacheIndexEntry entry; - if (std::fread(&entry, sizeof(entry), 1, fp) != 1 || + if (std::fread(&entry, sizeof(entry), 1, fp.get()) != 1 || (static_cast(entry.offset) + static_cast(entry.compressed_size)) >= size) { Error::SetStringView(error, "Failed to read disk cache entry."); m_program_cache.clear(); - std::fclose(fp); return false; } @@ -825,7 +832,6 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) { Error::SetStringView(error, "Duplicate program in disk cache."); m_program_cache.clear(); - std::fclose(fp); return false; } @@ -840,15 +846,42 @@ bool OpenGLDevice::OpenPipelineCache(const std::string& path, Error* error) } VERBOSE_LOG("Read {} programs from disk cache.", m_program_cache.size()); - m_pipeline_disk_cache_file = fp; + m_pipeline_disk_cache_file = fp.release(); +#ifdef OPENGL_PIPELINE_CACHE_NEEDS_LOCK + m_pipeline_disk_cache_file_lock = std::move(fp_lock); +#endif return true; } bool OpenGLDevice::CreatePipelineCache(const std::string& path, Error* error) { +#ifndef OPENGL_PIPELINE_CACHE_NEEDS_LOCK m_pipeline_disk_cache_file = FileSystem::OpenCFile(path.c_str(), "w+b", error); if (!m_pipeline_disk_cache_file) return false; +#else + // Manually truncate it, that way we don't blow away another process's file on Linux. + m_pipeline_disk_cache_file = FileSystem::OpenCFile(path.c_str(), "a+b", error); + if (!m_pipeline_disk_cache_file || !FileSystem::FSeek64(m_pipeline_disk_cache_file, 0, SEEK_SET, error)) + return false; + + m_pipeline_disk_cache_file_lock = FileSystem::POSIXLock(m_pipeline_disk_cache_file, true, error); + if (!m_pipeline_disk_cache_file_lock.IsLocked()) + { + Error::AddPrefix(error, "Failed to lock cache file: "); + std::fclose(m_pipeline_disk_cache_file); + m_pipeline_disk_cache_file = nullptr; + return false; + } + + if (!FileSystem::FTruncate64(m_pipeline_disk_cache_file, 0, error)) + { + Error::AddPrefix(error, "Failed to truncate cache file: "); + m_pipeline_disk_cache_file_lock = {}; + std::fclose(m_pipeline_disk_cache_file); + m_pipeline_disk_cache_file = nullptr; + } +#endif m_pipeline_disk_cache_data_end = 0; m_pipeline_disk_cache_changed = true; @@ -982,6 +1015,9 @@ bool OpenGLDevice::DiscardPipelineCache() if (!FileSystem::FTruncate64(m_pipeline_disk_cache_file, 0, &error)) { ERROR_LOG("Failed to truncate pipeline cache: {}", error.GetDescription()); +#ifdef OPENGL_PIPELINE_CACHE_NEEDS_LOCK + m_pipeline_disk_cache_file_lock.Unlock(); +#endif std::fclose(m_pipeline_disk_cache_file); m_pipeline_disk_cache_file = nullptr; return false; @@ -994,19 +1030,25 @@ bool OpenGLDevice::DiscardPipelineCache() bool OpenGLDevice::ClosePipelineCache(const std::string& filename, Error* error) { + const auto close_cache = [this]() { +#ifdef OPENGL_PIPELINE_CACHE_NEEDS_LOCK + m_pipeline_disk_cache_file_lock.Unlock(); +#endif + std::fclose(m_pipeline_disk_cache_file); + m_pipeline_disk_cache_file = nullptr; + }; + if (!m_pipeline_disk_cache_changed) { VERBOSE_LOG("Not updating pipeline cache because it has not changed."); - std::fclose(m_pipeline_disk_cache_file); - m_pipeline_disk_cache_file = nullptr; + close_cache(); return true; } // Rewrite footer/index entries. if (!FileSystem::FSeek64(m_pipeline_disk_cache_file, m_pipeline_disk_cache_data_end, SEEK_SET, error) != 0) { - std::fclose(m_pipeline_disk_cache_file); - m_pipeline_disk_cache_file = nullptr; + close_cache(); return false; } @@ -1027,8 +1069,7 @@ bool OpenGLDevice::ClosePipelineCache(const std::string& filename, Error* error) if (std::fwrite(&entry, sizeof(entry), 1, m_pipeline_disk_cache_file) != 1) [[unlikely]] { Error::SetErrno(error, "fwrite() for entry failed: ", errno); - std::fclose(m_pipeline_disk_cache_file); - m_pipeline_disk_cache_file = nullptr; + close_cache(); return false; } @@ -1039,15 +1080,14 @@ bool OpenGLDevice::ClosePipelineCache(const std::string& filename, Error* error) FillFooter(&footer, m_shader_cache.GetVersion()); footer.num_programs = count; - if (std::fwrite(&footer, sizeof(footer), 1, m_pipeline_disk_cache_file) != 1) [[unlikely]] + if (std::fwrite(&footer, sizeof(footer), 1, m_pipeline_disk_cache_file) != 1 || + std::fflush(m_pipeline_disk_cache_file) != 0) [[unlikely]] { Error::SetErrno(error, "fwrite() for footer failed: ", errno); - std::fclose(m_pipeline_disk_cache_file); - m_pipeline_disk_cache_file = nullptr; + close_cache(); + return false; } - if (std::fclose(m_pipeline_disk_cache_file) != 0) - Error::SetErrno(error, "fclose() failed: ", errno); - m_pipeline_disk_cache_file = nullptr; + close_cache(); return true; }