Add missing GUARDED_BY to fdcache_entity (#2549)

This requires a fake GetMutex for the lock checker to understand the
control flow.  Also remove unneeded locking comments that annotation
supersede.  Follows on to #2491.

src/common.h

@@ -69,6 +69,9 @@ extern std::string    instance_name;
 #define REQUIRES(...) \
     THREAD_ANNOTATION_ATTRIBUTE(requires_capability(__VA_ARGS__))
 
+#define RETURN_CAPABILITY(...) \
+    THREAD_ANNOTATION_ATTRIBUTE(lock_returned(__VA_ARGS__))
+
 #define NO_THREAD_SAFETY_ANALYSIS \
     THREAD_ANNOTATION_ATTRIBUTE(no_thread_safety_analysis)
 

src/fdcache_entity.cpp

@@ -29,6 +29,7 @@
 
 #include "common.h"
 #include "fdcache_entity.h"
+#include "fdcache_fdinfo.h"
 #include "fdcache_stat.h"
 #include "fdcache_untreated.h"
 #include "fdcache.h"
@@ -1001,11 +1002,6 @@ bool FdEntity::SetAllStatus(bool is_loaded)
     if(-1 == physical_fd){
         return false;
     }
-    // [NOTE]
-    // this method is only internal use, and calling after locking.
-    // so do not lock now.
-    //
-    //const std::lock_guard<std::mutex> lock(fdent_lock);
 
     // get file size
     struct stat st{};
@@ -1410,9 +1406,6 @@ int FdEntity::RowFlushHasLock(int fd, const char* tpath, bool force_sync)
     return result;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 int FdEntity::RowFlushNoMultipart(const PseudoFdInfo* pseudo_obj, const char* tpath)
 {
     S3FS_PRN_INFO3("[tpath=%s][path=%s][pseudo_fd=%d][physical_fd=%d]", SAFESTRPTR(tpath), path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd);
@@ -1475,9 +1468,6 @@ int FdEntity::RowFlushNoMultipart(const PseudoFdInfo* pseudo_obj, const char* tp
     return result;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 int FdEntity::RowFlushMultipart(PseudoFdInfo* pseudo_obj, const char* tpath)
 {
     S3FS_PRN_INFO3("[tpath=%s][path=%s][pseudo_fd=%d][physical_fd=%d]", SAFESTRPTR(tpath), path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd);
@@ -1581,9 +1571,6 @@ int FdEntity::RowFlushMultipart(PseudoFdInfo* pseudo_obj, const char* tpath)
     return result;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 int FdEntity::RowFlushMixMultipart(PseudoFdInfo* pseudo_obj, const char* tpath)
 {
     S3FS_PRN_INFO3("[tpath=%s][path=%s][pseudo_fd=%d][physical_fd=%d]", SAFESTRPTR(tpath), path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd);
@@ -1708,9 +1695,6 @@ int FdEntity::RowFlushMixMultipart(PseudoFdInfo* pseudo_obj, const char* tpath)
     return result;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 int FdEntity::RowFlushStreamMultipart(PseudoFdInfo* pseudo_obj, const char* tpath)
 {
     S3FS_PRN_INFO3("[tpath=%s][path=%s][pseudo_fd=%d][physical_fd=%d][mix_upload=%s]", SAFESTRPTR(tpath), path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd, (FdEntity::mixmultipart ? "true" : "false"));
@@ -2059,9 +2043,6 @@ ssize_t FdEntity::Write(int fd, const char* bytes, off_t start, size_t size)
     return wsize;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 ssize_t FdEntity::WriteNoMultipart(const PseudoFdInfo* pseudo_obj, const char* bytes, off_t start, size_t size)
 {
     S3FS_PRN_DBG("[path=%s][pseudo_fd=%d][physical_fd=%d][offset=%lld][size=%zu]", path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd, static_cast<long long int>(start), size);
@@ -2120,9 +2101,6 @@ ssize_t FdEntity::WriteNoMultipart(const PseudoFdInfo* pseudo_obj, const char* b
     return wsize;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 ssize_t FdEntity::WriteMultipart(PseudoFdInfo* pseudo_obj, const char* bytes, off_t start, size_t size)
 {
     S3FS_PRN_DBG("[path=%s][pseudo_fd=%d][physical_fd=%d][offset=%lld][size=%zu]", path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd, static_cast<long long int>(start), size);
@@ -2217,9 +2195,6 @@ ssize_t FdEntity::WriteMultipart(PseudoFdInfo* pseudo_obj, const char* bytes, of
     return wsize;
 }
 
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 ssize_t FdEntity::WriteMixMultipart(PseudoFdInfo* pseudo_obj, const char* bytes, off_t start, size_t size)
 {
     S3FS_PRN_DBG("[path=%s][pseudo_fd=%d][physical_fd=%d][offset=%lld][size=%zu]", path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd, static_cast<long long int>(start), size);
@@ -2299,9 +2274,6 @@ ssize_t FdEntity::WriteMixMultipart(PseudoFdInfo* pseudo_obj, const char* bytes,
 // On Stream upload, the uploading is executed in another thread when the
 // written area exceeds the maximum size of multipart upload.
 //
-// [NOTE]
-// Both fdent_lock and fdent_data_lock must be locked before calling.
-//
 ssize_t FdEntity::WriteStreamUpload(PseudoFdInfo* pseudo_obj, const char* bytes, off_t start, size_t size)
 {
     S3FS_PRN_DBG("[path=%s][pseudo_fd=%d][physical_fd=%d][offset=%lld][size=%zu]", path.c_str(), (pseudo_obj ? pseudo_obj->GetPseudoFd() : -1), physical_fd, static_cast<long long int>(start), size);
@@ -2537,9 +2509,6 @@ void FdEntity::MarkDirtyMetadata()
 
 bool FdEntity::IsDirtyMetadata() const
 {
-    // [NOTE]
-    // fdent_lock must be previously locked.
-    //
     return (pending_status_t::UPDATE_META_PENDING == pending_status);
 }
 
@@ -2555,11 +2524,6 @@ bool FdEntity::AddUntreated(off_t start, off_t size)
     return result;
 }
 
-// [NOTE]
-// An object that has already been locked with fdent_lock is passed to
-// UploadBoundaryLastUntreatedArea(), which calls this method.
-// Therefore, there is no need to lock fdent_lock in this method.
-//
 bool FdEntity::GetLastUpdateUntreatedPart(off_t& start, off_t& size) const
 {
     // Get last untreated area
@@ -2569,11 +2533,6 @@ bool FdEntity::GetLastUpdateUntreatedPart(off_t& start, off_t& size) const
     return true;
 }
 
-// [NOTE]
-// An object that has already been locked with fdent_lock is passed to
-// UploadBoundaryLastUntreatedArea(), which calls this method.
-// Therefore, there is no need to lock fdent_lock in this method.
-//
 bool FdEntity::ReplaceLastUpdateUntreatedPart(off_t front_start, off_t front_size, off_t behind_start, off_t behind_size)
 {
     if(0 < front_size){

src/fdcache_entity.h

@@ -28,10 +28,15 @@
 
 #include "common.h"
 #include "fdcache_page.h"
-#include "fdcache_fdinfo.h"
 #include "fdcache_untreated.h"
 #include "metaheader.h"
 
+//----------------------------------------------
+// Typedef
+//----------------------------------------------
+class PseudoFdInfo;
+typedef std::map<int, std::unique_ptr<PseudoFdInfo>> fdinfo_map_t;
+
 //------------------------------------------------
 // class FdEntity
 //------------------------------------------------
@@ -55,14 +60,7 @@ class FdEntity
         mutable std::mutex fdent_lock;
         std::string     path            GUARDED_BY(fdent_lock);       // object path
         int             physical_fd     GUARDED_BY(fdent_lock);       // physical file(cache or temporary file) descriptor
-
+        UntreatedParts  untreated_list  GUARDED_BY(fdent_lock);       // list of untreated parts that have been written and not yet uploaded(for streamupload)
-                                                                      // [FIXME]
-                                                                      // GetLastUpdateUntreatedPart and ReplaceLastUpdateUntreatedPart are called from
-                                                                      // PseudoFdInfo::UploadBoundaryLastUntreatedArea, but not sure how to write it between
-                                                                      // classes, so GUARDED_BY have beed removed for now.
-                                                                      //
-        UntreatedParts  untreated_list;                               // list of untreated parts that have been written and not yet uploaded(for streamupload)
-
         fdinfo_map_t    pseudo_fd_map   GUARDED_BY(fdent_lock);       // pseudo file descriptor information map
         FILE*           pfile           GUARDED_BY(fdent_lock);       // file pointer(tmp file or cache file)
         ino_t           inode           GUARDED_BY(fdent_lock);       // inode number for cache file
@@ -87,9 +85,9 @@ class FdEntity
         int NoCacheLoadAndPost(PseudoFdInfo* pseudo_obj, off_t start = 0, off_t size = 0) REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock);    // size=0 means loading to end
         PseudoFdInfo* CheckPseudoFdFlags(int fd, bool writable) REQUIRES(FdEntity::fdent_lock);
         bool IsUploading() REQUIRES(FdEntity::fdent_lock);
-        bool SetAllStatus(bool is_loaded) REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock);    // [NOTE] not locking
+        bool SetAllStatus(bool is_loaded) REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock);
         bool SetAllStatusUnloaded() REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock) { return SetAllStatus(false); }
-        int NoCachePreMultipartPost(PseudoFdInfo* pseudo_obj) REQUIRES(FdEntity::fdent_lock, fdent_data_lock);
+        int NoCachePreMultipartPost(PseudoFdInfo* pseudo_obj) REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock);
         int NoCacheMultipartPost(PseudoFdInfo* pseudo_obj, int tgfd, off_t start, off_t size) REQUIRES(FdEntity::fdent_lock);
         int NoCacheCompleteMultipartPost(PseudoFdInfo* pseudo_obj) REQUIRES(FdEntity::fdent_lock);
         int RowFlushHasLock(int fd, const char* tpath, bool force_sync) REQUIRES(FdEntity::fdent_lock, FdEntity::fdent_data_lock);
@@ -229,12 +227,11 @@ class FdEntity
         bool IsDirtyNewFile() const;
         void MarkDirtyMetadata();
 
-        // [FIXME]
+        bool GetLastUpdateUntreatedPart(off_t& start, off_t& size) const REQUIRES(FdEntity::fdent_lock);
-        // For these methods, REQUIRES(FdEntity::fdent_lock) should be specified, but since it is called back
+        bool ReplaceLastUpdateUntreatedPart(off_t front_start, off_t front_size, off_t behind_start, off_t behind_size) REQUIRES(FdEntity::fdent_lock);
-        // from PseudoFdInfo::UploadBoundaryLastUntreatedArea(), don't know how to write it.
+
-        //
+        // Intentionally unimplemented -- for lock checking only.
-        bool GetLastUpdateUntreatedPart(off_t& start, off_t& size) const;
+        std::mutex* GetMutex() RETURN_CAPABILITY(fdent_lock);
-        bool ReplaceLastUpdateUntreatedPart(off_t front_start, off_t front_size, off_t behind_start, off_t behind_size);
 };
 
 typedef std::map<std::string, std::unique_ptr<FdEntity>> fdent_map_t;           // key=path, value=unique_ptr<FdEntity>

src/fdcache_fdinfo.h

@@ -25,11 +25,11 @@
 #include <mutex>
 
 #include "common.h"
+#include "fdcache_entity.h"
 #include "psemaphore.h"
 #include "metaheader.h"
 #include "types.h"
 
-class FdEntity;
 class UntreatedParts;
 
 //------------------------------------------------
@@ -116,7 +116,7 @@ class PseudoFdInfo
         bool ParallelMultipartUploadAll(const char* path, const mp_part_list_t& to_upload_list, const mp_part_list_t& copy_list, int& result);
 
         int WaitAllThreadsExit();
-        ssize_t UploadBoundaryLastUntreatedArea(const char* path, headers_t& meta, FdEntity* pfdent);
+        ssize_t UploadBoundaryLastUntreatedArea(const char* path, headers_t& meta, FdEntity* pfdent) REQUIRES(pfdent->GetMutex());
         bool ExtractUploadPartsFromAllArea(UntreatedParts& untreated_list, mp_part_list_t& to_upload_list, mp_part_list_t& to_copy_list, mp_part_list_t& to_download_list, filepart_list_t& cancel_upload_list, bool& wait_upload_complete, off_t max_mp_size, off_t file_size, bool use_copy);
 };
 

src/sighandlers.cpp

@@ -22,6 +22,7 @@
 #include <csignal>
 #include <thread>
 
+#include "psemaphore.h"
 #include "s3fs_logger.h"
 #include "sighandlers.h"
 #include "fdcache.h"