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

Jump to bottom

fix_enable #2340

Closed
wants to merge 11 commits into from
Closed

fix_enable #2340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cdec899
fix_enable
bit-dance Mar 28, 2023
893411f
fix
bit-dance Apr 13, 2023
d4df8e2
merge
bit-dance Apr 13, 2023
dc787ae
addFlagInleaseExecutor_V0
bit-dance Apr 13, 2023
9dc3047
wip
bit-dance Apr 15, 2023
5eebed8
addUpdateInFuseclient
bit-dance Apr 16, 2023
4f83f1d
Merge branch
bit-dance Apr 16, 2023
92f7273
Merge branch 'opencurve:master' into fix_enable
bit-dance Apr 16, 2023
7f8d871
Merge branch 'opencurve:master' into fix_enable
bit-dance Apr 20, 2023
245d0b1
Merge branch 'opencurve:master' into fix_enable
bit-dance Apr 26, 2023
4b0f9cf
Merge branch 'opencurve:master' into fix_enable
bit-dance May 13, 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
2 changes: 1 addition & 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,7 @@ CURVEFS_ERROR FuseClient::FuseOpInit(void *userdata,
}
inodeManager_->SetFsId(fsInfo_->fsid());
dentryManager_->SetFsId(fsInfo_->fsid());
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

The given patch looks correct, it is changing the value of enableSumInDir_ to be equal to the "enablesumindir" value from fsInfo_, removing the FLAGS_enableCto condition.

It would be beneficial to add a comment explaining the purpose of this change and any assumptions made when making this change. This will help other developers understand why this change was made and prevent any unexpected behavior.

It is also important to ensure that this change does not break any existing functionality. Thorough testing should be done to ensure that any possible scenarios are tested before pushing this change to production.

Copy link

Choose a reason for hiding this comment

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

with code review:

The patch changes the code from disabling enableSumInDir_ if FLAGS_enableCto is set to not disabling it regardless of the value of FLAGS_enableCto. This change seems reasonable as it would allow enableSumInDir_ to be enabled even if FLAGS_enableCto is set. However, it is important to note that this might have unintended consequences depending on the implementation of FLAGS_enableCto. It is recommended to check the implementation of FLAGS_enableCto and ensure that this change does not have any unexpected side-effects.

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) {
Copy link
Contributor

@SeanHai SeanHai Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

// stats are not cached in xattr in multi-mount scenarios
if (fsInfo_.enablesumindir() && fsInfo_.mountnum() > 1) {
    fsInfo_.set_enablesumindir(false);
}

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.

the code review

  1. The first thing to check is the syntax of the code. Make sure that all the braces, semicolons and parentheses are present in the right places.

  2. Look for any logical errors in the code. For example, if the mountnum is greater than 1, then the enablesumindir should be set to false.

  3. Check if the code is optimized. For example, you can use a ternary operator instead of an if statement.

  4. Check for any security risks. For example, is the data sanitized before being used?

  5. Check if the code follows the coding style guide of your organization.

  6. Test the code to make sure it works as expected.

  7. If the code is part of a larger system, check if it integrates properly with other components.

Copy link

Choose a reason for hiding this comment

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

with understanding the code.
The code is about updating the MountPoint of FsInfoWrapper, which is a class for manipulating mount point information. It contains two methods: AddMountPoint and DeleteMountPoint.

The code patch adds a new line in the AddMountPoint method, which sets the enablesumindir field to false if the mountnum is greater than 1. This is necessary because the mountnum indicates the number of mount points, and if there are more than one mount points, then enablesumindir should be false.

From a bug risk perspective, this patch looks okay. However, it would be beneficial to add a comment that explains the purpose of setting the enablesumindir field to false when mountnum is greater than 1. This will make it easier for other developers to understand the code. Additionally, it might be worth adding a check to make sure that mountnum is not greater than the maximum allowed mount points, before setting enablesumindir to false.

Expand Down