curvefs/client: add qos for curve-fuse, we can limits the bandwidth #2424

UniverseParticle · 2023-04-24T14:16:19Z

What problem does this PR solve?

Issue Number: #2225

Problem Summary: add qos for curve-fuse, then we can limits the bandwidth or IOPS

What is changed and how it works?

What's Changed:

Read the configuration from the client.conf
enable the throttle

Side effects(Breaking backward compatibility? Performance regression?):
It does not start by default

Check List

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

UniverseParticle · 2023-04-24T15:05:09Z

cicheck

wuhongsong · 2023-04-25T10:43:53Z

cicheck

wuhongsong · 2023-04-25T13:16:36Z

@UniverseParticle To avoid having PRs blocked in the future, always include Signed-off-by: Author Name <authoremail@example.com> in every commit message. You can also do this automatically by using the -s flag (i.e., git commit -s).

wuhongsong · 2023-04-26T08:16:48Z

good job(and maybe we can also limit the metadata manipulation, but we can do it in ohter pr after this done)

wuhongsong · 2023-04-26T08:20:45Z

curvefs/src/client/fuse_client.cpp

@@ -143,6 +142,15 @@ CURVEFS_ERROR FuseClient::Init(const FuseClientOption &option) {
        warmupManager_->SetFsInfo(fsInfo_);
    }

+    ReadWriteThrottleParams params;


maybe we can dynamic throttle like diskcache throttle

OK, It's better this way

UniverseParticle · 2023-04-28T11:55:38Z

cicheck

UniverseParticle · 2023-04-28T12:15:58Z

cicheck

ilixiaocui · 2023-05-04T06:25:24Z

It is better to merge four commits into one commit.

ilixiaocui · 2023-05-04T06:26:15Z

Please use git commit -s -m "****"

wuhongsong · 2023-05-04T06:27:09Z

good job. LGTM. no any more questions, except git commit -s as said above.

UniverseParticle · 2023-05-05T05:05:31Z

cicheck

UniverseParticle · 2023-05-06T05:32:45Z

cicheck

UniverseParticle · 2023-05-06T16:04:13Z

cicheck

wuhongsong · 2023-05-07T06:22:32Z

client branch coverage is not ok, actual branch coverage ratio: 42.2714, expected branch coverage ratio: 59. maybe the unitest is abort, you can run unittest in your test environment and look at the reasons for the failure

wuhongsong · 2023-05-09T02:06:42Z

just miss package. run "make stor=fs dep"

…

