From 517574c40ce1f6f8819b3dd35f08387f7fe8eb6f Mon Sep 17 00:00:00 2001 From: Takeshi Nakatani Date: Wed, 24 Jan 2024 14:11:59 +0000 Subject: [PATCH] Fixed a bug in fdatasync(fsync) --- src/fdcache_entity.cpp | 43 +++++++++++++++++++++++++++++++---- src/fdcache_entity.h | 3 +++ src/s3fs.cpp | 12 +++++++++- test/integration-test-main.sh | 19 ++-------------- 4 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/fdcache_entity.cpp b/src/fdcache_entity.cpp index ee73d2d348..49d0ea34e5 100644 --- a/src/fdcache_entity.cpp +++ b/src/fdcache_entity.cpp @@ -907,6 +907,13 @@ bool FdEntity::UpdateMtime(bool clear_holding_mtime) if(!ClearHoldingMtime(AutoLock::ALREADY_LOCKED)){ return false; } + // [NOTE] + // If come here after fdatasync has been processed, the file + // content update has already taken place. However, the metadata + // update is necessary and needs to be flagged in order to + // perform it with flush, + // + pending_status = pending_status_t::UPDATE_META_PENDING; } }else{ struct stat st; @@ -1426,19 +1433,23 @@ int FdEntity::RowFlush(int fd, const char* tpath, AutoLock::Type type, bool forc AutoLock auto_lock2(&fdent_data_lock); - if(!force_sync && !pagelist.IsModified()){ + int result; + if(!force_sync && !pagelist.IsModified() && !IsDirtyMetadata()){ // nothing to update. return 0; } - if(S3fsLog::IsS3fsLogDbg()){ pagelist.Dump(); } - int result; if(nomultipart){ // No multipart upload - result = RowFlushNoMultipart(pseudo_obj, tpath); + if(!force_sync && !pagelist.IsModified()){ + // for only push pending headers + result = UploadPending(-1, AutoLock::ALREADY_LOCKED); + }else{ + result = RowFlushNoMultipart(pseudo_obj, tpath); + } }else if(FdEntity::streamupload){ // Stream multipart upload result = RowFlushStreamMultipart(pseudo_obj, tpath); @@ -1523,6 +1534,7 @@ int FdEntity::RowFlushNoMultipart(const PseudoFdInfo* pseudo_obj, const char* tp if(0 == result){ pagelist.ClearAllModified(); } + return result; } @@ -2573,6 +2585,29 @@ bool FdEntity::IsDirtyNewFile() const return (pending_status_t::CREATE_FILE_PENDING == pending_status); } +// [NOTE] +// The fdatasync call only uploads the content but does not update +// the meta data. In the flush call, if there is no update contents, +// need to upload only metadata, so use these functions. +// +void FdEntity::MarkDirtyMetadata() +{ + AutoLock auto_lock(&fdent_lock); + AutoLock auto_lock2(&fdent_data_lock); + + if(pending_status_t::NO_UPDATE_PENDING == pending_status){ + pending_status = pending_status_t::UPDATE_META_PENDING; + } +} + +bool FdEntity::IsDirtyMetadata() const +{ + // [NOTE] + // fdent_lock must be previously locked. + // + return (pending_status_t::UPDATE_META_PENDING == pending_status); +} + bool FdEntity::AddUntreated(off_t start, off_t size) { bool result = untreated_list.AddPart(start, size); diff --git a/src/fdcache_entity.h b/src/fdcache_entity.h index 5c34a3a723..9ed91f782e 100644 --- a/src/fdcache_entity.h +++ b/src/fdcache_entity.h @@ -97,6 +97,8 @@ class FdEntity bool AddUntreated(off_t start, off_t size); + bool IsDirtyMetadata() const; + public: static bool GetNoMixMultipart() { return mixmultipart; } static bool SetNoMixMultipart(); @@ -156,6 +158,7 @@ class FdEntity void MarkDirtyNewFile(); bool IsDirtyNewFile() const; + void MarkDirtyMetadata(); bool GetLastUpdateUntreatedPart(off_t& start, off_t& size) const; bool ReplaceLastUpdateUntreatedPart(off_t front_start, off_t front_size, off_t behind_start, off_t behind_size); diff --git a/src/s3fs.cpp b/src/s3fs.cpp index c746f6f0b8..aefd110b5b 100644 --- a/src/s3fs.cpp +++ b/src/s3fs.cpp @@ -3047,7 +3047,7 @@ static int s3fs_fsync(const char* _path, int datasync, struct fuse_file_info* fi WTF8_ENCODE(path) int result = 0; - FUSE_CTX_INFO("[path=%s][pseudo_fd=%llu]", path, (unsigned long long)(fi->fh)); + FUSE_CTX_INFO("[path=%s][datasync=%d][pseudo_fd=%llu]", path, datasync, (unsigned long long)(fi->fh)); AutoFdEntity autoent; FdEntity* ent; @@ -3060,6 +3060,16 @@ static int s3fs_fsync(const char* _path, int datasync, struct fuse_file_info* fi } result = ent->Flush(static_cast(fi->fh), AutoLock::NONE, false); + if(0 != datasync){ + // [NOTE] + // The metadata are not updated when fdatasync is called. + // Instead of it, these metadata are pended and set the dirty flag here. + // Setting this flag allows metadata to be updated even if there is no + // content update between the fdatasync call and the flush call. + // + ent->MarkDirtyMetadata(); + } + if(is_new_file){ // update parent directory timestamp int update_result; diff --git a/test/integration-test-main.sh b/test/integration-test-main.sh index 92a05d57c4..cfbdbe2971 100755 --- a/test/integration-test-main.sh +++ b/test/integration-test-main.sh @@ -408,8 +408,6 @@ function test_external_modification { local OBJECT_NAME; OBJECT_NAME=$(basename "${PWD}")/"${TEST_TEXT_FILE}" echo "new new" | aws_cli s3 cp - "s3://${TEST_BUCKET_1}/${OBJECT_NAME}" - wait_ostype 1 "Darwin" - cmp "${TEST_TEXT_FILE}" <(echo "new new") rm -f "${TEST_TEXT_FILE}" } @@ -884,7 +882,6 @@ function test_mtime_file { #copy the test file with preserve mode cp -p "${TEST_TEXT_FILE}" "${ALT_TEST_TEXT_FILE}" - wait_ostype 1 "Darwin" local testmtime; testmtime=$(get_mtime "${TEST_TEXT_FILE}") local testctime; testctime=$(get_ctime "${TEST_TEXT_FILE}") @@ -953,7 +950,6 @@ function test_update_time_chown() { local base_atime; base_atime=$(get_atime "${TEST_TEXT_FILE}") local base_ctime; base_ctime=$(get_ctime "${TEST_TEXT_FILE}") local base_mtime; base_mtime=$(get_mtime "${TEST_TEXT_FILE}") - wait_ostype 1 "Darwin" # [NOTE] # In this test, chown is called with the same UID. @@ -1050,7 +1046,6 @@ function test_update_time_touch_a() { # "touch -a" -> update ctime/atime, not update mtime # touch -a "${TEST_TEXT_FILE}" - wait_ostype 1 "Darwin" local atime; atime=$(get_atime "${TEST_TEXT_FILE}") local ctime; ctime=$(get_ctime "${TEST_TEXT_FILE}") @@ -1119,6 +1114,8 @@ function test_update_time_cp_p() { echo "cp with -p option expected updated ctime: $base_ctime != $ctime and same mtime: $base_mtime == $mtime, atime: $base_atime == $atime" return 1 fi + rm_test_file + rm_test_file "${TIME_TEST_TEXT_FILE}" } function test_update_time_mv() { @@ -1425,7 +1422,6 @@ function test_update_parent_directory_time_sub() { local base_atime; base_atime=$(get_atime "${TEST_PARENTDIR_PARENT}") local base_ctime; base_ctime=$(get_ctime "${TEST_PARENTDIR_PARENT}") local base_mtime; base_mtime=$(get_mtime "${TEST_PARENTDIR_PARENT}") - wait_ostype 1 "Darwin" touch "${TEST_PARENTDIR_FILE}" @@ -1444,7 +1440,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" touch "${TEST_PARENTDIR_FILE}" @@ -1463,7 +1458,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" mv "${TEST_PARENTDIR_FILE}" "${TEST_PARENTDIR_FILE_MV}" @@ -1482,7 +1476,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" ln -s "${TEST_PARENTDIR_SYMFILE_BASE}" "${TEST_PARENTDIR_SYMFILE}" @@ -1501,7 +1494,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" touch "${TEST_PARENTDIR_SYMFILE}" @@ -1520,7 +1512,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" mv "${TEST_PARENTDIR_SYMFILE}" "${TEST_PARENTDIR_SYMFILE_MV}" @@ -1539,7 +1530,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" rm "${TEST_PARENTDIR_SYMFILE_MV}" @@ -1558,7 +1548,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" rm "${TEST_PARENTDIR_FILE_MV}" @@ -1577,7 +1566,6 @@ function test_update_parent_directory_time_sub() { local base_atime; base_atime=$(get_atime "${TEST_PARENTDIR_PARENT}") local base_ctime; base_ctime=$(get_ctime "${TEST_PARENTDIR_PARENT}") local base_mtime; base_mtime=$(get_mtime "${TEST_PARENTDIR_PARENT}") - wait_ostype 1 "Darwin" mkdir "${TEST_PARENTDIR_DIR}" @@ -1596,7 +1584,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" touch "${TEST_PARENTDIR_DIR}" @@ -1615,7 +1602,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" mv "${TEST_PARENTDIR_DIR}" "${TEST_PARENTDIR_DIR_MV}" @@ -1634,7 +1620,6 @@ function test_update_parent_directory_time_sub() { base_atime="${after_atime}" base_ctime="${after_ctime}" base_mtime="${after_mtime}" - wait_ostype 1 "Darwin" rm -r "${TEST_PARENTDIR_DIR_MV}"