curvefs/metaserver: now we support rocksdb storage. #1214

Wine93 · 2022-03-26T13:03:51Z

No description provided.

wu-hanqing · 2022-03-30T07:40:52Z

curvefs/conf/metaserver.conf

 # metaserver max memory quota bytes (default: 30GB)
 storage.max_memory_quota_bytes=32212254720
 # metaserver max disk quota bytes (default: 2TB)
 storage.max_disk_quota_bytes=2199023255552
+# storage root dir, rocksdb's all SST and other files store under it
+storage.data_dir=/tmp  # __CURVEADM_TEMPLATE__ ${prefix}/data/storage __CURVEADM_TEMPLATE__
+# rocksdb column failmy's write_buffer_size


typo: Family ?

typo: Family ?

fixed.

wu-hanqing · 2022-03-30T07:42:09Z

curvefs/conf/metaserver.conf

+storage.rocksdb.unordered_write_buffer_size=134217728
+# rocksdb column failmy's max_write_buffer_number
+# for store inode which exclude its s3chunkinfo list (default: 5)
+storage.rocksdb.unordered_max_write_buffer_number=5


what is the unit? bytes?

what is the unit? bytes?

yep~

so, what is the meaning of 5?

wu-hanqing · 2022-03-30T07:42:09Z

curvefs/conf/metaserver.conf

+storage.rocksdb.unordered_write_buffer_size=134217728
+# rocksdb column failmy's max_write_buffer_number
+# for store inode which exclude its s3chunkinfo list (default: 5)
+storage.rocksdb.unordered_max_write_buffer_number=5


what is the unit? bytes?

what is the unit? bytes?

yep~

