curvefs/client: local cache policy optimization #2064

Tangruilin · 2022-11-12T10:23:43Z

What problem does this PR solve?

Issue Number: #2025
Issue Number: #1280
Problem Summary:

What is changed and how it works?

What's Changed:
add use ReadCache and some Related function.

How it Works:
Add useReadCache to decide if use read cache. If do, write to the read cache and update to s3 async.

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

Tangruilin · 2022-11-12T15:00:58Z

curvefs/test/client/test_disk_cache_manager_impl.cpp

+    EXPECT_CALL(*diskCacheManager_, IsDiskUsedInited()).WillOnce(Return(true));
+    EXPECT_CALL(*diskCacheManager_, IsDiskCacheFull()).WillOnce(Return(false));
+    EXPECT_CALL(*diskCacheManager_, WriteReadDirect(_, _, _))
+        .WillOnce(Return(context->bufferSize));


There should have EXPECT_CALL(*diskCacheRead_, WriteDiskFile(_, _, _)).WillOnce(Return(context->bufferSize)); here, but the test will fail with it(It seems not enter the func which will be done ).

zhanghuidinah · 2022-11-14T01:35:00Z

Thanks for your contribution!

use git commit -s -a **** to satisfy DCO requirement

wuhongsong · 2022-11-14T02:55:54Z

curvefs/src/client/s3/client_s3_cache_manager.cpp

