curvefs: support curvebs as data backend #1207

wu-hanqing · 2022-03-24T06:46:06Z

What problem does this PR solve?

Issue Number: close #1032

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 · 2022-03-24T09:11:27Z

recheck

wu-hanqing · 2022-03-24T10:40:47Z

recheck

wu-hanqing · 2022-03-24T14:11:47Z

recheck

SeanHai · 2022-03-29T09:32:57Z

curvefs/conf/client.conf

@@ -51,6 +51,7 @@ spaceserver.rpcTimeoutMs=1000

 #### bdev
 bdev.confpath=/etc/curve/client.conf
+bdev.threadnum=10


What does this threadnum used for? You can add some notes.

Didn't you say that you have used other learning interfaces before?

SeanHai · 2022-03-29T09:40:43Z

curvefs/conf/tools.conf

 volumeBlockSize=4096
 volumeName=volume
 volumeUser=user
 volumePassword=password
+volumeBlockGroupSize=134217728
+# support |AtStart| and |AtEnd|
+volumeBitmapLocation=AtStart


Each bolckGroup has a bitmap and the blockGroupSize is 128MiB and 4MiB is signed by one bit of bitmap. Only 4 bytes of bitmap on each blockGroup？ 4B + 128MiB ?

The minimum allocate granularity(block size) is 4KiB, so we need a 4KiB bitmap and each bit represents whether a block is used or not.

And 4MiB is an optimization for continuity of file data-block allocation.

you can see #959

Can we change from AtStart to AtEnd?

SeanHai · 2022-03-30T02:43:29Z

curvefs/proto/metaserver.proto

