fix_enable #2340

bit-dance · 2023-03-28T08:46:25Z

What problem does this PR solve?

In multi mount situation,the xattr information will wrong and we can not use the sum to calculate the file bytes.
Issue Number: #2094

Problem Summary:
When the fs mount is mounted multiple times with different enableSumInDir switches, the xattr information recorded in the metadata will be inaccurate.

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

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

cr-gpt · 2023-03-28T08:46:35Z

curvefs/src/client/fuse_client.cpp

@@ -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);
    }


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.

cr-gpt · 2023-03-28T08:46:41Z

curvefs/src/mds/fs_info_wrapper.cpp

@@ -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) {


the code review

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.

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.

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

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

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

Test the code to make sure it works as expected.

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

SeanHai · 2023-03-28T09:05:30Z

cicheck

zhanghuidinah · 2023-03-30T02:07:25Z

cicheck

fix cpplint error Signed-off-by: jbk <[email protected]>

cr-gpt · 2023-03-30T10:29:54Z

curvefs/src/client/fuse_client.cpp

@@ -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);
    }


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.

cr-gpt · 2023-03-30T10:30:00Z

curvefs/src/mds/fs_info_wrapper.cpp

@@ -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) {


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.

bit-dance · 2023-03-30T10:32:05Z

cicheck

zhanghuidinah · 2023-03-31T01:56:19Z

cicheck

caoxianfei1 · 2023-04-03T02:26:33Z

curve_fuse is not found @bit-dance

bit-dance · 2023-04-03T14:19:20Z

Ok,i will research this part later

SeanHai · 2023-04-06T09:08:04Z

cicheck

SeanHai · 2023-04-07T02:59:51Z

curvefs/src/mds/fs_info_wrapper.cpp

@@ -113,6 +113,9 @@ void FsInfoWrapper::AddMountPoint(const Mountpoint& mp) {
    *p = mp;

    fsInfo_.set_mountnum(fsInfo_.mountnum() + 1);
+    if (fsInfo_.mountnum() > 1) {


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

SeanHai · 2023-04-07T06:49:03Z

@bit-dance
The current implementation is incomplete. In the multi-mount scenario, the flag in fsInfo is set to false, and subsequent mount points perceive this change (enableSumInDir_ in the client will be a new value), but the mounted client The enableSumInDir_ in is still old, so it is necessary to reversely update the change of this value from mds. At present, the connection from clinet to mds is realized by session. You can consider taking back the flag from mds in the renewal response and updating it to the client.

Cyber-SiKu · 2023-04-24T01:29:17Z

@bit-dance Will you continue to follow up this PR?

bit-dance · 2023-04-24T06:56:50Z

Yes,these times i am dealing with my exams and later i will continue following codes.

caoxianfei1 · 2023-05-04T01:48:32Z

@bit-dance feel free to continue.

bit-dance · 2023-05-04T23:35:35Z

@caoxianfei1 I am so sorry but i will have time to continue next week.

cr-gpt bot reviewed Mar 28, 2023

View reviewed changes

fix_enable

cdec899

fix cpplint error Signed-off-by: jbk <[email protected]>

bit-dance force-pushed the fix_enable branch from 7a2db1b to cdec899 Compare March 30, 2023 10:29

cr-gpt bot reviewed Mar 30, 2023

View reviewed changes

ilixiaocui requested a review from SeanHai April 7, 2023 02:48

ilixiaocui mentioned this pull request Apr 7, 2023

[2023 Q2 Developer Activities]: Call For Participation！ #2334

Closed

SeanHai reviewed Apr 7, 2023

View reviewed changes

bit-dance added 8 commits April 13, 2023 10:58

fix

893411f

merge

d4df8e2

addFlagInleaseExecutor_V0

dc787ae

wip

9dc3047

addUpdateInFuseclient

5eebed8

Merge branch

4f83f1d

Merge branch 'opencurve:master' into fix_enable

92f7273

Merge branch 'opencurve:master' into fix_enable

7f8d871

Merge branch 'opencurve:master' into fix_enable

245d0b1

Merge branch 'opencurve:master' into fix_enable

4b0f9cf

zhanghuidinah added the Developer Activities label May 16, 2023

opencurveadmin requested review from SeanHai and caoxianfei1 May 19, 2023 07:06

bit-dance closed this May 19, 2023

bit-dance deleted the fix_enable branch May 30, 2023 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix_enable #2340

fix_enable #2340

bit-dance commented Mar 28, 2023

cr-gpt bot Mar 28, 2023

cr-gpt bot Mar 28, 2023

SeanHai commented Mar 28, 2023

zhanghuidinah commented Mar 30, 2023

cr-gpt bot Mar 30, 2023

cr-gpt bot Mar 30, 2023

bit-dance commented Mar 30, 2023

zhanghuidinah commented Mar 31, 2023

caoxianfei1 commented Apr 3, 2023

bit-dance commented Apr 3, 2023

SeanHai commented Apr 6, 2023

SeanHai Apr 7, 2023 •

edited

Loading

SeanHai commented Apr 7, 2023

Cyber-SiKu commented Apr 24, 2023

bit-dance commented Apr 24, 2023

caoxianfei1 commented May 4, 2023

bit-dance commented May 4, 2023

fix_enable #2340

fix_enable #2340

Conversation

bit-dance commented Mar 28, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

cr-gpt bot Mar 28, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 28, 2023

Choose a reason for hiding this comment

SeanHai commented Mar 28, 2023

zhanghuidinah commented Mar 30, 2023

cr-gpt bot Mar 30, 2023

Choose a reason for hiding this comment

cr-gpt bot Mar 30, 2023

Choose a reason for hiding this comment

bit-dance commented Mar 30, 2023

zhanghuidinah commented Mar 31, 2023

caoxianfei1 commented Apr 3, 2023

bit-dance commented Apr 3, 2023

SeanHai commented Apr 6, 2023

SeanHai Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

SeanHai commented Apr 7, 2023

Cyber-SiKu commented Apr 24, 2023

bit-dance commented Apr 24, 2023

caoxianfei1 commented May 4, 2023

bit-dance commented May 4, 2023

SeanHai Apr 7, 2023 •

edited

Loading