-            if (!useDiskCache) {
-                s3ClientAdaptor_->GetS3Client()->UploadAsync(*iter);
-            } else {
+            if (useDiskCache) {


if we add useReadCache, maybe we need a new name of useDiskCache?

wuhongsong · 2022-11-14T03:01:32Z

curvefs/src/client/s3/disk_cache_manager_impl.cpp

@@ -56,13 +56,37 @@ int DiskCacheManagerImpl::Init(const S3ClientAdaptorOption option) {
 }

 void DiskCacheManagerImpl::Enqueue(
-  std::shared_ptr<PutObjectAsyncContext> context) {
+  std::shared_ptr<PutObjectAsyncContext> context, bool isReadWrite) {


isReadWrite may cause confusion? isReadCacheOnly may be better?

wuhongsong · 2022-11-14T03:05:36Z

curvefs/src/client/s3/disk_cache_manager_impl.cpp

+int DiskCacheManagerImpl::WriteReadDirectClosure(
+    std::shared_ptr<PutObjectAsyncContext> context) {
+        VLOG(9) << "WriteReadClosure start, name: " << context->key;
+        int ret = WriteReadDirect(context->key,


The write may have to be put after the s3 upload is successful, otherwise if the write is successful but the client exits before the upload is successful, the data will be inconsistent

wuhongsong · 2022-11-14T12:15:07Z

curvefs/src/client/s3/disk_cache_manager_impl.cpp

+    std::shared_ptr<PutObjectAsyncContext> context) {
+        VLOG(9) << "WriteReadClosure start, name: " << context->key;
+        // Upload to s3
+        int s3Ret = client_->Upload(context->key,


synchronous upload may affects performance, maybe we can put the enqueue(WriteReadDirect) to PutObjectAsyncCallBack which is defined in DataCache::Flush？

and i think we don't need to care if the cache read disk is successfully written

yeah，I will do this

wuhongsong · 2022-11-15T04:53:08Z

cpplint isd failed

@Tangruilin

wuhongsong · 2022-11-15T08:23:30Z

curvefs/test/client/test_disk_cache_manager_impl.cpp

+    context->startTime = butil::cpuwide_time_us();
+
+    S3ClientAdaptorOption s3AdaptorOption;
+    s3AdaptorOption.diskCacheOpt.threads = 5;


The unit test is failed because code modification not covered, I guess it should be set here(s3AdaptorOption.diskCacheOpt.diskCache.diskCacheType=1)

wuhongsong

good job, LGTM

ilixiaocui · 2022-11-16T03:04:29Z

Please synthesize a commit to submit.

Tangruilin · 2022-11-16T08:52:12Z

Please synthesize a commit to submit.

@ilixiaocui done

Cyber-SiKu · 2022-11-16T09:38:44Z

curvefs/src/client/s3/client_s3_cache_manager.cpp

                return;
            }
            LOG(WARNING) << "Put object failed, key: " << context->key;
            s3ClientAdaptor_->GetS3Client()->UploadAsync(context);
        };

    std::vector<std::shared_ptr<PutObjectAsyncContext>> uploadTasks;
-    bool useDiskCache =
+    bool useReadWriteCache =


useReadCacheOnly 和 useReadWriteCache
Is it possible to add an enum, like this:

enum CachePolicy { NCache, RCache, WRCache, }

To express caching strategy?

A good idea.

So how about now？

A good idea.

So how about now？

IMO, the best way is bit operation, that is, 0 means NCache, 1 means RCache, 2 means WCache, and 3 means WRcache. Bitwise AND to calculate whether to open the corresponding operation.

enum CachePolicy { NCache = 0, RCache = 1<<0, WCache = 1<<1, WRCache=RCache|WCache, };

Whether to enable read cache:

if (cachePoily & CachePolicy::RCache) { .... }

But this will lose the readability of the code.
just an opinion

cachePoily & CachePolicy::RCache is not a good expression for enum class, and it will really lose the readability as you say. I think readability can be more important in function Flush.
Also, thanks for your suggestion, it's really a beautiful expression, I learn a lot

Tangruilin · 2022-11-16T12:36:53Z

If this looks ok, I will rebase and commit again
@wuhongsong @ilixiaocui @Cyber-SiKu

wuhongsong · 2022-11-17T01:52:43Z

recheck

ilixiaocui · 2022-11-17T02:12:36Z

curvefs/src/client/s3/client_s3_cache_manager.cpp

@@ -2226,13 +2231,20 @@ CURVEFS_ERROR DataCache::Flush(uint64_t inodeId, bool toS3) {
            << ",Len:" << tmpLen << ",blockPos:" << blockPos
            << ",blockIndex:" << blockIndex;

-    bool useReadCacheOnly =
-    s3ClientAdaptor_->IsReadCache() &&
+    if ( s3ClientAdaptor_->IsReadCache() &&


bool mayCache = s3ClientAdaptor_->HasDiskCache() && !s3ClientAdaptor_->GetDiskCacheManager()->IsDiskCacheFull() && !toS3 ; if ( mayCache) { cachePoily = IsReadWriteCache()? cachePoily::WRCache:cachePoily::RCache; } else { cachePoily = cachePoily::NCache; }

bool mayCache = s3ClientAdaptor_->HasDiskCache() && !s3ClientAdaptor_->GetDiskCacheManager()->IsDiskCacheFull() && !toS3 ; if ( mayCache) { cachePoily = IsReadWriteCache()? cachePoily::WRCache:cachePoily::RCache; } else { cachePoily = cachePoily::NCache; }

I modify the code as this, and not the ut will fail, which is odd

Cyber-SiKu · 2022-11-17T02:47:39Z

curvefs/src/client/s3/client_s3_cache_manager.cpp

@@ -2243,17 +2262,19 @@ CURVEFS_ERROR DataCache::Flush(uint64_t inodeId, bool toS3) {
                }
                VLOG(9) << "PutObjectAsyncCallBack: " << context->key
                        << " pendingReq is: " << pendingReq;
+                if (cachePoily::RCache == cachePoily) {


cachePoily::RCache == cachePoily || cachePoily::WRCache == cachePoily ？

Only ReadCache，WRCache will do s3ClientAdaptor_->GetDiskCacheManager()->Enqueue(*iter);，in which it will wirte to WCache before uploading then doing upload async.

Only ReadCache，WRCache will do s3ClientAdaptor_->GetDiskCacheManager()->Enqueue(*iter);，in which it will wirte to WCache before uploading then doing upload async.

ok

Cyber-SiKu · 2022-11-17T03:15:09Z

curvefs/src/client/s3/client_s3_cache_manager.cpp

                return;
            }
            LOG(WARNING) << "Put object failed, key: " << context->key;
            s3ClientAdaptor_->GetS3Client()->UploadAsync(context);
        };

    std::vector<std::shared_ptr<PutObjectAsyncContext>> uploadTasks;
-    bool useDiskCache =
+    bool useReadWriteCache =


A good idea.

So how about now？

IMO, the best way is bit operation, that is, 0 means NCache, 1 means RCache, 2 means WCache, and 3 means WRcache. Bitwise AND to calculate whether to open the corresponding operation.

enum CachePolicy { NCache = 0, RCache = 1<<0, WCache = 1<<1, WRCache=RCache|WCache, };

Whether to enable read cache:

if (cachePoily & CachePolicy::RCache) { .... }

But this will lose the readability of the code.
just an opinion

cachePoily & CachePolicy::RCache is not a good expression for enum class, and it will really lose the readability as you say. I think readability can be more important in function Flush.
Also, thanks for your suggestion, it's really a beautiful expression, I learn a lot

wuhongsong · 2022-11-17T07:09:59Z

recheck

wuhongsong · 2022-11-17T11:31:03Z

If this looks ok, I will rebase and commit again @wuhongsong @ilixiaocui @Cyber-SiKu

I have no further questions and have reviewed +1

wuhongsong · 2022-11-18T05:40:08Z

recheck

Cyber-SiKu

LGTM!

wuhongsong · 2022-11-18T06:57:56Z

unittest has failed @Tangruilin

Tangruilin · 2022-11-18T09:03:38Z

recheck

Tangruilin · 2022-11-18T09:29:07Z

unittest has failed @Tangruilin

It seems there are some problem, I'll slove it these days, I have try three time and it failed for two different reasons

Tangruilin · 2022-11-18T12:36:02Z

recheck

Tangruilin · 2022-11-21T02:51:24Z

recheck

Cyber-SiKu · 2022-11-21T05:52:50Z

recheck

Tangruilin · 2022-11-21T09:17:21Z

recheck

Signed-off-by: tangruilin <[email protected]>

wuhongsong · 2022-11-22T02:09:03Z

recheck

Tangruilin commented Nov 12, 2022

View reviewed changes

wuhongsong reviewed Nov 14, 2022

View reviewed changes

Tangruilin force-pushed the feature-#2025 branch 2 times, most recently from e1ae703 to fdd2804 Compare November 14, 2022 05:19

Tangruilin requested a review from wuhongsong November 14, 2022 11:44

wuhongsong reviewed Nov 14, 2022

View reviewed changes

wuhongsong changed the title ~~[Issue #2025] local cache~~ curvefs/client: local cache policy optimization Nov 14, 2022

Tangruilin requested a review from wuhongsong November 15, 2022 04:32

Tangruilin force-pushed the feature-#2025 branch from 47fa5d0 to 9def2b5 Compare November 15, 2022 05:58

wuhongsong reviewed Nov 15, 2022

View reviewed changes

Tangruilin force-pushed the feature-#2025 branch 2 times, most recently from c6e55af to dce170e Compare November 15, 2022 12:27

wuhongsong approved these changes Nov 16, 2022

View reviewed changes

ilixiaocui self-requested a review November 16, 2022 03:00

Tangruilin force-pushed the feature-#2025 branch from dce170e to 86f101f Compare November 16, 2022 06:22

Cyber-SiKu reviewed Nov 16, 2022

View reviewed changes

Tangruilin force-pushed the feature-#2025 branch from 7c89082 to 131a9c3 Compare November 16, 2022 12:39

ilixiaocui reviewed Nov 17, 2022

View reviewed changes

Cyber-SiKu reviewed Nov 17, 2022

View reviewed changes

zhanghuidinah mentioned this pull request Nov 18, 2022

[2022 Q4 Developer Activities]: Call For Participation！ #2017

Open

Tangruilin force-pushed the feature-#2025 branch from 131a9c3 to 2985361 Compare November 18, 2022 04:51

Cyber-SiKu approved these changes Nov 18, 2022

View reviewed changes

Tangruilin force-pushed the feature-#2025 branch 2 times, most recently from 796eac3 to e198f65 Compare November 18, 2022 08:34

Tangruilin force-pushed the feature-#2025 branch from e198f65 to 8ac6bd5 Compare November 21, 2022 01:23

Tangruilin force-pushed the feature-#2025 branch from c311b33 to 8ac6bd5 Compare November 21, 2022 06:52

Tangruilin force-pushed the feature-#2025 branch 4 times, most recently from 9ad3e07 to dc59822 Compare November 21, 2022 13:15

curvefs/client: local cache policy optimization

05e0102

Signed-off-by: tangruilin <[email protected]>

Tangruilin force-pushed the feature-#2025 branch from dc59822 to 05e0102 Compare November 21, 2022 13:19

ilixiaocui approved these changes Nov 22, 2022

View reviewed changes

ilixiaocui merged commit 7d9dd88 into opencurve:master Nov 22, 2022

curvefs/client: local cache policy optimization #2064

curvefs/client: local cache policy optimization #2064

Conversation

Tangruilin commented Nov 12, 2022 • edited by wuhongsong Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

zhanghuidinah commented Nov 14, 2022

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

wuhongsong commented Nov 15, 2022

Choose a reason for hiding this comment

wuhongsong left a comment

Choose a reason for hiding this comment

ilixiaocui commented Nov 16, 2022

Tangruilin commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyber-SiKu Nov 17, 2022 • edited by Tangruilin Loading

Choose a reason for hiding this comment

Tangruilin commented Nov 16, 2022

wuhongsong commented Nov 17, 2022

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

Cyber-SiKu Nov 17, 2022 • edited by Tangruilin Loading

Choose a reason for hiding this comment

wuhongsong commented Nov 17, 2022

wuhongsong commented Nov 17, 2022

wuhongsong commented Nov 18, 2022

Cyber-SiKu left a comment

Choose a reason for hiding this comment

wuhongsong commented Nov 18, 2022

Tangruilin commented Nov 18, 2022

Tangruilin commented Nov 18, 2022

Tangruilin commented Nov 18, 2022

Tangruilin commented Nov 21, 2022

Cyber-SiKu commented Nov 21, 2022

Tangruilin commented Nov 21, 2022

wuhongsong commented Nov 22, 2022

Tangruilin commented Nov 12, 2022 •

edited by wuhongsong

Loading

Cyber-SiKu Nov 17, 2022 •

edited by Tangruilin

Loading

Cyber-SiKu Nov 17, 2022 •

edited by Tangruilin

Loading