Remove recursive locking

Recursive locking is frowned upon and is incompatible with
PTHREAD_MUTEX_ERRORCHECK.  Also clean up pthread_mutex_lock error
checking.

.travis.yml

@@ -33,7 +33,7 @@ matrix:
         - sudo pip install --upgrade awscli
       script:
         - ./autogen.sh
-        - ./configure CPPFLAGS='-I/usr/local/opt/openssl/include' CXXFLAGS='-std=c++03'
+        - ./configure CPPFLAGS='-I/usr/local/opt/openssl/include' CXXFLAGS='-std=c++03 -DS3FS_PTHREAD_ERRORCHECK=1'
         - make
         - make cppcheck
         - make check -C src
@@ -51,7 +51,7 @@ matrix:
         - sudo ln -s /usr/local/opt/coreutils/bin/gstdbuf /usr/local/bin/stdbuf
       script:
         - ./autogen.sh
-        - PKG_CONFIG_PATH=/usr/local/opt/curl/lib/pkgconfig:/usr/local/opt/openssl/lib/pkgconfig ./configure CXXFLAGS='-std=c++03'
+        - PKG_CONFIG_PATH=/usr/local/opt/curl/lib/pkgconfig:/usr/local/opt/openssl/lib/pkgconfig ./configure CXXFLAGS='-std=c++03 -DS3FS_PTHREAD_ERRORCHECK=1'
         - make
         - make cppcheck
         - make check -C src
@@ -72,7 +72,7 @@ matrix:
         - sudo pip install --upgrade awscli
       script:
         - ./autogen.sh
-        - ./configure CPPFLAGS='-I/usr/local/opt/openssl/include' CXXFLAGS='-std=c++03'
+        - ./configure CPPFLAGS='-I/usr/local/opt/openssl/include' CXXFLAGS='-std=c++03 -DS3FS_PTHREAD_ERRORCHECK=1'
         - make
         - make cppcheck
         - make check -C src

src/cache.cpp

@@ -147,7 +147,9 @@ StatCache::StatCache() : IsExpireTime(false), IsExpireIntervalType(false), Expir
     stat_cache.clear();
     pthread_mutexattr_t attr;
     pthread_mutexattr_init(&attr);
-    pthread_mutexattr_settype(&attr, S3FS_MUTEX_RECURSIVE);
+#if S3FS_PTHREAD_ERRORCHECK
+    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
+#endif
     pthread_mutex_init(&StatCache::stat_cache_lock, &attr);
   }else{
     abort();
@@ -297,7 +299,7 @@ bool StatCache::GetStat(const string& key, struct stat* pst, headers_t* meta, bo
   }
 
   if(is_delete_cache){
-    DelStat(strpath);
+    DelStat(strpath, /*lock_already_held=*/ true);
   }
   return false;
 }
@@ -536,14 +538,14 @@ bool StatCache::TruncateCache()
   return true;
 }
 