---- Replied Message ---- | From | ***@***.***> | | Date | 05/08/2023 21:48 | | To | ***@***.***> | | Cc | ***@***.***>、Review ***@***.***> | | Subject | Re: [opencurve/curve] curvefs/client: add qos for curve-fuse, we can limits the bandwidth (PR #2424) | Unit test compilation failed — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: ***@***.***>

aspirer · 2023-05-11T07:37:47Z

curvefs/conf/client.conf

+# the read throttle bps of fuseClient, default no limit
+fuseClient.throttle.avgReadBytes=0
+# the read throttle iops of fuseClient, default no limit
+fuseClient.throttle.avgReadIops=0


burst should contains four types:

read

bps

iops

write

bps : just this one here, we'd better add anothers.

iops

I means we can fix this by a new pr.

aspirer · 2023-05-11T07:42:24Z

curvefs/src/client/common/config.cpp

-void S3Info2FsS3Option(const curvefs::common::S3Info& s3,
-                       S3InfoOption* fsS3Opt) {
+void S3Info2FsS3Option(const curvefs::common::S3Info &s3,
+                       S3InfoOption *fsS3Opt) {


these changes look like only about code styles, the old and new styles in C++ are all good, we can leave them to old style, so the reviewer can pay more attentions to your added codes.

UniverseParticle · 2023-05-13T13:50:37Z

cicheck

UniverseParticle · 2023-05-14T02:33:15Z

wuhongsong · 2023-05-16T01:56:24Z

cicheck

wuhongsong · 2023-05-16T01:57:02Z

just wget failed, recheck

UniverseParticle · 2023-05-16T11:46:12Z

cicheck

wuhongsong · 2023-05-16T13:00:19Z

cicheck

wuhongsong · 2023-05-17T01:48:02Z

curvefs/src/client/fuse_client.cpp

+
+    throttle_.UpdateThrottleParams(params);
+
+    int ret = bthread_timer_add(&throttleTimer_, butil::seconds_from_now(1),


not periodic tasks? maybe can reference timer in curve

This is a timer task that refreshes variables once per second

This is a timer task that refreshes variables once per second
yes

ilixiaocui · 2023-05-17T11:02:40Z

curvefs/src/client/common/config.h

@@ -205,6 +205,34 @@ struct FuseClientOption {
    bool disableXattr = false;
    uint32_t downloadMaxRetryTimes;
    uint32_t warmupThreadsNum = 10;
+
+    // the write throttle bps of fuseClient
+    uint64_t avgWriteBytes;


ilixiaocui · 2023-05-17T11:05:58Z

curvefs/src/client/common/config.cpp

@@ -308,6 +308,35 @@ void InitFuseClientOption(Configuration *conf, FuseClientOption *clientOption) {
        clientOption->entryTimeOut = 0;
    }

+
+    conf->GetValueFatalIfFail("fuseClient.throttle.avgWriteBytes",
+                              &clientOption->avgWriteBytes);


Here you can consider initializing FLAGS_XX directly, without going through the option to switch to one layer.

I referred to the qos usage of S3 disk cache。

There is a situation where users only need to initialize from the configuration file and do not need to dynamically modify the qos

If so, the following regular InitQosParam will be problematic.

Even if the user does not set the value of FLAG_XX, the value read from the configuration file will be overwritten.

There is a situation where users only need to initialize from the configuration file and do not need to dynamically modify the qos

It can be considered that FLAG_XX is directly read in the code, and there are two sources of its assignment, one is the configuration file, and the other is the setting of the http interface.

The fuse client initialization will read from the configuration file and assign values to FLAG_XX.

The fuse client initialization will read from the configuration file and assign values to FLAG_XX.

so，here you can consider initializing FLAGS_XX directly, without going through the option to switch to one layer? like:

conf->GetValueFatalIfFail("fuseClient.throttle.avgReadBytes", &FLAG_fuseClientAvgReadBytes);

It can be considered that FLAG_XX is directly read in the code, and there are two sources of its assignment, one is the configuration file, and the other is the setting of the http interface.

There are two ways to initializing values . Is this the standard writing method in the future? （initializing FLAGS_XX directly)

ilixiaocui · 2023-05-17T11:07:33Z

curvefs/src/client/fuse_client.cpp

@@ -1471,5 +1548,32 @@ FuseClient::SetMountStatus(const struct MountOption *mountOption) {
    return CURVEFS_ERROR::OK;
 }

+void FuseClient::InitQosParam() {


Add new code, please add the corresponding unit test.

ilixiaocui · 2023-05-17T11:08:49Z

Please merge into one commit to submit.

UniverseParticle · 2023-05-24T09:39:25Z

cicheck

Signed-off-by: UniverseParticle <daidonghuachn@163.com>

UniverseParticle · 2023-05-24T11:36:05Z

cicheck

UniverseParticle · 2023-05-24T11:37:00Z

cicheck

ilixiaocui · 2023-05-25T05:59:31Z

LGTM!

UniverseParticle changed the title ~~feat: add qos for curve-fuse, then we can limits the bandwidth~~ feat: add qos for curve-fuse, we can limits the bandwidth Apr 24, 2023

zhanghuidinah requested review from wuhongsong and ilixiaocui April 25, 2023 02:30

wuhongsong reviewed Apr 26, 2023

View reviewed changes

wuhongsong changed the title ~~feat: add qos for curve-fuse, we can limits the bandwidth~~ curvefs/client: add qos for curve-fuse, we can limits the bandwidth May 4, 2023

aspirer reviewed May 11, 2023

View reviewed changes

UniverseParticle closed this May 13, 2023

UniverseParticle reopened this May 13, 2023

UniverseParticle requested review from wuhongsong May 15, 2023 02:01

zhanghuidinah added the Developer Activities label May 16, 2023

wuhongsong mentioned this pull request May 16, 2023

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

Closed

wuhongsong reviewed May 17, 2023

View reviewed changes

UniverseParticle requested a review from aspirer May 17, 2023 08:52

wuhongsong approved these changes May 17, 2023

View reviewed changes

ilixiaocui reviewed May 17, 2023

View reviewed changes

UniverseParticle requested review from ilixiaocui and wuhongsong May 24, 2023 11:21

add qos for curve-fuse, then we can limits the bandwidth and iops

057f42e

Signed-off-by: UniverseParticle <daidonghuachn@163.com>

ilixiaocui approved these changes May 25, 2023

View reviewed changes

ilixiaocui merged commit 4bb0b52 into opencurve:master May 25, 2023
3 checks passed


		throttle_.UpdateThrottleParams(params);

		int ret = bthread_timer_add(&throttleTimer_, butil::seconds_from_now(1),

curvefs/client: add qos for curve-fuse, we can limits the bandwidth #2424

curvefs/client: add qos for curve-fuse, we can limits the bandwidth #2424

Conversation

UniverseParticle commented Apr 24, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

UniverseParticle commented Apr 24, 2023

wuhongsong commented Apr 25, 2023

wuhongsong commented Apr 25, 2023 • edited

wuhongsong commented Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniverseParticle commented Apr 28, 2023

UniverseParticle commented Apr 28, 2023

ilixiaocui commented May 4, 2023

ilixiaocui commented May 4, 2023

wuhongsong commented May 4, 2023

UniverseParticle commented May 5, 2023

UniverseParticle commented May 6, 2023

UniverseParticle commented May 6, 2023

wuhongsong commented May 7, 2023

wuhongsong commented May 9, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniverseParticle commented May 13, 2023

UniverseParticle commented May 14, 2023

wuhongsong commented May 16, 2023

wuhongsong commented May 16, 2023

UniverseParticle commented May 16, 2023

wuhongsong commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilixiaocui May 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilixiaocui commented May 17, 2023

UniverseParticle commented May 24, 2023

UniverseParticle commented May 24, 2023

UniverseParticle commented May 24, 2023

ilixiaocui commented May 25, 2023

UniverseParticle commented Apr 24, 2023 •

edited

wuhongsong commented Apr 25, 2023 •

edited

wuhongsong commented Apr 26, 2023 •

edited

ilixiaocui May 17, 2023 •

edited