chunkserver: added clean chunk feature #322

Wine93 · 2021-04-20T07:27:02Z

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

wu-hanqing · 2021-04-21T12:00:59Z

conf/chunkserver.conf.example

@@ -163,6 +163,10 @@ chunkfilepool.chunk_file_pool_dir=./0/
 chunkfilepool.cpmeta_file_size=4096
 # chunkfilepool get chunk最大重试次数
 chunkfilepool.retry_times=5
+# Enable clean chunk
+chunkfilepool.clean.enable=true
+# The throttle iops for cleaning chunk (4KB/IO)


IO size should be configurable

done.

In my origin design, i think the IO size is as small as better to reduce effect for user's IO request. if we want control the speed of cleaning, we can increase the limited iops.

wu-hanqing · 2021-04-21T12:04:52Z

src/chunkserver/datastore/chunkserver_chunkfile.cpp

        LOG(ERROR) << "File format version incompatible."
                    << "file version: "
                    << static_cast<uint32_t>(version)
-                    << ", format version: "
-                    << static_cast<uint32_t>(FORMAT_VERSION);
+                    << ", format version: ["


may be valid version is better, and the statis_cast is redundant

wu-hanqing · 2021-04-21T12:31:17Z

src/chunkserver/datastore/chunkserver_chunkfile.cpp

