dlock: solve the concurrency issues between discard and flatten #312

SeanHai · 2021-04-12T09:35:38Z

…flatten

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

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

SeanHai · 2021-04-12T12:33:30Z

recheck

SeanHai · 2021-04-14T05:50:04Z

recheck

xu-chaojie · 2021-04-23T07:53:51Z

src/snapshotcloneserver/clone/clone_service_manager.cpp

@@ -231,6 +232,25 @@ int CloneServiceManager::Flatten(

    auto cloneInfoMetric = std::make_shared<CloneInfoMetric>(taskId);
    auto closure = std::make_shared<CloneClosure>();


not only Flatten has this bug, 'not lazy clone' also should fix.

not only Flatten has this bug, 'not lazy clone' also should fix.

The file is invisible before not lazy clone task finished, so it will not meet with discard.

xu-chaojie · 2021-04-23T07:54:55Z

src/snapshotcloneserver/clone/clone_service_manager.cpp

@@ -231,6 +232,25 @@ int CloneServiceManager::Flatten(

    auto cloneInfoMetric = std::make_shared<CloneInfoMetric>(taskId);
    auto closure = std::make_shared<CloneClosure>();
+
+    // init dlock
+    if (!useMockDLock_) {


unit test like this test nothing, add some intergration test

use gmock in test

alread use MockDLock, but the dlock is inited in Flatten based on cloneInfo.destId
even if set the mockDlock, it will be overwritten here. so a judgment is needed here anymore.

have modified the code.

xu-chaojie · 2021-04-23T08:00:03Z

src/snapshotcloneserver/clone/clone_service_manager.cpp

+        }
+    }
+
+    DLockGuard dlockGuard(dlock_);


missing lock on mds

missing lock on mds

The dlock of discard on mds is in another pr latter.

ilixiaocui · 2021-04-25T09:31:41Z

src/common/concurrent/dlock.h

+    std::string pfx;
+    int retryTimes;
+    // the interface timeout，unit is millisecond
+    int timeout;


XX_timeoutMS?

Unit included in the name

Unit included in the name

done

ilixiaocui · 2021-04-25T09:32:10Z

src/common/concurrent/dlock.h

+    // the interface timeout，unit is millisecond
+    int timeout;
+    // the session and lease timeout, unit is second
+    int ttl;


like this: ttlSec

like this: ttlSec

done

ilixiaocui · 2021-04-25T09:43:20Z

src/common/concurrent/dlock.h

+     *
+     * @return error code EtcdErrCode
+     */
+    // int TryLock();


？

EtcdMutexTryLock (not support at etcd v3.4)

EtcdMutexTryLock (not support at etcd v3.4)

add comment

also remark with //TODO

ilixiaocui · 2021-04-26T08:57:37Z

src/common/concurrent/dlock.h

+     *
+     * @return error code EtcdErrCode
+     */
+    virtual int Unlock();


why return value 'int' ？not same as 'virtual int64_t Init();'

why return value 'int' ？not same as 'virtual int64_t Init();'

becase the "GetErrCode" in etcdclient.go is unit32 type, if return C.int64_t will get error below:
cannot use GetErrCode(EtcdLock, err) (type uint32) as type _Ctype_long in return argument

ilixiaocui · 2021-04-26T09:21:55Z

src/common/concurrent/dlock.h

+        lock_->Lock();
+    }
+
+    ~DLockGuard() {


Do I have to manually call Release()?

Do I have to manually call Release()?

yes.
after set dlock in task's closure，then Release, and Unlock in callback function
DLockGuard dlockGuard(dlock_);
closure->SetDLock(dlock_);
dlockGuard.Release();

Does Releaese() used to avoid repeated destruction?

Does Releaese() used to avoid repeated destruction?

the Release() is used to not destructed and give the unlock to task callback
~DLockGuard() {
if (!release_) {
lock_->Unlock();
}
}

ilixiaocui · 2021-04-26T09:33:57Z

src/common/concurrent/dlock.cpp

+    return errCode;
+}
+
+// int DLock::TryLock() {


?

EtcdMutexTryLock (not support at etcd v3.4)

Remarks with //TODO

ilixiaocui · 2021-04-26T09:37:11Z

thirdparties/etcdclient/etcdclient.go

+	return GetErrCode(EtcdLock, err)
+}
+
+// //export EtcdMutexTryLock (not support at etcd v3.4)


