curvefs client : update inode async #1020

xu-chaojie · 2022-01-19T07:53:12Z

What problem does this PR solve?

Issue Number: close #935

Problem Summary:

What is changed and how it works?

Update inode asynchronously， so that update inode will not block each others.

Check List

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

wu-hanqing · 2022-01-21T06:49:53Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+        LOG(WARNING) << "UpdateInode Failed, errorcode = "
+                     << cntl.ErrorCode()
+                     << ", error content:" << cntl.ErrorText()
+                     << ", log id = " << cntl.log_id();


separator should be :

wu-hanqing · 2022-01-21T07:21:19Z

curvefs/src/client/rpcclient/task_excutor.cpp

+    if (!HasValidTarget() && !GetTarget()) {
+        LOG(WARNING) << "get target fail for " << task_->TaskContextStr()
+                     << ", sleep and retry";
+        bthread_usleep(opt_.retryIntervalUS);


what's the purpose of sleep in here? I think is conflicting with sleep in line 210

wu-hanqing · 2022-01-21T07:21:27Z

curvefs/src/client/rpcclient/task_excutor.cpp

+    if (!channel) {
+        LOG(WARNING) << "GetOrCreateChannel fail for "
+                     << task_->TaskContextStr() << ", sleep and retry";
+        bthread_usleep(opt_.retryIntervalUS);


wu-hanqing · 2022-01-21T07:23:53Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+        MetaServerClientRpcDoneBase(done, metaserverClientMetric) {}
+    virtual ~UpdateInodeRpcDone() {}
+    void Run() override;
+    UpdateInodeResponse response;


public data member?

wu-hanqing · 2022-01-21T07:25:46Z

curvefs/src/client/rpcclient/task_excutor.h


 namespace curvefs {
 namespace client {
 namespace rpcclient {

+class TaskExecutorDone;
+
+MetaStatusCode ConvertToMetaStatusCode(int retcode);


it seems this function is only used in .cpp file, so no need to declare it in the header file.

it was used by two different cpp files, so it can not be put in a cpp file.

wu-hanqing · 2022-01-21T07:39:58Z

curvefs/src/client/rpcclient/task_excutor.cpp

@@ -72,6 +79,41 @@ int TaskExecutor::DoRPCTask(std::shared_ptr<TaskContext> task) {
    return retCode;
 }

+void TaskExecutor::DoAsyncRPCTask(TaskExecutorDone *done) {


above synchronous processing can be implemented by this asynchronous, so the main logic here doesn't have to repeat twice.

yeap. sync and async can be solved in same DoRPCTask distinguished by whether done is null.

no, the synchronous way will retry in this function, but asynchronous way retry by closure return, they not same

wu-hanqing · 2022-01-21T07:44:05Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+
+        curvefs::metaserver::MetaServerService_Stub stub(channel);
+        stub.UpdateInode(cntl, &request, &rpcDone->response, rpcDone);
+        return MetaStatusCode::OK;


return value is redundant

if remove this return value， the async task and sync task can not be assign to the same member of task context

wu-hanqing · 2022-01-21T07:44:17Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+        curvefs::metaserver::MetaServerService_Stub stub(channel);
+        stub.GetOrModifyS3ChunkInfo(
+            cntl, &request, &rpcDone->response, rpcDone);
+        return MetaStatusCode::OK;


wu-hanqing · 2022-01-21T07:54:25Z

curvefs/src/client/inode_wrapper.cpp

+        metaClient_->GetOrModifyS3ChunkInfoAsync(
+            inode_.fsid(), inode_.inodeid(), s3ChunkInfoAdd_,
+            done);
+        s3ChunkInfoAdd_.clear();


why not move it when calling GetOrModifyS3ChunkInfoAsync?

s3ChunkInfoAdd_ should clear here， so that new s3chunkinfo can add to it when new write come

ilixiaocui · 2022-01-21T11:37:23Z

curvefs/src/client/inode_wrapper.cpp

+        LockSyncingInode();
+        auto *done = new UpdateInodeAsyncDone(shared_from_this());
+        metaClient_->UpdateInodeAsync(inode_, done);
+        dirty_ = false;


dirty_=false should update in done?

no, should be update here, so when new write come, it can be set dirty again

ilixiaocui · 2022-01-21T11:40:12Z

curvefs/src/client/inode_wrapper.cpp

+        metaClient_->GetOrModifyS3ChunkInfoAsync(
+            inode_.fsid(), inode_.inodeid(), s3ChunkInfoAdd_,
+            done);
+        s3ChunkInfoAdd_.clear();


should clear it in done?

s3ChunkInfoAdd_ should clear here， so that new s3chunkinfo can add to it when new write come

ilixiaocui · 2022-01-21T11:41:59Z

curvefs/src/client/inode_wrapper.h

@@ -220,18 +238,51 @@ class InodeWrapper {
            inode_.mutable_s3chunkinfomap());
    }

+    void MarkInodeError() {
+        // TODO(xuchaojie) : when inode is marked error, prevent futher write.
+        status_ = -1;


enum status {
}?

ilixiaocui · 2022-01-21T11:42:55Z

curvefs/src/client/rpcclient/metaserver_client.cpp

-        brpc::Controller * cntl) -> int
+        brpc::Controller * cntl, TaskExecutorDone *taskExecutorDone) -> int
+
+#define AsyncRPCTask                                                           \


AsyncRPCTask is defined same as RPCTask

not the same, the AsyncRPCTask is "capture by copy" and SyncRpcTask is "capture by reference"

ilixiaocui · 2022-01-21T11:49:40Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+                     << ", errmsg: " << MetaStatusCode_Name(ret);
+        done_->SetRetCode(ret);
+        return;
+    } else if (response.has_appliedindex()) {


why not judge s3ChunkInfoMap in GetOrModifyS3ChunkInfoResponse?

judge what? it can be empty.

ilixiaocui · 2022-01-21T11:54:23Z

curvefs/src/client/rpcclient/task_excutor.cpp

@@ -72,6 +79,41 @@ int TaskExecutor::DoRPCTask(std::shared_ptr<TaskContext> task) {
    return retCode;
 }

+void TaskExecutor::DoAsyncRPCTask(TaskExecutorDone *done) {


yeap. sync and async can be solved in same DoRPCTask distinguished by whether done is null.

wu-hanqing self-requested a review January 20, 2022 12:21

wu-hanqing reviewed Jan 21, 2022

View reviewed changes

ilixiaocui reviewed Jan 21, 2022

View reviewed changes

xu-chaojie force-pushed the master branch from c983238 to 4a9b04c Compare January 24, 2022 06:27

curvefs client : update inode async

2beb1fc

xu-chaojie force-pushed the master branch from 4a9b04c to 2beb1fc Compare January 24, 2022 06:45

wu-hanqing approved these changes Jan 24, 2022

View reviewed changes

ilixiaocui approved these changes Jan 24, 2022

View reviewed changes

xu-chaojie merged commit d0bd59d into opencurve:master Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

curvefs client : update inode async #1020

curvefs client : update inode async #1020

xu-chaojie commented Jan 19, 2022 •

edited

Loading

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022 •

edited

Loading

wu-hanqing Jan 21, 2022

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022 •

edited

Loading

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022 •

edited

Loading

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

wu-hanqing Jan 21, 2022

xu-chaojie Jan 24, 2022

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022 •

edited

Loading

ilixiaocui Jan 21, 2022

xu-chaojie Jan 24, 2022

ilixiaocui Jan 21, 2022

curvefs client : update inode async #1020

curvefs client : update inode async #1020

Conversation

xu-chaojie commented Jan 19, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-chaojie Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-chaojie Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-chaojie Jan 24, 2022 • edited Loading

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

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

xu-chaojie Jan 24, 2022 • edited Loading

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

xu-chaojie commented Jan 19, 2022 •

edited

Loading

xu-chaojie Jan 24, 2022 •

edited

Loading

xu-chaojie Jan 24, 2022 •

edited

Loading

xu-chaojie Jan 24, 2022 •

edited

Loading

xu-chaojie Jan 24, 2022 •

edited

Loading