@@ -189,7 +192,14 @@ CSErrorCode CSChunkFile::Open(bool createFile) {
        char buf[pageSize_];  // NOLINT
        memset(buf, 0, sizeof(buf));
        metaPage_.encode(buf);
-        int rc = chunkFilePool_->GetFile(chunkFilePath, buf);
+
+        auto updateMetapageCb = [this](char* buffer) {


I think you can just update default version from FORMAT_VERSION to FORMAT_VERSION_V2 in ChunkFileMetaPage's constructor. And, remove this callback to update version.

We can't gurantee get clean chunk success in every operation, so if we get dirty chunk we should use FORMAT_VERSION in metapage.

We can't gurantee get clean chunk success in every operation, so if we get dirty chunk we should use FORMAT_VERSION in metapage.

I think you will get a clean chunk if you set needClean to true when calling GetChunk.
And after GetChunk returned with true, you got a cleaned chunk even if the chunk is from dirty pool, because you did fallocate it with FALLOC_FL_ZERO_RANGE. And for FALLOC_FL_ZERO_RANGE, it will mark the file as unwritten, so read from this file is just like a clean chunk file.

@wu-hanqing done

wu-hanqing · 2021-04-21T12:35:25Z

src/chunkserver/datastore/file_pool.cpp

+    std::string chunkpath = currentdir_ + "/" + std::to_string(chunkid);
+    int ret = fsptr_->Open(chunkpath.c_str(), O_RDWR);
+    if (ret < 0) {
+        LOG(ERROR) << "Open file failed: " << chunkpath.c_str();


.c_str() is redundant

wu-hanqing · 2021-04-21T12:37:58Z

src/chunkserver/datastore/file_pool.cpp

+
+    uint64_t chunklen = poolOpt_.fileSize + poolOpt_.metaPageSize;
+    if (onlyMarked) {
+        ret = fsptr_->Fallocate(fd, FALLOC_FL_ZERO_RANGE, 0, chunklen);


Does FALLOC_FL_KEEP_SIZE flag is needed?

The chunklen is fixed, I think we don't need this flag.

wu-hanqing · 2021-04-22T01:51:39Z

src/chunkserver/datastore/file_pool.cpp

+    auto popBack = [this](std::vector<uint64_t>& chunks,
+        uint64_t& chunksLeft) -> uint64_t {
+        std::unique_lock<std::mutex> lk(mtx_);
+        if (0 == chunks.size()) {


check whether a vector is empty, just use .empty() member function.

wu-hanqing · 2021-04-22T02:18:26Z

src/chunkserver/datastore/define.h

@@ -35,6 +35,7 @@ namespace chunkserver {
 using curve::common::Bitmap;

 const uint8_t FORMAT_VERSION = 1;
+const uint8_t FORMAT_VERSION_V2 = 2;


Add some annotation to explain the meaning of each version

wu-hanqing · 2021-04-22T02:56:36Z

src/chunkserver/datastore/file_pool.h

 private:
-    // Protect tmpChunkvec_
+    // The suffix of clean chunk file (".0")
+    static const char* kCleanChunkSuffix_;


Just define as std::string, so static std::string GetCleanChunkSuffix() no need to convert from const char* to std::string.
And, I think .clean suffix is more meaningful.

YES, i think it's better to use .clean suffix.

wu-hanqing · 2021-04-22T03:07:00Z

src/chunkserver/datastore/file_pool.cpp

+}
+
+uint64_t FilePool::GetChunk(bool needClean, bool* isCleaned) {
+    auto smartPop = [this](std::vector<uint64_t>& chunks1,


This lambda a little bit complex. It better to define two member functions, e.g., Push and Pop, and each one takes one argument to determine whether push/pop from dirty pool or clean pool.

By the way, if the function going to change the argument, it should task a pointer instead of a non-const reference.

wu-hanqing · 2021-04-22T03:11:07Z

src/chunkserver/datastore/file_pool.cpp

-            std::unique_lock<std::mutex> lk(mtx_);
-            if (tmpChunkvec_.empty()) {
-                LOG(ERROR) << "no avaliable chunk!";
+            chunkID = GetChunk(needClean, &isCleaned);


I'm not sure whether exist a chunk which id is 0, so it's better to let chunkID as an argument and check by the return value(true/false).

done

yeap, the valid chunk id start with 1

Wine93 · 2021-04-23T06:14:45Z

All review suggestions had been implemented @wu-hanqing

SeanHai · 2021-04-23T06:41:56Z

curve-ansible/roles/generate_config/defaults/main.yml

@@ -131,6 +131,9 @@ chunkserver_rconcurrentapply_queuedepth: 1
 chunkserver_chunkfilepool_chunk_file_pool_dir: ./0/
 chunkserver_chunkfilepool_cpmeta_file_size: 4096
 chunkserver_chunkfilepool_retry_times: 5
+chunkserver_chunkfilepool_clean_enable: true
+chunkserver_chunkfilepool_clean_bytes_per_write: 4096
+chunkserver_chunkfilepool_clean_throttle_iops: 500


in chunkserver.conf it is 2000, why here 500?

@SeanHai I will unify them.

SeanHai · 2021-04-25T05:39:57Z

src/chunkserver/datastore/file_pool.cpp

+    return true;
+}
+
+void FilePool::CleanWorker() {


how to clean the dirty chunks already in chunkfilepool?

@SeanHai The chunkserver will create a thread to clean dirty chunk when it start, see code

SeanHai · 2021-04-25T05:43:55Z

src/chunkserver/datastore/file_pool.cpp

        }
        std::unique_lock<std::mutex> lk(mtx_);
-        tmpChunkvec_.push_back(newfilenum);
-        ++currentState_.preallocatedChunksLeft;
+        dirtyChunks_.push_back(newfilenum);


if the chunkserver crashed, will the recycled dirty chunks missed?

@SeanHai No, the recycled dirty chunks already in the chunkfile directory, the dirtyChunks_ vector is only hold the chunk's number in memory.

wu-hanqing · 2021-04-25T06:25:03Z

src/chunkserver/chunkserver.cpp

        LOG_IF(FATAL, !conf->GetUInt32Value("chunkfilepool.clean.throttle_iops",
            &chunkFilePoolOptions->iops4clean));
+
+        if (chunkFilePoolOptions->bytesPerWrite < 0


You have defined chunkFilePoolOptions->bytesPerWrite as unsigned int, so it cannot less than 0.
And, I think bytesPerWrite should be aligned to 4K.

wu-hanqing · 2021-04-25T07:26:07Z

src/chunkserver/datastore/file_pool.cpp

-        memset(buffer, 0, kBytesPerWrite_);
+        uint32_t bytesPerWrite = poolOpt_.bytesPerWrite;
+
+        char *buffer = new (std::nothrow) char[bytesPerWrite];


forget free this space, and I think buffer can be a member variable, so you don't have to frequently new/delete it.

@wu-hanqing done

wu-hanqing · 2021-04-25T08:04:28Z

src/chunkserver/datastore/file_pool.h

@@ -202,7 +207,9 @@ class CURVE_CACHELINE_ALIGNMENT FilePool {
    /**
     * @brief: Return the suffix of clean chunk
     */
-    static std::string GetCleanChunkSuffix();
+    inline static std::string GetCleanChunkSuffix() {


keyword inline is redundant because member functions defined in header files are implicit inline.

@wu-hanqing done.

wu-hanqing · 2021-04-25T08:32:40Z

src/chunkserver/datastore/file_pool.cpp

+    bool ret = pop(&cleanChunks_, &currentState_.cleanChunksLeft, true)
+        || pop(&dirtyChunks_, &currentState_.dirtyChunksLeft, false);
+
+    if (false == *isCleaned && CleanChunk(*chunkid, true)) {


You should also test ret is true or not.

@wu-hanqing done.

wu-hanqing · 2021-04-25T08:51:41Z

src/chunkserver/datastore/chunkserver_chunkfile.cpp

@@ -189,7 +192,14 @@ CSErrorCode CSChunkFile::Open(bool createFile) {
        char buf[pageSize_];  // NOLINT
        memset(buf, 0, sizeof(buf));
        metaPage_.encode(buf);
-        int rc = chunkFilePool_->GetFile(chunkFilePath, buf);
+
+        auto updateMetapageCb = [this](char* buffer) {


We can't gurantee get clean chunk success in every operation, so if we get dirty chunk we should use FORMAT_VERSION in metapage.

I think you will get a clean chunk if you set needClean to true when calling GetChunk.
And after GetChunk returned with true, you got a cleaned chunk even if the chunk is from dirty pool, because you did fallocate it with FALLOC_FL_ZERO_RANGE. And for FALLOC_FL_ZERO_RANGE, it will mark the file as unwritten, so read from this file is just like a clean chunk file.

wu-hanqing · 2021-04-25T08:52:52Z

src/chunkserver/datastore/file_pool.cpp

@@ -199,7 +204,9 @@ FilePool::FilePool(std::shared_ptr<LocalFileSystem> fsptr)
    : currentmaxfilenum_(0) {
    CHECK(fsptr != nullptr) << "fs ptr allocate failed!";
    fsptr_ = fsptr;
-    tmpChunkvec_.clear();
+    cleanAlived_ = false;
+    dirtyChunks_.clear();


No need to clear it, it's empty by default.

@wu-hanqing In some test case, it will call the UnInitialize() to do some clear work, and expect to clear these vectors.

wu-hanqing · 2021-04-27T05:53:36Z

src/chunkserver/datastore/file_pool.h

+    InterruptibleSleeper cleanSleeper_;
+
+    // The buffer for write chunk file
+    std::shared_ptr<char> writeBuffer_;


std::unique_ptr<char[]> is more suitable.

std::unique_ptr<char[]> is more suitable.

done.

wu-hanqing · 2021-04-27T05:55:18Z

src/chunkserver/datastore/chunkserver_chunkfile.cpp

@@ -188,8 +187,10 @@ CSErrorCode CSChunkFile::Open(bool createFile) {
        && metaPage_.sn > 0) {
        char buf[pageSize_];  // NOLINT
        memset(buf, 0, sizeof(buf));
+        metaPage_.version = FORMAT_VERSION_V2;


Change this version in MetaPage's constructor is better, but some unit tests may be affected.

Change this version in MetaPage's constructor is better, but some unit tests may be affected.

When chunkserver start, it will scan all chunk files in the copyset directory, and these chunk files is not clean. so if we change the version in MetaPage's constructor, these dirty chunk files will cause confuse.

Add TiKV 2021 spring proposal

wu-hanqing reviewed Apr 22, 2021

View reviewed changes

SeanHai reviewed Apr 25, 2021

View reviewed changes

wu-hanqing reviewed Apr 25, 2021

View reviewed changes

wu-hanqing reviewed Apr 27, 2021

View reviewed changes

SeanHai approved these changes Apr 28, 2021

View reviewed changes

wu-hanqing approved these changes Apr 28, 2021

View reviewed changes

chunkserver: added clean chunk feature

0d7ac08

ilixiaocui approved these changes Apr 29, 2021

View reviewed changes

ilixiaocui merged commit 5ee6378 into opencurve:master Apr 29, 2021

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

Merge pull request opencurve#322 from andylokandy/master

2d8c0ac

Add TiKV 2021 spring proposal

chunkserver: added clean chunk feature #322

chunkserver: added clean chunk feature #322

Conversation

Wine93 commented Apr 20, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Wine93 Apr 22, 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

Wine93 Apr 22, 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

Choose a reason for hiding this comment

Wine93 Apr 22, 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

Wine93 Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

Wine93 commented Apr 23, 2021

Choose a reason for hiding this comment

Wine93 Apr 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Apr 25, 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

wu-hanqing Apr 25, 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

Wine93 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

Wine93 Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Wine93 Apr 22, 2021 •

edited

Loading

Wine93 Apr 22, 2021 •

edited

Loading

Wine93 Apr 22, 2021 •

edited

Loading

Wine93 Apr 22, 2021 •

edited

Loading

Wine93 Apr 25, 2021 •

edited

Loading

Wine93 Apr 25, 2021 •

edited

Loading

wu-hanqing Apr 25, 2021 •

edited

Loading

Wine93 Apr 26, 2021 •

edited

Loading

Wine93 Apr 27, 2021 •

edited

Loading