Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enableSumIndir problem #2339

Closed
wants to merge 3 commits into from
Closed

Fix enableSumIndir problem #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22a8819
fix_enableSumInDir
bit-dance Mar 19, 2023
4d7e4ce
Merge branch 'opencurve:master' into fix_eanbleSumInDir_2
bit-dance Mar 26, 2023
f50e244
fix_cpplint_error
bit-dance Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion curvefs/src/client/fuse_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
3 changes: 3 additions & 0 deletions curvefs/src/mds/fs_info_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review:

  1. 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.
  2. 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.
  3. 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.

Expand Down