wu-hanqing · 2022-03-30T07:56:17Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+    // StreamStatus::RECEIVE_OK
+    auto iter = out->find(chunkIndex);
+    if (iter == out->end()) {
+        out->insert({chunkIndex, list});


out->insert({chunkIndex, std::move(list});

out->insert({chunkIndex, std::move(list});

fixed.

wu-hanqing · 2022-03-30T08:03:41Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+    if (iter == out->end()) {
+        out->insert({chunkIndex, list});
+    } else {
+        merge(list, &iter->second);


list can also move into iter->second

list can also move into iter->second

fixed.

wu-hanqing · 2022-03-30T11:20:37Z

curvefs/src/metaserver/metastore.cpp

+        buffer.clear();
+        buffer.append(std::to_string(key.chunkIndex));
+        buffer.append(":");
+        buffer.append(svalue);


void TrivialDelete(void*) {} buffer.append_user_data(svalue.data(), svalue.size(), TrivialDeleter)

this can also reduce extra copy.

void TrivialDelete(void*) {} buffer.append_user_data(svalue.data(), svalue.size(), TrivialDeleter)

this can also reduce extra copy.

practical tips! i will improve it.

wu-hanqing · 2022-03-30T11:39:30Z

curvefs/src/metaserver/dentry_storage.cpp

    return EQUAL(fsid) && EQUAL(parentinodeid) &&
           EQUAL(name) && lhs.txid() <= rhs.txid();
 }

-bool MemoryDentryStorage::IsSameDentry(const Dentry& lhs, const Dentry& rhs) {
+inline bool DentryStorage::IsSameDentry(const Dentry& lhs, const Dentry& rhs) {


for inline, it's not as we think about it from spelling, it just means that a function's definition can appear in more than one translation unit.

and if you really want a function to be unfolded in the caller, you should use magic from compiler, like __attribute__((always_inline)) in gcc

for inline, it's not as we think about it from spelling, it just means that a function's definition can appear in more than one translation unit.

and if you really want a function to be unfolded in the caller, you should use magic from compiler, like __attribute__((always_inline)) in gcc

yep, fixed.

wu-hanqing · 2022-03-30T11:44:53Z

curvefs/src/metaserver/dentry_storage.cpp

-    auto parentId = dentry.parentinodeid();
-    auto exclude = dentry.name();
-    auto txId = dentry.txid();
+MetaStatusCode DentryStorage::List(const Dentry& dentry,


list doesn't hold lock when processing?

list doesn't hold lock when processing?

fixed.

wu-hanqing · 2022-03-30T11:54:28Z

curvefs/src/metaserver/inode_manager.cpp

+MetaStatusCode InodeManager::PaddingInodeS3ChunkInfo(int32_t fsId,
+                                                     uint64_t inodeId,
+                                                     Inode* inode) {
+    NameLockGuard lg(inodeLock_, GetInodeLockName(fsId, inodeId));


if you hold lock in here, every operations of current inode will be blocked.

if you hold lock in here, every operations of current inode will be blocked.

the lock is redundant for PaddingInodeS3ChunkInfo operator, i will remove it.

wu-hanqing · 2022-03-30T12:01:06Z

curvefs/src/metaserver/storage/utils.h

+
+std::string EncodeNumber(size_t num);
+
+inline size_t DecodeNumber(std::string str) {


const std::string& str

and should you check str's length? if str.length() < sizeof(size_t), this will cause undefined behavior

const std::string& str

and should you check str's length? if str.length() < sizeof(size_t), this will cause undefined behavior

fixed

wu-hanqing · 2022-03-30T12:07:53Z

BTW, avoid sharing everything, you just need to clarify object's lifetime, if you can find out when it starts and when it ends, then a unique_ptr or raw pointer is enough

Wine93 · 2022-03-31T01:53:59Z

BTW, avoid sharing everything, you just need to clarify object's lifetime, if you can find out when it starts and when it ends, then a unique_ptr or raw pointer is enough

I will pay attention it and improve it in this branch.

h0hmj · 2022-04-01T02:14:16Z

curvefs/conf/metaserver.conf

 # metaserver max memory quota bytes (default: 30GB)
 storage.max_memory_quota_bytes=32212254720
 # metaserver max disk quota bytes (default: 2TB)
 storage.max_disk_quota_bytes=2199023255552
+# storage root dir, rocksdb's all SST and other files store under it
+storage.data_dir=./storage  # __CURVEADM_TEMPLATE__ ${prefix}/data/storage __CURVEADM_TEMPLATE__


use ./storage as the default value seems bad

use ./storage as the default value seems bad

FYH

h0hmj · 2022-04-01T03:02:49Z

curvefs/src/client/rpcclient/metaserver_client.cpp

@@ -819,6 +829,49 @@ void MetaServerClientImpl::UpdateXattrAsync(const Inode &inode,
    excutor->DoAsyncRPCTask(taskDone);
 }

+bool MetaServerClientImpl::ParseStreamBuffer(butil::IOBuf* buffer,


make function name more specific.
Client::ParseStreamBuffer is too generic
it seems only for parsing s3chunkinfo related.
maybe somthing like ParseS3MetaBuffer?

make function name more specific. Client::ParseStreamBuffer is too generic it seems only for parsing s3chunkinfo related. maybe somthing like ParseS3MetaBuffer?

yep, i will improve it.

h0hmj · 2022-04-01T07:04:27Z

thirdparties/rocksdb/Makefile

+prefix?= "$(PWD)"
+
+download:
+	@wget https://curve-build.nos-eastchina1.126.net/rocksdb-6.28.fb.tar.gz && tar -zxvf rocksdb-6.28.fb.tar.gz


library is static or shared? any dependence?
arch is amd64?

h0hmj · 2022-04-01T08:15:25Z

curvefs/src/metaserver/inode_storage.cpp

+        return MetaStatusCode::OK;
+    }
+
+    auto txn = kvStorage_->BeginTransaction();


it seems when kvstorage type is memory, BeginTransaction/EndTransaction are not implemented as expected.
when request is from compaction, AppendS3ChunkInfoList is not atomic.

it seems when kvstorage type is memory, BeginTransaction/EndTransaction are not implemented as expected. when request is from compaction, AppendS3ChunkInfoList is not atomic.

yep, maybe we can using MVCC to implement a simple transaction for our memoy storage in the next release version.

i forget we have raft for metadata operations : )
it should be okay.

h0hmj · 2022-04-01T08:36:16Z

curvefs/src/metaserver/storage/config.h~HEAD

@@ -1,5 +1,5 @@
 /*


strange filename? config.h~HEAD?

strange filename

i will remove it.

wu-hanqing · 2022-04-01T12:38:00Z

curvefs/src/client/rpcclient/channel_manager.h

+    // see issue: https://github.com/apache/incubator-brpc/issues/392
+    auto channel = std::make_shared<brpc::Channel>();
+    brpc::ChannelOptions options;
+    options.connection_type = "pooled";


maybe, you can use options.connection_group to separate normal channel and streaming channel

separate normal channel and streaming channel

yep, i will improve it.

wu-hanqing · 2022-04-01T12:39:23Z

curvefs/conf/metaserver.conf

+storage.rocksdb.unordered_write_buffer_size=134217728
+# rocksdb column failmy's max_write_buffer_number
+# for store inode which exclude its s3chunkinfo list (default: 5)
+storage.rocksdb.unordered_max_write_buffer_number=5


so, what is the meaning of 5?

wu-hanqing · 2022-04-01T12:43:48Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+    } else if (!StringToUll(out.to_string(), chunkIndex)) {
+        LOG(ERROR) << "invalid stream buffer: invalid chunkIndex";
+        return false;
+    } else if (!list->ParseFromString(buffer->to_string())) {


you can check whether https://github.com/apache/incubator-brpc/blob/4839ec2add93b439ccfe2761ce76a8914952992a/src/brpc/protocol.h#L207 is useful to reduce memory copy in here.

yep, i will improve it.

wu-hanqing · 2022-04-01T12:45:14Z

curvefs/src/client/rpcclient/metaserver_client.cpp

+    auto merge = [](const S3ChunkInfoList& from, S3ChunkInfoList* to) {
+        for (size_t i = 0; i < from.s3chunks_size(); i++) {
+            auto chunkinfo = to->add_s3chunks();
+            *chunkinfo = std::move(from.s3chunks(i));


this might not work, because the result of from.s3chunks(i) is const, and you can move from a const variable.

this might not work, because the result of from.s3chunks(i) is const, and you can move from a const variable.

fixed:

*chunkinfo = std::move(*from->mutable_s3chunks(i));

wu-hanqing · 2022-04-02T01:57:22Z

curvefs/src/metaserver/storage/memory_storage.cpp

+        }
+    }
+    {
+        WriteLockGuard writeLockGuard(rwLock_);


you should double-check in write lock

you should double-check in write lock

fixed.

wu-hanqing · 2022-04-02T01:57:27Z

curvefs/src/metaserver/storage/memory_storage.cpp

+        }
+    }
+    {
+        WriteLockGuard writeLockGuard(rwLock_);


ditto

fixed.

wu-hanqing · 2022-04-02T02:01:38Z

curvefs/src/metaserver/metastore.cpp

-    ReadLockGuard readLockGuard(rwLock_);
-    std::thread th =
-        std::thread(&MetaStoreImpl::SaveBackground, this, path, done);
+    WriteLockGuard writeLockGuard(rwLock_);


I think this lock doesn't help, even you build iterators within the lock, because this lock is released after this function is returned.

And the dump works without lock, so data that iterators point to can be modified or deleted.

For memory storage, the possibility is relatively low because you forked. But as for rocksdb, user's requests and dump work on the same database concurrently.

yep, rocksdb's problem has been fixed by snapshot, #1228

now for memory storage, we will hold write lock until child process forked, it can gurantee all data consistent.

if metastore has stored lots of data, this will block other requests, anyway, we can leave it later.

if metastore has stored lots of data, this will block other requests, anyway, we can leave it later.

actually, we only return the iterator of all datas execpt the partition iterator which is small enough, and iterate all data in the child process, so it will not lead to a performance problem.

Wine93 · 2022-04-06T09:11:50Z

recheck

wu-hanqing · 2022-04-08T01:53:09Z

curvefs/conf/metaserver.conf

 #
 # storage settings
 #
-# metaserver storage data directory
-storage.data_dir=/tmp/storage  # __CURVEADM_TEMPLATE__ ${prefix}/data __CURVEADM_TEMPLATE__
+# metaserver storage type, "momory" or "rocksdb"


typo: memory?

typo: memory?

fixed

wu-hanqing · 2022-04-08T03:11:07Z

curvefs/src/metaserver/storage/memory_storage.cpp

+Status MemoryStorage::HGet(const std::string& name,
+                           const std::string& key,
+                           ValueType* value) {
+    if (options_.compression) {


if (options_.compression) { GET_SERALIZED(UnorderedSeralizedContainer, name, key, value); return; } GET(UnorderedContainer, name, key, value);

add an explicit return here, it's more readable.

and I think those macros can be replaced by template

add an explicit return here, it's more readable.

yep, i will improve it, but the return value of those functions is Status not void, so we should return Status::OK()

and I think those macros can be replaced by template

Actually, i implement it with templates at first, but it's not easy to deal with GetContainer(), the code become more heavy, so i trun to use macros.
maybe it's not an elegant implementation, i had mark a todo and will improve it in the next commit.

wu-hanqing · 2022-04-08T03:37:59Z

curvefs/src/metaserver/storage/memory_storage.h

+class ValueWrapper {
+ public:
+    explicit ValueWrapper(const ValueType& value) {
+        RETURN_IF_CONVERT_VALUE_SUCCESS(Dentry);
+        RETURN_IF_CONVERT_VALUE_SUCCESS(Inode);
+        RETURN_IF_CONVERT_VALUE_SUCCESS(S3ChunkInfoList);
+    }
+
+    std::shared_ptr<ValueType> Message() const {
+        return value_;
+    }
+
+ private:
+    std::shared_ptr<ValueType> value_;
+};


the dynamic cast has runtime costs and the const_cast may cause undefined behavior.

but currently, you use ValueType as the interface, so it's not easy to change this.

maybe, std::any(needs C++17) or absl::any is the solution.

and, use unique_ptr is enough for this case, and Message() just return the pointer.

the dynamic cast has runtime costs and the const_cast may cause undefined behavior.

but currently, you use ValueType as the interface, so it's not easy to change this.

maybe, std::any(needs C++17) or absl::any is the solution.

maybe replace the type of interface param value toabsl::any is a good solution, but i looking through the documentation of absl::any, it states that absl::any can only constructed by copying value, it means all data will be copyed:

// An `absl::any` can only store a type that is copy-constructible; move-only // types are not allowed within an `any` object.

and, use unique_ptr is enough for this case, and Message() just return the pointer.

actually we need assignment operate, so shared_ptr is our choice:

#define SET(TYPE, NAME, KEY, VALUE) \ do { \ auto container = GET_CONTAINER(TYPE, NAME); \ auto valueWrapper = ValueWrapper(VALUE); \ auto ret = container->emplace(KEY, valueWrapper); \ if (!ret.second) { \ ret.first->second = valueWrapper; \ // <--- here } \ return Status::OK(); \ } while (0)

wu-hanqing · 2022-04-08T03:40:29Z

curvefs/src/metaserver/metastore.cpp

-    ReadLockGuard readLockGuard(rwLock_);
-    std::thread th =
-        std::thread(&MetaStoreImpl::SaveBackground, this, path, done);
+    WriteLockGuard writeLockGuard(rwLock_);


if metastore has stored lots of data, this will block other requests, anyway, we can leave it later.

h0hmj · 2022-04-08T02:48:04Z

curvefs/conf/metaserver.conf

 #
 # storage settings
 #
-# metaserver storage data directory
-storage.data_dir=/tmp/storage  # __CURVEADM_TEMPLATE__ ${prefix}/data __CURVEADM_TEMPLATE__
+# metaserver storage type, "momory" or "rocksdb"


typo “momory”

currently rocksdb is just cheaper replacement of memory cache.
"metaserver storage" may be misleading.

typo “momory”

it already fixed.

currently rocksdb is just cheaper replacement of memory cache. "metaserver storage" may be misleading.

fixed.

Wine93 · 2022-04-09T14:43:40Z

recheck

Wine93 · 2022-04-10T07:22:13Z

recheck

wu-hanqing reviewed Mar 30, 2022

View reviewed changes

h0hmj self-requested a review April 1, 2022 02:05

h0hmj reviewed Apr 1, 2022

View reviewed changes

wu-hanqing reviewed Apr 2, 2022

View reviewed changes

wu-hanqing reviewed Apr 8, 2022

View reviewed changes

h0hmj reviewed Apr 8, 2022

View reviewed changes

h0hmj approved these changes Apr 8, 2022

View reviewed changes

wu-hanqing approved these changes Apr 8, 2022

View reviewed changes

curvefs/metaserver: now we support rocksdb storage.

de6ee2d

Wine93 merged commit 79e09d3 into opencurve:master Apr 10, 2022


		std::string EncodeNumber(size_t num);

		inline size_t DecodeNumber(std::string str) {

curvefs/metaserver: now we support rocksdb storage. #1214

curvefs/metaserver: now we support rocksdb storage. #1214

Conversation

Wine93 commented Mar 26, 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

Choose a reason for hiding this comment

Wine93 Mar 31, 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

Wine93 Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

wu-hanqing commented Mar 30, 2022

Wine93 commented Mar 31, 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

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

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

Wine93 Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Wine93 commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Wine93 commented Apr 9, 2022

Wine93 commented Apr 10, 2022

Wine93 Mar 31, 2022 •

edited

Loading

Wine93 Mar 31, 2022 •

edited

Loading

Wine93 Mar 31, 2022 •

edited

Loading

wu-hanqing Apr 2, 2022 •

edited

Loading

Wine93 Apr 8, 2022 •

edited

Loading

Wine93 Apr 8, 2022 •

edited

Loading

Wine93 Apr 8, 2022 •

edited

Loading