@@ -150,6 +150,7 @@ enum FsFileType {
    TYPE_FILE = 2;
    TYPE_SYM_LINK = 3;
    TYPE_S3 = 4;
+    TYPE_VOLUME = 5;


What's the meaning of FsFileType here? Kind of backend S3 or Volume ? The file type of filesystem like regular file , dir , sym_link?

Are TYPE_S3 and TYPE_VOLUME the same level as the other three?

SeanHai · 2022-03-30T02:46:25Z

curvefs/proto/metaserver.proto

@@ -194,7 +195,7 @@ message Inode {
    required FsFileType type = 14;
    optional string symlink = 15;   // TYPE_SYM_LINK only
    optional uint64 rdev = 16;
-    optional VolumeExtentList volumeExtentList = 17;  // TYPE_FILE only
+    map<uint64, VolumeExtentList> volumeExtentMap = 17;  // TYPE_FILE only


meaning of key

SeanHai · 2022-03-30T06:00:13Z

curvefs/src/client/inode_wrapper.h

@@ -67,15 +68,19 @@ class InodeWrapper : public std::enable_shared_from_this<InodeWrapper> {
        status_(InodeStatus::Normal),
        metaClient_(metaClient),
        openCount_(0),
-        dirty_(false) {}
+        dirty_(false) {
+        BuildExtentCache();


Will rebuild each time cost much when 'VolumeExtentList' is bigger in inode?

yep, so we should separate inode's attributes and s3chunkinfo/volumeextentlist later.

SeanHai · 2022-03-30T06:12:24Z

curvefs/src/client/inode_wrapper.cpp

+        //                      1. open a file with O_RDWR
+        //                      2. write "hello, world"
+        //                      3. unlink this file
+        //                      4. pread from 0 to 13, expected "hello, world"


Why expected "hello, world" after unlink the file?

the short answer is, unlink is delayed until all processes close the file.

you can see unlink's descriptions for more information.

https://linux.die.net/man/2/unlink

SeanHai · 2022-03-30T08:05:53Z

curvefs/src/mds/space/usage.cpp

+      availableSize_(availableSize),
+      totalGroups_(size / blockGroupSize),
+      allocatedGroups_(allocatedGroups.size()) {
+    for (auto& g : allocatedGroups) {


const auto& g

SeanHai · 2022-03-30T08:14:38Z

curvefs/src/tools/curvefs_tool_define.cpp

@@ -43,7 +43,7 @@ DEFINE_bool(detail, false, "show more infomation");
 DEFINE_string(partitionId, "1,2,3", "partition id");
 DEFINE_string(metaserverId, "1,2,3", "metaserver id");
 DEFINE_bool(noconfirm, false, "execute command without confirmation");
-DEFINE_uint64(blockSize, 1048576, "block size");
+DEFINE_uint64(blockSize, 0, "volume size");


Set default value same as the item in conf '4096' ?

Cyber-SiKu · 2022-04-01T02:56:26Z

curvefs/src/tools/curvefs_tool_define.cpp

@@ -43,7 +43,7 @@ DEFINE_bool(detail, false, "show more infomation");
 DEFINE_string(partitionId, "1,2,3", "partition id");
 DEFINE_string(metaserverId, "1,2,3", "metaserver id");
 DEFINE_bool(noconfirm, false, "execute command without confirmation");
-DEFINE_uint64(blockSize, 1048576, "block size");
+DEFINE_uint64(blockSize, 4096, "volume block size");


The following ‘volumeBlockSize’ is the blocksize of the volume, not this ‘blockszie’.
The interface of createfs has a blocksize, and in fsdetail, s3 and volume have their own blockszie.

ilixiaocui · 2022-04-01T03:24:26Z

curvefs/src/metaserver/metaserver.cpp

@@ -312,7 +312,7 @@ int Metaserver::PersistDataToLocalFile(std::shared_ptr<LocalFileSystem> fs,
    }

    int writtenCount = fs->Write(fd, data.c_str(), 0, data.size());
-    if (writtenCount < data.size()) {
+    if (writtenCount < 0 || static_cast<size_t>(writtenCount) != data.size()) {


Why writtenCount < data.size() does not satisfy this requirement？

nope, according to the rule, when a signed integral compare with an unsigned integral, the signed value will be converted to unsigned.

for example, data's size is 4096, and fs->Write returns -1, previous if (writtenCount < data.size) will be false.

see https://godbolt.org/z/fK1z8Penb

ilixiaocui · 2022-04-01T06:36:16Z

curvefs/src/metaserver/inode_manager.cpp

-    if (request.has_volumeextentlist()) {
-        VLOG(1) << "update inode has extent";
-        old->mutable_volumeextentlist()->CopyFrom(request.volumeextentlist());
+    // FIXME(wuhanqing): turncate


Why truncate cannot be processed now？

truncate needs release space, and we don't implement yet

ilixiaocui · 2022-04-01T06:44:49Z

curvefs/conf/client.conf

@@ -51,6 +51,7 @@ spaceserver.rpcTimeoutMs=1000

 #### bdev
 bdev.confpath=/etc/curve/client.conf
+bdev.threadnum=10


Didn't you say that you have used other learning interfaces before?

ilixiaocui · 2022-04-01T06:47:08Z

curvefs/conf/tools.conf

 volumeBlockSize=4096
 volumeName=volume
 volumeUser=user
 volumePassword=password
+volumeBlockGroupSize=134217728
+# support |AtStart| and |AtEnd|
+volumeBitmapLocation=AtStart


Can we change from AtStart to AtEnd?

ilixiaocui · 2022-04-01T06:53:06Z

curvefs/proto/common.proto

+    required uint64 blockGroupSize = 6;
+    required BitmapLocation bitmapLocation = 7;
+
+    // TODO(all): maybe we need curvebs cluster's mds ip and port in here


Maybe we can use the id to mark the curvebs cluster. The correspondence between the id and the curvebs cluster is recorded in the curvefs-mds.

Didn't you say that you have used other learning interfaces before?

it's because, for unaligned write operation, RMW is performed, so there might be a data consistent problem, like #985. and if this really happens, we should redesign the implementation here. so I just add a dedicated thread pool now.

Can we change from AtStart to AtEnd?

yep, both are supported, but they have no much difference.

Maybe we can use the id to mark the curvebs cluster. The correspondence between the id and the curvebs cluster is recorded in the curvefs-mds.

currently, it's only affects how we extend the volume, now we can only add an extra client.conf in mds.

and if we are going to support volume in different curvebs clusters, we may need ip/port or id.

ilixiaocui · 2022-04-01T11:31:12Z

curvefs/src/client/fuse_volume_client.cpp

@@ -306,5 +280,13 @@ void FuseVolumeClient::FlushData() {
    // TODO(xuchaojie) : flush volume data
 }

+void FuseVolumeClient::SetSpaceManagerForTesting(SpaceManager *manager) {


Implement FuseOpFlush. If cto, persist metadata and data, otherwise only persist data

ilixiaocui · 2022-04-01T11:36:10Z

curvefs/src/client/inode_wrapper.cpp

@@ -350,5 +396,34 @@ InodeWrapper::UpdateInodeStatus(InodeOpenStatusChange statusChange) {
    return CURVEFS_ERROR::OK;
 }

+// TODO(wuhanqing): update extents incrementally
+CURVEFS_ERROR InodeWrapper::SyncVolumeExtents() {


We have an interface for SyncVolumeExtents, why SyncFullInode doesn't just update other parts？

SyncVolumeExtents is unused and is been deleted.

But, I think this class is going to be unmaintainable, it has different responsibilities, and somewhere use this interface and somewhere else, but may do the same thing. And, it has to know the data backend.

ilixiaocui · 2022-04-01T11:37:10Z

curvefs/src/client/rpcclient/base_client.cpp

@@ -148,6 +148,60 @@ void MDSBaseClient::AllocS3ChunkId(uint32_t fsId,
    curvefs::mds::MdsService_Stub stub(channel);
    stub.AllocateS3Chunk(cntl, &request, response, nullptr);
 }
+
+// TODO(all): do we really need pass `fsId` all the time?
+//            each curve-fuse process only mount one filesystem


fsid is used to mark curve-fuse. The block group is uniquely identified by the owner,
the representation in the s3 scene is fsid.

I mean we can store fsid as a global variable, so that, we can just use this variable, and don't have to pass it as a parameter all the time and everywhere.

ilixiaocui · 2022-04-01T11:41:30Z

curvefs/src/client/volume/default_volume_storage.cpp

+            << ", len: " << len << ", block write requests: " << writes;
+
+    ssize_t nr = blockDeviceClient_->Writev(writes);
+    if (nr < 0 /*|| nr != len*/) {


Why comment out the latter part： nr != len

for unaligned requests, the return value will be bigger than the origin read/write length.

remark here

ilixiaocui · 2022-04-01T11:50:36Z

curvefs/src/client/volume/extent_cache.h

+    mutable curve::common::RWLock lock_;
+
+    // key is offset
+    std::unordered_map<uint64_t, std::map<uint64_t, PExtent>> extents_;


extents_ needs to be limited in size or length

nope, it's the latest extents of an inode.

ilixiaocui · 2022-04-06T02:22:50Z

WORKSPACE

@@ -20,7 +20,7 @@ load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")

 git_repository(
    name = "com_github_baidu_braft",
-    remote = "https://github.com/baidu/braft",
+    remote = "https://gitee.com/baidu/braft",


No need to modify here

ilixiaocui · 2022-04-06T02:37:38Z

curvefs/src/mds/space/volume_space.cpp

+
+    LockGuard lk(mtx_);
+    for (auto& group : allocatedGroups_) {
+        auto err = ClearBlockGroup(group.second);


mark as TODO here： etcdclient need to implement remove by prefix.

ilixiaocui · 2022-04-06T02:44:46Z

curvefs/src/client/fuse_volume_client.cpp

-    return MakeNode(req, parent, name, mode, FsFileType::TYPE_FILE, rdev, e);
+    VLOG(3) << "FuseOpMkNod, parent: " << parent << ", name: " << name
+            << ", mode: " << mode << ", rdev: " << rdev;
+    return MakeNode(req, parent, name, mode, FsFileType::TYPE_VOLUME, rdev, e);
 }

 CURVEFS_ERROR FuseVolumeClient::FuseOpFsync(fuse_req_t req, fuse_ino_t ino,


FuseOpFlush do not implement in line275

ilixiaocui · 2022-04-06T02:47:29Z

curvefs/src/client/volume/default_volume_storage.cpp

+            << ", len: " << len << ", block write requests: " << writes;
+
+    ssize_t nr = blockDeviceClient_->Writev(writes);
+    if (nr < 0 /*|| nr != len*/) {


remark here

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

curvefs/src/client/fuse_volume_client.cpp

@@ -298,13 +274,23 @@ CURVEFS_ERROR FuseVolumeClient::Truncate(Inode *inode, uint64_t length) {

 CURVEFS_ERROR FuseVolumeClient::FuseOpFlush(fuse_req_t req, fuse_ino_t ino,
                                            struct fuse_file_info *fi) {
-    // Todo: flush data and note cto
-    return CURVEFS_ERROR::OK;
+    VLOG(9) << "FuseOpFlush, ino: " << ino;


curvefs::client::common::FLAGS_enableCto = true, FuseOpFlush need to sync inode for cto(close will call flush internal)

wu-hanqing force-pushed the curvefs/volume-space branch 2 times, most recently from 48cbede to f6202d8 Compare March 24, 2022 08:29

wu-hanqing force-pushed the curvefs/volume-space branch 2 times, most recently from 4172780 to 85ec688 Compare March 29, 2022 02:52

SeanHai reviewed Mar 31, 2022

View reviewed changes

wu-hanqing force-pushed the curvefs/volume-space branch 3 times, most recently from 6f2e72c to fb221fd Compare March 31, 2022 11:31

Cyber-SiKu reviewed Apr 1, 2022

View reviewed changes

SeanHai approved these changes Apr 1, 2022

View reviewed changes

ilixiaocui reviewed Apr 1, 2022

View reviewed changes

wu-hanqing force-pushed the curvefs/volume-space branch 3 times, most recently from a9a9f52 to a155b56 Compare April 5, 2022 07:43

ilixiaocui reviewed Apr 6, 2022

View reviewed changes

wu-hanqing force-pushed the curvefs/volume-space branch 3 times, most recently from 5493b25 to 386fa73 Compare April 7, 2022 08:33

ilixiaocui reviewed Apr 8, 2022

View reviewed changes

ilixiaocui approved these changes Apr 8, 2022

View reviewed changes

curvefs: support curvebs as data backend

846ec93

wu-hanqing force-pushed the curvefs/volume-space branch from 386fa73 to 846ec93 Compare April 8, 2022 03:05

wu-hanqing merged commit edc5fda into opencurve:master Apr 8, 2022

wu-hanqing deleted the curvefs/volume-space branch April 13, 2022 09:35

wu-hanqing mentioned this pull request Apr 26, 2022

FsFileType definition is changed which introduce incompatibilities #1358

Closed

curvefs: support curvebs as data backend #1207

curvefs: support curvebs as data backend #1207

Conversation

wu-hanqing commented Mar 24, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

wu-hanqing commented Mar 24, 2022

wu-hanqing commented Mar 24, 2022

wu-hanqing commented Mar 24, 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

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

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