From 1a50b9a04a82678b05e36927c323f42d94ca4a07 Mon Sep 17 00:00:00 2001 From: Takeshi Nakatani Date: Sun, 23 Jun 2024 16:29:17 +0000 Subject: [PATCH] Fixed a deadlock in the FdManager::ChangeEntityToTempPath method call --- src/fdcache.cpp | 61 ++++++++++++++++++++++++++++++++++++-------- src/fdcache.h | 5 +++- src/fdcache_entity.h | 3 ++- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/fdcache.cpp b/src/fdcache.cpp index 1b7d9ff..16cb501 100644 --- a/src/fdcache.cpp +++ b/src/fdcache.cpp @@ -78,6 +78,7 @@ FdManager FdManager::singleton; std::mutex FdManager::fd_manager_lock; std::mutex FdManager::cache_cleanup_lock; std::mutex FdManager::reserved_diskspace_lock; +std::mutex FdManager::except_entmap_lock; std::string FdManager::cache_dir; bool FdManager::check_cache_dir_exist(false); off_t FdManager::free_disk_space = 0; @@ -465,6 +466,7 @@ FdManager::~FdManager() S3FS_PRN_WARN("To exit with the cache file opened: path=%s, refcnt=%d", ent->GetPath().c_str(), ent->GetOpenCount()); } fent.clear(); + except_fent.clear(); }else{ abort(); } @@ -478,6 +480,8 @@ FdEntity* FdManager::GetFdEntityHasLock(const char* path, int& existfd, bool new return nullptr; } + UpdateEntityToTempPath(); + fdent_map_t::iterator iter = fent.find(path); if(fent.end() != iter && iter->second){ if(-1 == existfd){ @@ -534,6 +538,8 @@ FdEntity* FdManager::Open(int& fd, const char* path, const headers_t* pmeta, off const std::lock_guard lock(FdManager::fd_manager_lock); + UpdateEntityToTempPath(); + // search in mapping by key(path) fdent_map_t::iterator iter = fent.find(path); if(fent.end() == iter && !force_tmpfile && !FdManager::IsCacheDir()){ @@ -620,6 +626,8 @@ FdEntity* FdManager::GetExistFdEntity(const char* path, int existfd) const std::lock_guard lock(FdManager::fd_manager_lock); + UpdateEntityToTempPath(); + // search from all entity. for(fdent_map_t::iterator iter = fent.begin(); iter != fent.end(); ++iter){ if(iter->second && iter->second->FindPseudoFd(existfd)){ @@ -656,6 +664,8 @@ int FdManager::GetPseudoFdCount(const char* path) return 0; } + UpdateEntityToTempPath(); + // search from all entity. for(fdent_map_t::iterator iter = fent.begin(); iter != fent.end(); ++iter){ if(iter->second && iter->second->GetPath() == path){ @@ -671,6 +681,8 @@ void FdManager::Rename(const std::string &from, const std::string &to) { const std::lock_guard lock(FdManager::fd_manager_lock); + UpdateEntityToTempPath(); + fdent_map_t::iterator iter = fent.find(from); if(fent.end() == iter && !FdManager::IsCacheDir()){ // If the cache directory is not specified, s3fs opens a temporary file @@ -715,6 +727,8 @@ bool FdManager::Close(FdEntity* ent, int fd) } const std::lock_guard lock(FdManager::fd_manager_lock); + UpdateEntityToTempPath(); + for(fdent_map_t::iterator iter = fent.begin(); iter != fent.end(); ++iter){ if(iter->second.get() == ent){ ent->Close(fd); @@ -737,23 +751,46 @@ bool FdManager::Close(FdEntity* ent, int fd) return false; } -bool FdManager::ChangeEntityToTempPath(const FdEntity* ent, const char* path) +bool FdManager::ChangeEntityToTempPath(FdEntity* ent, const char* path) { - const std::lock_guard lock(FdManager::fd_manager_lock); + const std::lock_guard lock(FdManager::except_entmap_lock); + except_fent[path] = ent; + return true; +} - for(fdent_map_t::iterator iter = fent.begin(); iter != fent.end(); ){ - if(iter->second.get() == ent){ - std::string tmppath; - FdManager::MakeRandomTempPath(path, tmppath); +// [NOTE] +// FdManager::fd_manager_lock should be locked by the caller. +// +bool FdManager::UpdateEntityToTempPath() +{ + const std::lock_guard lock(FdManager::except_entmap_lock); + + for(fdent_direct_map_t::iterator except_iter = except_fent.begin(); except_iter != except_fent.end(); ){ + std::string tmppath; + FdManager::MakeRandomTempPath(except_iter->first.c_str(), tmppath); + + fdent_map_t::iterator iter = fent.find(except_iter->first); + if(fent.end() != iter && iter->second.get() == except_iter->second){ // Move the entry to the new key fent[tmppath] = std::move(iter->second); - iter = fent.erase(iter); - return true; + iter = fent.erase(iter); + except_iter = except_fent.erase(except_iter); }else{ - ++iter; + // [NOTE] + // ChangeEntityToTempPath method is called and the FdEntity pointer + // set into except_fent is mapped into fent. + // And since this method is always called before manipulating fent, + // it will not enter here. + // Thus, if it enters here, a warning is output. + // + S3FS_PRN_WARN("For some reason the FdEntity pointer(for %s) is not found in the fent map. Recovery procedures are being performed, but the cause needs to be identified.", except_iter->first.c_str()); + + // Add the entry for recovery procedures + fent[tmppath] = std::unique_ptr(except_iter->second); + except_iter = except_fent.erase(except_iter); } } - return false; + return true; } void FdManager::CleanupCacheDir() @@ -807,6 +844,8 @@ void FdManager::CleanupCacheDirInternal(const std::string &path) S3FS_PRN_INFO("could not get fd_manager_lock when clean up file(%s), then skip it.", next_path.c_str()); continue; } + UpdateEntityToTempPath(); + fdent_map_t::iterator iter = fent.find(next_path); if(fent.end() == iter) { S3FS_PRN_DBG("cleaned up: %s", next_path.c_str()); @@ -917,6 +956,8 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const { const std::lock_guard lock(FdManager::fd_manager_lock); + UpdateEntityToTempPath(); + fdent_map_t::iterator iter = fent.find(object_file_path); if(fent.end() != iter){ // This file is opened now, then we need to put warning message. diff --git a/src/fdcache.h b/src/fdcache.h index f8fb78d..f86260d 100644 --- a/src/fdcache.h +++ b/src/fdcache.h @@ -36,6 +36,7 @@ class FdManager static std::mutex fd_manager_lock; static std::mutex cache_cleanup_lock; static std::mutex reserved_diskspace_lock; + static std::mutex except_entmap_lock; static std::string cache_dir; static bool check_cache_dir_exist; static off_t free_disk_space; // limit free disk space @@ -46,6 +47,7 @@ class FdManager static std::string tmp_dir; fdent_map_t fent; + fdent_direct_map_t except_fent; // A map of delayed deletion fdentity private: static off_t GetFreeDiskSpace(const char* path); @@ -54,6 +56,7 @@ class FdManager static int GetVfsStat(const char* path, struct statvfs* vfsbuf); int GetPseudoFdCount(const char* path); + bool UpdateEntityToTempPath(); void CleanupCacheDirInternal(const std::string &path = ""); bool RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const char* sub_path, int& total_file_cnt, int& err_file_cnt, int& err_dir_cnt); @@ -106,7 +109,7 @@ class FdManager FdEntity* OpenExistFdEntity(const char* path, int& fd, int flags = O_RDONLY); void Rename(const std::string &from, const std::string &to); bool Close(FdEntity* ent, int fd); - bool ChangeEntityToTempPath(const FdEntity* ent, const char* path); + bool ChangeEntityToTempPath(FdEntity* ent, const char* path); void CleanupCacheDir(); bool CheckAllCache(); diff --git a/src/fdcache_entity.h b/src/fdcache_entity.h index 28cf44f..fdd8998 100644 --- a/src/fdcache_entity.h +++ b/src/fdcache_entity.h @@ -219,7 +219,8 @@ class FdEntity bool ReplaceLastUpdateUntreatedPart(off_t front_start, off_t front_size, off_t behind_start, off_t behind_size); }; -typedef std::map> fdent_map_t; // key=path, value=FdEntity* +typedef std::map> fdent_map_t; // key=path, value=unique_ptr +typedef std::map fdent_direct_map_t; // key=path, value=FdEntity* #endif // S3FS_FDCACHE_ENTITY_H_