Skip to content

Commit

Permalink
Fixed a bug in fdatasync(fsync)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ggtakec @gaul
ggtakec authored and gaul committed Feb 6, 2024
1 parent 5e6f21a commit 517574c
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 22 deletions.
43 changes: 39 additions & 4 deletions src/fdcache_entity.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1523,6 +1534,7 @@ int FdEntity::RowFlushNoMultipart(const PseudoFdInfo* pseudo_obj, const char* tp
if(0 == result){
pagelist.ClearAllModified();
}

return result;
}

Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/fdcache_entity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 11 additions & 1 deletion src/s3fs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -3060,6 +3060,16 @@ static int s3fs_fsync(const char* _path, int datasync, struct fuse_file_info* fi
}
result = ent->Flush(static_cast<int>(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;
Expand Down
19 changes: 2 additions & 17 deletions test/integration-test-main.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
}
Expand Down Expand Up @@ -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}")
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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}")
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand All @@ -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}"

Expand Down

0 comments on commit 517574c

Please sign in to comment.