diff --git a/.travis.yml b/.travis.yml index dc05255..edcf294 100644 --- a/.travis.yml +++ b/.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 diff --git a/src/cache.cpp b/src/cache.cpp index eed6954..6c24042 100644 --- a/src/cache.cpp +++ b/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)))){ diff --git a/src/cache.h b/src/cache.h index 8f57ba9..6472685 100644 --- a/src/cache.h +++ b/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(std::string& key) { - return DelStat(key.c_str()); + bool DelStat(const char* key, bool lock_already_held = false); + bool DelStat(std::string& key, bool lock_already_held = false) { + return DelStat(key.c_str(), lock_already_held); } }; diff --git a/src/fdcache.cpp b/src/fdcache.cpp index d57c1d1..3b0009a 100644 --- a/src/fdcache.cpp +++ b/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(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(start), static_cast(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(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(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"); diff --git a/src/fdcache.h b/src/fdcache.h index 0296550..574f0ee 100644 --- a/src/fdcache.h +++ b/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); diff --git a/src/s3fs_util.cpp b/src/s3fs_util.cpp index 52f5bf6..e676a0d 100644 --- a/src/s3fs_util.cpp +++ b/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) { - is_lock_acquired = pthread_mutex_trylock(auto_mutex) == 0; + if (type == ALREADY_LOCKED) { + 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(); + } } } diff --git a/src/s3fs_util.h b/src/s3fs_util.h index a70f000..b5ad12e 100644 --- a/src/s3fs_util.h +++ b/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; }; //-------------------------------------------------------------------