Fix enableSumIndir problem #2339
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| @@ -198,7 +198,8 @@ CURVEFS_ERROR FuseClient::FuseOpInit(void *userdata, | |||
| } | |||
| inodeManager_->SetFsId(fsInfo_->fsid()); | |||
| dentryManager_->SetFsId(fsInfo_->fsid()); | |||
| enableSumInDir_ = fsInfo_->enablesumindir() && !FLAGS_enableCto; | |||
| // enableSumInDir_ = fsInfo_->enablesumindir() && !FLAGS_enableCto; | |||
| enableSumInDir_ = fsInfo_->enablesumindir(); | |||
| if (fsInfo_->has_recycletimehour()) { | |||
| enableSumInDir_ = enableSumInDir_ && (fsInfo_->recycletimehour() == 0); | |||
| } | |||
There was a problem hiding this comment.
the code review,
First of all, it looks like the code patch is trying to resolve a specific bug or issue. From the context, it appears that the patch is attempting to set the variable enableSumInDir_, which is used to enable summaries in a directory.
The patch removes the existing condition of !FLAGS_enableCto, which means that the variable will now be set regardless of the value of FLAGS_enableCto. While this may be necessary in order to resolve the issue, it would be helpful to better understand why the FLAGS_enableCto condition was removed and if there are any potential risks to removing it. Additionally, it might be helpful to add a comment explaining why the condition was removed in order to make the code more understandable.
Overall, the patch looks reasonable and should help to resolve the issue. However, it would be helpful to get a better understanding of why the FLAGS_enableCto condition was removed in order to ensure that there are no unintended consequences.
| @@ -113,6 +113,9 @@ void FsInfoWrapper::AddMountPoint(const Mountpoint& mp) { | |||
| *p = mp; | |||
|
|
|||
| fsInfo_.set_mountnum(fsInfo_.mountnum() + 1); | |||
| if(fsInfo_.mountnum()>1){ | |||
| fsInfo_.set_enablesumindir(false); | |||
| } | |||
| } | |||
|
|
|||
| FSStatusCode FsInfoWrapper::DeleteMountPoint(const Mountpoint& mp) { | |||
There was a problem hiding this comment.
with the code review:
- The code adds a new statement to check if the mountnum of the fsInfo is greater than 1, if it is it sets enablesumindir to false. This seems to make sense as it would prevent summing up files in multiple directories.
- The code looks correct and should work as intended. However, there is no error handling for if mountnum is not greater than 1. It would be better to add a check to make sure that mountnum is greater than 1 before setting enablesumindir to false.
- Additionally, it would be helpful to add some comments to explain the logic of the code to make it easier for other engineers to understand.
bit-dance
force-pushed
the
fix_eanbleSumInDir_2
branch
from
0162f7c to
f50e244
Compare
| @@ -198,7 +198,8 @@ CURVEFS_ERROR FuseClient::FuseOpInit(void *userdata, | |||
| } | |||
| inodeManager_->SetFsId(fsInfo_->fsid()); | |||
| dentryManager_->SetFsId(fsInfo_->fsid()); | |||
| enableSumInDir_ = fsInfo_->enablesumindir() && !FLAGS_enableCto; | |||
| // enableSumInDir_ = fsInfo_->enablesumindir() && !FLAGS_enableCto; | |||
| enableSumInDir_ = fsInfo_->enablesumindir(); | |||
| if (fsInfo_->has_recycletimehour()) { | |||
| enableSumInDir_ = enableSumInDir_ && (fsInfo_->recycletimehour() == 0); | |||
| } | |||
There was a problem hiding this comment.
the code review.
The code patch looks okay on the surface. It removes the FLAGS_enableCto check, which might have been used to disable the enableSumInDir_ flag. However, it is important to determine why this change is being implemented and if the removal of this check can have any adverse effects on the application.
Also, the code should be tested thoroughly to ensure that this change does not introduce any bugs or security vulnerabilities. Additionally, it is important to consider if there are any potential performance improvements that can be made.
What problem does this PR solve?
When the fs mount is mounted multiple times with different enableSumInDir switches, the xattr information recorded in the metadata will be inaccurate
Issue Number: #2094
Problem Summary:
What is changed and how it works?
If mds find there are multi mounts and then it will close the enableSumInDir forever.
What's Changed:
fuse_client.cpp fs_info_wrapper.cpp
How it Works:
just like above
Side effects(Breaking backward compatibility? Performance regression?):
maybe we can't use the sum xattr later if we close the enableSumInDir.
Check List