why not delete?

why not delete?

if use newer version etcd later, it can be used immediately

ilixiaocui · 2021-04-26T11:14:25Z

src/snapshotcloneserver/clone/clone_service_manager.cpp

@@ -231,6 +232,25 @@ int CloneServiceManager::Flatten(

    auto cloneInfoMetric = std::make_shared<CloneInfoMetric>(taskId);
    auto closure = std::make_shared<CloneClosure>();
+
+    // init dlock
+    if (!useMockDLock_) {


use gmock in test

ilixiaocui · 2021-04-26T11:16:49Z

src/snapshotcloneserver/clone/clone_service_manager.cpp

+
+    DLockGuard dlockGuard(dlock_);
+    closure->SetDLock(dlock_);
+    dlockGuard.Release();


do nothing after lock?

do nothing after lock?

unlock fater task finished in task Run()

ilixiaocui · 2021-04-26T11:17:17Z

src/snapshotcloneserver/clone/clone_service_manager.h

@@ -362,6 +366,12 @@ class CloneServiceManager {
     */
    virtual int RecoverCloneTask();

+    // for test
+    void SetDLock(std::shared_ptr<DLock> dlock) {


not just for test
use in src/snapshotcloneserver/clone/clone_service_manager.h

ilixiaocui · 2021-04-29T02:04:27Z

src/common/concurrent/dlock.cpp

+    return errCode;
+}
+
+// int DLock::TryLock() {


Remarks with //TODO

src/common/concurrent/dlock.cpp

ilixiaocui · 2021-04-29T02:07:46Z

src/common/concurrent/dlock.h

+     *
+     * @return error code EtcdErrCode
+     */
+    // int TryLock();


also remark with //TODO

ilixiaocui · 2021-04-29T02:10:33Z

src/common/concurrent/dlock.h

+        lock_->Lock();
+    }
+
+    ~DLockGuard() {


Does Releaese() used to avoid repeated destruction?

test/common/dlock_test.cpp

Fixed typo in README

SeanHai changed the title ~~dlock: add dlock to solve the concurrency issues between discard and …~~ dlock: solve the concurrency issues between discard and flatten Apr 12, 2021

SeanHai force-pushed the curve_discard_flatten branch 2 times, most recently from 2a31a30 to 0574f3f Compare April 12, 2021 12:19

SeanHai force-pushed the curve_discard_flatten branch 7 times, most recently from 3a6e525 to 154bc76 Compare April 14, 2021 04:31

SeanHai force-pushed the curve_discard_flatten branch 6 times, most recently from 7a1df59 to 152ea61 Compare April 20, 2021 12:43

xu-chaojie reviewed Apr 23, 2021

View reviewed changes

ilixiaocui reviewed Apr 26, 2021

View reviewed changes

SeanHai force-pushed the curve_discard_flatten branch 4 times, most recently from 8230201 to ec9de0d Compare April 27, 2021 11:17

ilixiaocui reviewed Apr 29, 2021

View reviewed changes

SeanHai force-pushed the curve_discard_flatten branch from ec9de0d to e2024e9 Compare April 29, 2021 05:59

common: add dlock and use dlock in flatten

911acd7

SeanHai force-pushed the curve_discard_flatten branch from e2024e9 to 911acd7 Compare April 29, 2021 06:11

ilixiaocui approved these changes Apr 29, 2021

View reviewed changes

xu-chaojie merged commit c75f32b into opencurve:master Apr 30, 2021

ilixiaocui pushed a commit to ilixiaocui/curve that referenced this pull request Feb 6, 2023

Merge pull request opencurve#312 from kunal-kushwaha/typo-fixed

71eec3a

Fixed typo in README

		@@ -231,6 +232,25 @@ int CloneServiceManager::Flatten(

		auto cloneInfoMetric = std::make_shared<CloneInfoMetric>(taskId);
		auto closure = std::make_shared<CloneClosure>();

dlock: solve the concurrency issues between discard and flatten #312

dlock: solve the concurrency issues between discard and flatten #312

Conversation

SeanHai commented Apr 12, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

SeanHai commented Apr 12, 2021

SeanHai commented Apr 14, 2021

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

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

SeanHai Apr 26, 2021 • 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

ilixiaocui Apr 26, 2021 • 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

SeanHai Apr 26, 2021 •

edited

Loading

ilixiaocui Apr 26, 2021 •

edited

Loading