-bool StatCache::DelStat(const char* key)
+bool StatCache::DelStat(const char* key, bool lock_already_held)
 {
   if(!key){
     return false;
   }
   S3FS_PRN_INFO3("delete stat cache entry[path=%s]", key);
 
-  AutoLock lock(&StatCache::stat_cache_lock);
+  AutoLock lock(&StatCache::stat_cache_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
 
   stat_cache_t::iterator iter;
   if(stat_cache.end() != (iter = stat_cache.find(string(key)))){

src/cache.h

@@ -120,9 +120,9 @@ class StatCache
     void ChangeNoTruncateFlag(const std::string& key, bool no_truncate);
 
     // Delete stat cache
-    bool DelStat(const char* key);
+    bool DelStat(const char* key, bool lock_already_held = false);
-    bool DelStat(std::string& key) {
+    bool DelStat(std::string& key, bool lock_already_held = false) {
-      return DelStat(key.c_str());
+      return DelStat(key.c_str(), lock_already_held);
     }
 };
 

src/fdcache.cpp

@@ -630,7 +630,9 @@ FdEntity::FdEntity(const char* tpath, const char* cpath)
   try{
     pthread_mutexattr_t attr;
     pthread_mutexattr_init(&attr);
-    pthread_mutexattr_settype(&attr, S3FS_MUTEX_RECURSIVE);   // recursive mutex
+#if S3FS_PTHREAD_ERRORCHECK
+    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
+#endif
     pthread_mutex_init(&fdent_lock, &attr);
     pthread_mutex_init(&fdent_data_lock, &attr);
     is_lock_init = true;
@@ -723,12 +725,13 @@ void FdEntity::Close()
   }
 }
 
-int FdEntity::Dup()
+int FdEntity::Dup(bool lock_already_held)
 {
+  AutoLock auto_lock(&fdent_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
+
   S3FS_PRN_DBG("[path=%s][fd=%d][refcnt=%d]", path.c_str(), fd, (-1 != fd ? refcnt + 1 : refcnt));
 
   if(-1 != fd){
-    AutoLock auto_lock(&fdent_lock);
     refcnt++;
   }
   return fd;
@@ -793,9 +796,10 @@ int FdEntity::OpenMirrorFile()
 
 int FdEntity::Open(headers_t* pmeta, off_t size, time_t time, bool no_fd_lock_wait)
 {
+  AutoLock auto_lock(&fdent_lock, no_fd_lock_wait ? AutoLock::NO_WAIT : AutoLock::NONE);
+
   S3FS_PRN_DBG("[path=%s][fd=%d][size=%lld][time=%jd]", path.c_str(), fd, static_cast<long long int>(size), (intmax_t)time);
 
-  AutoLock auto_lock(&fdent_lock, no_fd_lock_wait);
   if (!auto_lock.isLockAcquired()) {
     // had to wait for fd lock, return
     return -EIO;
@@ -804,7 +808,7 @@ int FdEntity::Open(headers_t* pmeta, off_t size, time_t time, bool no_fd_lock_wa
   AutoLock auto_data_lock(&fdent_data_lock);
   if(-1 != fd){
     // already opened, needs to increment refcnt.
-    Dup();
+    Dup(/*lock_already_held=*/ true);
 
     // check only file size(do not need to save cfs and time.
     if(0 <= size && pagelist.Size() != size){
@@ -971,7 +975,7 @@ int FdEntity::Open(headers_t* pmeta, off_t size, time_t time, bool no_fd_lock_wa
 
   // set mtime(set "x-amz-meta-mtime" in orgmeta)
   if(-1 != time){
-    if(0 != SetMtime(time)){
+    if(0 != SetMtime(time, /*lock_already_held=*/ true)){
       S3FS_PRN_ERR("failed to set mtime. errno(%d)", errno);
       fclose(pfile);
       pfile = NULL;
@@ -1007,7 +1011,7 @@ bool FdEntity::OpenAndLoadAll(headers_t* pmeta, off_t* size, bool force_load)
   //
   // TODO: possibly do background for delay loading
   //
-  if(0 != (result = Load())){
+  if(0 != (result = Load(/*start=*/ 0, /*size=*/ 0, /*lock_already_held=*/ true))){
     S3FS_PRN_ERR("could not download, result(%d)", result);
     return false;
   }
@@ -1020,9 +1024,9 @@ bool FdEntity::OpenAndLoadAll(headers_t* pmeta, off_t* size, bool force_load)
   return true;
 }
 
-bool FdEntity::GetStats(struct stat& st)
+bool FdEntity::GetStats(struct stat& st, bool lock_already_held)
 {
-  AutoLock auto_lock(&fdent_lock);
+  AutoLock auto_lock(&fdent_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
   if(-1 == fd){
     return false;
   }
@@ -1045,7 +1049,7 @@ int FdEntity::SetCtime(time_t time)
   return 0;
 }
 
-int FdEntity::SetMtime(time_t time)
+int FdEntity::SetMtime(time_t time, bool lock_already_held)
 {
   S3FS_PRN_INFO3("[path=%s][fd=%d][time=%jd]", path.c_str(), fd, (intmax_t)time);
 
@@ -1053,7 +1057,7 @@ int FdEntity::SetMtime(time_t time)
     return 0;
   }
 
-  AutoLock auto_lock(&fdent_lock);
+  AutoLock auto_lock(&fdent_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
   if(-1 != fd){
     struct timeval tv[2];
     tv[0].tv_sec = time;
@@ -1084,7 +1088,7 @@ bool FdEntity::UpdateCtime()
 {
   AutoLock auto_lock(&fdent_lock);
   struct stat st;
-  if(!GetStats(st)){
+  if(!GetStats(st, /*lock_already_held=*/ true)){
     return false;
   }
   orgmeta["x-amz-meta-ctime"] = str(st.st_ctime);
@@ -1095,7 +1099,7 @@ bool FdEntity::UpdateMtime()
 {
   AutoLock auto_lock(&fdent_lock);
   struct stat st;
-  if(!GetStats(st)){
+  if(!GetStats(st, /*lock_already_held=*/ true)){
     return false;
   }
   orgmeta["x-amz-meta-ctime"] = str(st.st_ctime);
@@ -1171,14 +1175,16 @@ bool FdEntity::SetAllStatus(bool is_loaded)
   return true;
 }
 
-int FdEntity::Load(off_t start, off_t size)
+int FdEntity::Load(off_t start, off_t size, bool lock_already_held)
 {
+  AutoLock auto_lock(&fdent_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
+
   S3FS_PRN_DBG("[path=%s][fd=%d][offset=%lld][size=%lld]", path.c_str(), fd, static_cast<long long int>(start), static_cast<long long int>(size));
 
   if(-1 == fd){
     return -EBADF;
   }
-  AutoLock auto_lock(&fdent_data_lock);
+  AutoLock auto_data_lock(&fdent_data_lock, lock_already_held ? AutoLock::ALREADY_LOCKED : AutoLock::NONE);
 
   int result = 0;
 
@@ -1668,7 +1674,7 @@ ssize_t FdEntity::Read(char* bytes, off_t start, size_t size, bool force_load)
     // Loading
     int result = 0;
     if(0 < size){
-      result = Load(start, load_size);
+      result = Load(start, load_size, /*lock_already_held=*/ true);
     }
 
     FdManager::FreeReservedDiskSpace(load_size);
@@ -1721,7 +1727,7 @@ ssize_t FdEntity::Write(const char* bytes, off_t start, size_t size)
 
       // Load uninitialized area which starts from 0 to (start + size) before writing.
       if(0 < start){
-        result = Load(0, start);
+        result = Load(0, start, /*lock_already_held=*/ true);
       }
       FdManager::FreeReservedDiskSpace(restsize);
       if(0 != result){
@@ -1760,7 +1766,7 @@ ssize_t FdEntity::Write(const char* bytes, off_t start, size_t size)
 
   // Load uninitialized area which starts from (start + size) to EOF after writing.
   if(pagelist.Size() > start + static_cast<off_t>(size)){
-    result = Load(start + size, pagelist.Size());
+    result = Load(start + size, pagelist.Size(), /*lock_already_held=*/ true);
     if(0 != result){
       S3FS_PRN_ERR("failed to load uninitialized area after writing(errno=%d)", result);
       return static_cast<ssize_t>(result);
@@ -1793,7 +1799,7 @@ ssize_t FdEntity::Write(const char* bytes, off_t start, size_t size)
 
 void FdEntity::CleanupCache()
 {
-  AutoLock auto_lock(&fdent_lock, true);
+  AutoLock auto_lock(&fdent_lock, AutoLock::NO_WAIT);
 
   if (!auto_lock.isLockAcquired()) {
     return;
@@ -2282,7 +2288,7 @@ void FdManager::CleanupCacheDir()
     return;
   }
 
-  AutoLock auto_lock_no_wait(&FdManager::cache_cleanup_lock, true);
+  AutoLock auto_lock_no_wait(&FdManager::cache_cleanup_lock, AutoLock::NO_WAIT);
 
   if(auto_lock_no_wait.isLockAcquired()){
     S3FS_PRN_INFO("cache cleanup started");

src/fdcache.h

@@ -148,15 +148,15 @@ class FdEntity
     bool IsMultiOpened(void) const { return refcnt > 1; }
     int Open(headers_t* pmeta = NULL, off_t size = -1, time_t time = -1, bool no_fd_lock_wait = false);
     bool OpenAndLoadAll(headers_t* pmeta = NULL, off_t* size = NULL, bool force_load = false);
-    int Dup();
+    int Dup(bool lock_already_held = false);
 
     const char* GetPath(void) const { return path.c_str(); }
     void SetPath(const std::string &newpath) { path = newpath; }
     int GetFd(void) const { return fd; }
 
-    bool GetStats(struct stat& st);
+    bool GetStats(struct stat& st, bool lock_already_held = false);
     int SetCtime(time_t time);
-    int SetMtime(time_t time);
+    int SetMtime(time_t time, bool lock_already_held = false);
     bool UpdateCtime(void);
     bool UpdateMtime(void);
     bool GetSize(off_t& size);
@@ -165,7 +165,7 @@ class FdEntity
     bool SetGId(gid_t gid);
     bool SetContentType(const char* path);
 
-    int Load(off_t start = 0, off_t size = 0);                 // size=0 means loading to end
+    int Load(off_t start = 0, off_t size = 0, bool lock_already_held = false);  // size=0 means loading to end
     int NoCacheLoadAndPost(off_t start = 0, off_t size = 0);   // size=0 means loading to end
     int NoCachePreMultipartPost(void);
     int NoCacheMultipartPost(int tgfd, off_t start, off_t size);

src/s3fs_util.cpp

@@ -422,12 +422,28 @@ void free_mvnodes(MVNODE *head)
 //-------------------------------------------------------------------
 // Class AutoLock
 //-------------------------------------------------------------------
-AutoLock::AutoLock(pthread_mutex_t* pmutex, bool no_wait) : auto_mutex(pmutex)
+AutoLock::AutoLock(pthread_mutex_t* pmutex, Type type) : auto_mutex(pmutex), type(type)
 {
-  if (no_wait) {
+  if (type == ALREADY_LOCKED) {
-    is_lock_acquired = pthread_mutex_trylock(auto_mutex) == 0;
+    is_lock_acquired = false;
+  } else if (type == NO_WAIT) {
+    int res = pthread_mutex_trylock(auto_mutex);
+    if(res == 0){
+      is_lock_acquired = true;
+    }else if(res == EBUSY){
+      is_lock_acquired = false;
+    }else{
+      S3FS_PRN_CRIT("pthread_mutex_trylock returned: %d", res);
+      abort();
+    }
   } else {
-    is_lock_acquired = pthread_mutex_lock(auto_mutex) == 0;
+    int res = pthread_mutex_lock(auto_mutex);
+    if(res == 0){
+      is_lock_acquired = true;
+    }else{
+      S3FS_PRN_CRIT("pthread_mutex_lock returned: %d", res);
+      abort();
+    }
   }
 }
 

src/s3fs_util.h

@@ -86,14 +86,20 @@ typedef struct mvnode {
 
 class AutoLock
 {
-  private:
-    pthread_mutex_t* auto_mutex;
-    bool is_lock_acquired;
-
   public:
-    explicit AutoLock(pthread_mutex_t* pmutex, bool no_wait = false);
+    enum Type {
+      NO_WAIT = 1,
+      ALREADY_LOCKED = 2,
+      NONE = 0
+    };
+    explicit AutoLock(pthread_mutex_t* pmutex, Type type = NONE);
     bool isLockAcquired() const;
     ~AutoLock();
+
+  private:
+    pthread_mutex_t* const auto_mutex;
+    bool is_lock_acquired;
+    const Type type;
 };
 
 //-------------------------------------------------------------------