client read not allocate segment #172

SeanHai · 2020-11-26T03:06:18Z

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

再添加一下相关场景的单元测试吧。

wu-hanqing · 2020-11-26T06:40:51Z

src/client/io_tracker.cpp

+        curveCombineCtx->curveCtx.op = LIBCURVE_OP::LIBCURVE_OP_READ;
+        curveCombineCtx->curveCtx.cb = CurveAioCallback;
+
+        int ret = curveClient->AioRead(fd, &curveCombineCtx->curveCtx);


这个调用少了一个参数吧？

这个调用少了一个参数吧？

改为：
int ret = curveClient->AioRead(fd, &curveCombineCtx->curveCtx, userDataType_);

wu-hanqing · 2020-11-26T06:44:04Z

src/client/io_tracker.cpp

+                    // add zero data
+                    std::string zeroData(r->rawlength_, 0);
+                    butil::IOBuf zeroDataBuf;
+                    zeroDataBuf.append(zeroData);


IOBuf直接有一个resize的接口，可以直接用。

IOBuf直接有一个resize的接口，可以直接用。

改为：
butil::IOBuf zeroDataBuf;
zeroDataBuf.resize(r->rawlength_, 0);

wu-hanqing · 2020-11-26T06:45:58Z

src/client/io_tracker.cpp

+
+        for(auto r : reqlist_) {
+            // fake subrequest
+            if(0 == r->idinfo_.lpid_ && 0 == r->idinfo_.cpid_


这个判断可以封装一下，或者直接用ChunkIDInfo::Valid？再确认一下，正常情况下chunkid、copysetid、logicalpoolid会不会等于0

这个判断可以封装一下，或者直接用ChunkIDInfo::Valid？再确认一下，正常情况下chunkid、copysetid、logicalpoolid会不会等于0

改为：
ChunKIDInfo中新增字段 bool chunkExist = true; 标记chunk不存在时添加到metaChache中的特殊ChunkIdInfo

wu-hanqing · 2020-11-26T06:47:54Z

src/client/io_tracker.cpp

+                    std::string zeroData(r->rawlength_, 0);
+                    butil::IOBuf zeroDataBuf;
+                    zeroDataBuf.append(zeroData);
+                    readDatas_[r->subIoIndex_] = zeroDataBuf;


这里r->subIoIndex_还没赋值吧？

这里r->subIoIndex_还没赋值吧？

改为readDatas_[subIoIndex] = zeroDataBuf;

wu-hanqing · 2020-11-26T06:48:55Z

src/client/io_tracker.cpp

        ret = scheduler_->ScheduleRequest(reqlist_);
+        ret = ReadFromOrigin(originReadVec, fileInfo->userinfo);


这里会把上一步返回的值给覆盖掉

这里会把上一步返回的值给覆盖掉

改为：
ret = scheduler_->ScheduleRequest(reqlist_);
ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);
...
if (ret < 0) {
LOG(ERROR) << "split or schedule failed, return and recycle resource!";
ReturnOnFail();
}

wu-hanqing · 2020-11-26T06:53:02Z

src/client/io_tracker.cpp

+        int fd = 0;
+        {
+            std::unique_lock<std::mutex> lock(mtx_);
+            auto iter = fdMap_.find(fileName);


IOTracker是跟踪一个用户IO请求的，fdMap_放IOTracker内部，每次都是空的。
可以考虑放到FileClient4ReadClone里面去。

而且需要添加一下，定期关闭一直没有IO的卷。

bai-charisu · 2020-12-14T06:41:02Z

src/client/io_tracker.cpp

+                    fakeReqNum++;
+                } else {
+                    // read from original volume
+                    originReadVec.push_back(r);


改用emplace_back吧

改用emplace_back吧

done

bai-charisu · 2020-12-14T06:42:41Z

src/client/io_tracker.cpp

+                    // add zero data
+                    butil::IOBuf zeroDataBuf;
+                    zeroDataBuf.resize(r->rawlength_, 0);
+                    r->readData_ = zeroDataBuf;


这一段，直接r->readData_.resize就可以了吧

这一段，直接r->readData_.resize就可以了吧

done

bai-charisu · 2020-12-14T06:47:22Z

src/client/io_tracker.cpp

+                    originReadVec.push_back(r);
+                }
+
+                r->subIoIndex_ = subIoIndex++;


这个是不是跟下面的r->subIoIndex_ = subIoIndex++重复了？

这个是不是跟下面的r->subIoIndex_ = subIoIndex++重复了？

不会，这个下句就continue了

bai-charisu · 2020-12-14T06:55:05Z

src/client/io_tracker.cpp

        ret = scheduler_->ScheduleRequest(reqlist_);
+        ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);


为啥是+=？

为啥是+=？

意思是这两个都返回0（成功）才会成功，否则其中一个为失败（-1）都算失败。
这两可以表达成+ 或者 ||

bai-charisu · 2020-12-14T07:29:29Z

src/client/io_tracker.cpp

+        done->SetFailed(LIBCURVE_ERROR::FAILED);
+    }
+    delete curveCombineCtx;
+    done->SetFailed(LIBCURVE_ERROR::OK);


上面SetFailed成FAILED之后，这里又Set成OK？？

上面SetFailed成FAILED之后，这里又Set成OK？？

done

bai-charisu · 2020-12-14T07:38:06Z

test/client/iotracker_splitor_unittest.cpp

+       // LogicalPoolCopysetIDInfo_t lpcsIDInfo;
+       // mdsclient_.GetOrAllocateSegment(true, 0, &fi, &sinfo);
+       // int count = 0;
+       // for (auto iter : sinfo.chunkvec) {


为啥注释掉？

为啥注释掉？

这块注释掉的内容为调用GetOrAllocateSegment，获取一个segment，并且将相关信息（chunkIdInfo、copysetInfo）。注释掉的原因是：
1、这和上面 2. 设置GetOrAllocateSegmentresponse 功能类似，注释掉会在调用GetOrAllocateSegment时返回segment信息并更新metacache。
2、如果这更新了metacache，新增测试用例读普通卷（未写）和克隆卷（未写）时，需要调整offset到第二个segment，就与上面测试用例在形式上没法保持一致。如果保持原来不注释，可以先修改下offset测试看看

bai-charisu · 2020-12-14T07:40:18Z

test/client/iotracker_splitor_unittest.cpp

+
+    LOG(ERROR) << "address = " << &data;
+    ASSERT_EQ(0, data[0]);
+    ASSERT_EQ(0, data[4 * 1024 - 1]);


这几个断言是为了断言data是全0的吗？可以写在循环里

这几个断言是为了断言data是全0的吗？可以写在循环里

是的，验证全0；这里形式主要是为了和上面之前的用例在形式上保持一致。

bai-charisu · 2020-12-14T07:45:30Z

test/client/iotracker_splitor_unittest.cpp

+
+    LOG(ERROR) << "address = " << &data;
+    ASSERT_EQ('a', data[0]);
+    ASSERT_EQ('a', data[4 * 1024 - 1]);


全是a？可以写成，没分配的部分从源卷读到的是a，已分配的部分从当前的卷读到是b。方便验证。

全是a？可以写成，没分配的部分从源卷读到的是a，已分配的部分从当前的卷读到是b。方便验证。

这里读当前卷和源卷复用是同一个mock_schedule，这个mock_schedule中是根据请求fakedata[index%10]来填充请求数据的（fakadata{'a','b',...'j'}）,可以调整下用例中已写过的chunk index，来使结果看起来更明显。

bai-charisu · 2020-12-14T07:45:38Z

test/client/iotracker_splitor_unittest.cpp

+    LOG(ERROR) << "address = " << &data;
+    ASSERT_EQ('a', data[0]);
+    ASSERT_EQ('a', data[4 * 1024 - 1]);
+    ASSERT_EQ('a', data[4 * 1024]);


bai-charisu · 2020-12-14T07:46:04Z

test/client/iotracker_splitor_unittest.cpp

+
+    LOG(ERROR) << "address = " << &data;
+    ASSERT_EQ('a', data[0]);
+    ASSERT_EQ('a', data[4 * 1024 - 1]);


这个应该包含了上面那个全0的情况吧？是不是有点重复？

这个应该包含了上面那个全0的情况吧？是不是有点重复？

上面那个用例是读的普通卷，且读的chunk全都没写过，这个是读的普通卷，部分写过，部分没写过。严格来说也可以合成下面这个

wu-hanqing · 2020-12-14T02:34:11Z

src/client/io_tracker.h

+     *  @return 0 success; -1 fail
+     */
+    int ReadFromOrigin(std::vector<RequestContext*> reqCtxVec,
+                       UserInfo_t userInfo);


userInfo改成const引用

userInfo改成const引用

done

wu-hanqing · 2020-12-14T02:36:05Z

src/client/libcurve_client.cpp

@@ -34,10 +34,14 @@ CurveClient::~CurveClient() {
 }

 int CurveClient::Init(const std::string& configPath) {
-    return fileClient_->Init(configPath);
+    int ret = fileClient_->Init(configPath);


configPath可以保存一份全局的，FileClient4ReadClone::GetFileClient4ReadClone().Init不用放到这里

wu-hanqing · 2020-12-14T02:38:24Z

src/client/io_tracker.cpp

+        return 0;
+    }
+
+    FileClient4ReadClone &fileClient4Clone =


可以把逻辑都封装到FileClient4ReadClone里面，提供一个接口，这里直接调用就行了。

可以把逻辑都封装到FileClient4ReadClone里面，提供一个接口，这里直接调用就行了。

done

wu-hanqing · 2020-12-14T02:42:16Z

src/client/libcurve_file.h

+    FileClient4ReadClone& operator=(const FileClient4ReadClone &);
+    ~FileClient4ReadClone() {}
+
+    FileClient *fileClient_ = nullptr;


用unique_ptr吧

用unique_ptr吧

done

wu-hanqing · 2020-12-14T02:43:55Z

src/client/libcurve_file.h

+    // the mutex lock for fdMap_
+    std::mutex  mtx_;
+    // the mapping from filename to fd
+    std::unordered_map<std::string, int> fdMap_;


这里上次讨论的时候，应该是需要加一个定时关闭的功能。一段时间不用，自动把卷close。

wu-hanqing · 2020-12-14T03:10:15Z

test/client/iotracker_splitor_unittest.cpp

+// with no clonesource
+TEST_F(IOTrackerSplitorTest, StartReadNotAllocateSegment2) {
+    curvefsservice.SetGetOrAllocateSegmentFakeReturn(notallocatefakeret);
+    MockRequestScheduler* mockschuler = new MockRequestScheduler;


delete

done

wu-hanqing · 2020-12-14T03:10:25Z

test/client/iotracker_splitor_unittest.cpp

+
+TEST_F(IOTrackerSplitorTest, AsyncStartReadNotAllocateSegment2) {
+    curvefsservice.SetGetOrAllocateSegmentFakeReturn(notallocatefakeret);
+    MockRequestScheduler* mockschuler = new MockRequestScheduler;


delete

done

wu-hanqing · 2020-12-14T03:19:18Z

src/client/io_tracker.cpp

        ret = scheduler_->ScheduleRequest(reqlist_);
+        ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);


这里为什么用+=？

这里为什么用+=？

意思是这两个都返回0（成功）才会成功，否则其中一个为失败（-1）都算失败。
这两可以表达成+ 或者 ||

用if来判断吧这种方式太隐晦了，读起来很不友好

wu-hanqing · 2020-12-14T03:22:02Z

src/client/io_tracker.cpp

+        uint32_t fakeReqNum = 0;
+        std::vector<RequestContext*> originReadVec;
+
+        for (auto r : reqlist_) {


这里的逻辑有些分散了。
这里应该可以把所有的请求，都放到Schedule中，在Schedule::Process里面，根据差异再不同处理。

wu-hanqing · 2020-12-14T03:25:14Z

test/integration/snapshotcloneserver/snapshotcloneserver_common_test.cpp

@@ -334,6 +334,8 @@ class SnapshotCloneServerTest : public ::testing::Test {

        fileClient_ = new FileClient();
        fileClient_->Init(kClientConfigPath);
+        curve::client::FileClient4ReadClone::GetFileClient4ReadClone().


这里尽量把初始化放到FileClient4ReadClone内部，不要让其他模块来做初始化。

ilixiaocui · 2020-12-14T07:02:02Z

src/client/io_tracker.cpp

+                }
+
+                r->subIoIndex_ = subIoIndex++;
+                continue;


这里不需要continue吧

这里不需要continue吧

done

ilixiaocui · 2020-12-14T07:03:51Z

src/client/io_tracker.cpp

    } else {
        LOG(ERROR) << "splitor read io failed, "
                   << "offset = " << offset_ << ", length = " << length_;
    }

-    if (ret == -1) {
+    if (ret < 0) {


上面else不是已经处理了ret<0的场景？

else {
LOG(ERROR) << "splitor read io failed, " LOG(ERROR) << "splitor read io failed, "
<< "offset = " << offset_ << ", length = " << length_; << "offset = " << offset_ << ", length = " << length_;
}

上面else不是已经处理了ret<0的场景？

else {
LOG(ERROR) << "splitor read io failed, " LOG(ERROR) << "splitor read io failed, "
<< "offset = " << offset_ << ", length = " << length_; << "offset = " << offset_ << ", length = " << length_;
}

ret = scheduler_->ScheduleRequest(reqlist_);
ret += ReadFromOrigin(originReadVec, &fileInfo->userinfo);
这里都会gengxret的值，意思是两个都成功（0）才算成功

ilixiaocui · 2020-12-14T07:05:33Z

src/client/io_tracker.cpp

+        uint32_t fakeReqNum = 0;
+        std::vector<RequestContext*> originReadVec;
+
+        for (auto r : reqlist_) {


可以把这一段for封装一下

ilixiaocui · 2020-12-14T07:20:11Z

src/client/io_tracker.cpp

+        }
+        CurveAioCombineContext *curveCombineCtx = new CurveAioCombineContext();
+        curveCombineCtx->done = reqCtx->done_;
+        curveCombineCtx->curveCtx.offset = reqCtx->sourceInfo_.cloneFileOffset


为啥不直接是reqCtx->offset_

为啥不直接是reqCtx->offset_

reqCtx->offset_只是chunk内偏移，reqCtx->sourceInfo_.cloneFileOffset 在CalcRequestSourceInfo中会被计算出chunk偏移

ilixiaocui · 2020-12-14T07:25:07Z

src/client/io_tracker.cpp

        ret = scheduler_->ScheduleRequest(reqlist_);
+        ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);


这里ret为什么要加？ret返回的是0或者-1

这里ret为什么要加？ret返回的是0或者-1

想表达两个都成功才算成功，用+ 或 || 应该都行，下面进行 ret<0 判断

ilixiaocui · 2020-12-14T07:36:41Z

src/client/io_tracker.cpp

+    }
+
+    FileClient4ReadClone &fileClient4Clone =
+                            FileClient4ReadClone::GetFileClient4ReadClone();


为什么需要新增一个FileClient4ReadClone的类，直接使用原来的不行吗？无论是clone还是非clone, 读的流程不都是一样的？

为什么需要新增一个FileClient4ReadClone的类，直接使用原来的不行吗？无论是clone还是非clone, 读的流程不都是一样的？

一方面在IO_tracker中FileClient无法获取，另外如果复用原FileClient在打开源卷read时 FileClient::Read中
ReadLockGuard lk(rwlock_)这个锁无法获得

ilixiaocui · 2020-12-14T07:41:05Z

src/client/libcurve_file.h

@@ -299,6 +320,59 @@ class FileClient {
    bvar::Adder<uint64_t> openedFileNum_;
 };

+class FileClient4ReadClone {


FileClient不能满足下面的接口要求吗

FileClient不能满足下面的接口要求吗

理由同上

ilixiaocui · 2020-12-14T07:42:29Z

src/client/request_scheduler.cpp

@@ -91,6 +91,14 @@ int RequestScheduler::ScheduleRequest(
    if (running_.load(std::memory_order_acquire)) {
        /* TODO(wudemiao): 后期考虑 qos */
        for (auto it : requests) {
+            // skip the fake request
+            if (!it->idinfo_.chunkExist) {


fake的request不是不会丢在这里面吗为啥需要判断？

fake的request不是不会丢在这里面吗为啥需要判断？

丢到这里的请求包括非源卷的所有请求，最初不包括fake request，改动原因是：
如果只有真实的请求，fake请求之间在IOTracker中变量时填充fakedata, 那么reqcount_.store计数就只有真实请求，
RequestScheduler::ScheduleRequest 会对reqlist逐个处理，在场景假设有64个子请求，第一个是读写过的真实请求，后面都是fake请求，当第一个真实请求返回后在IOTracker::Done中会DestoryRequestList(); 这边遍历就会访问已经释放的空间。

ilixiaocui · 2020-12-14T07:42:57Z

src/client/splitor.cpp

                         static_cast<uint64_t>(chunkidx) * fileinfo->chunksize,
-                         mdsclient, metaCache, fileinfo)) {
+                         mdsclient, metaCache, fileinfo, chunkidx)) {


chunkidx是用于？

chunkidx是用于？

Splitor::GetOrAllocateSegment 中更新ChunkIDInfo使用
ChunkIDInfo chunkIdInfo(0, 0, 0);
chunkIdInfo.chunkExist = false;
metaCache->UpdateChunkInfoByIndex(chunkidx, chunkIdInfo);

ilixiaocui · 2020-12-14T07:51:06Z

test/client/iotracker_splitor_unittest.cpp

+       // for (auto iter : sinfo.chunkvec) {
+       //     uint64_t index = (sinfo.startoffset + count*fi.chunksize )
+       //                     / fi.chunksize;
+       //     mc->UpdateChunkInfoByIndex(index, iter);


为啥这里全部注释掉了

为啥这里全部注释掉了

这块注释掉的内容为调用GetOrAllocateSegment，获取一个segment，并且将相关信息（chunkIdInfo、copysetInfo）。注释掉的原因是：
1、这和上面 2. 设置GetOrAllocateSegmentresponse 功能类似，注释掉会在调用GetOrAllocateSegment时返回segment信息并更新metacache。
2、如果这更新了metacache，新增测试用例读普通卷（未写）和克隆卷（未写）时，需要调整offset到第二个segment，就与上面测试用例在形式上没法保持一致。如果保持原来不注释，可以先修改下offset测试看看

ilixiaocui · 2020-12-23T07:56:17Z

include/client/libcurve.h

+    /**
+     * close the timeout fd with timed thread
+     */
+    int Closefd();


既然是后台线程来删除，作为private感觉更合理

既然是后台线程来删除，作为private感觉更合理

done

ilixiaocui · 2020-12-23T08:03:53Z

include/client/libcurve.h

+    std::mutex  mtx_;
+    // the mapping from filename to fd_timestamp
+    std::unordered_map<std::string, std::string> fdMap_;
+    std::thread closeThread;


fdCloseThread

fdCloseThread
done, fdCloseThread_

ilixiaocui · 2020-12-23T08:21:03Z

include/client/libcurve.h

@@ -491,6 +501,66 @@ class CurveClient {
    FileClient* fileClient_{nullptr};
 };

+class OriginReader {


SourceReader?

SourceReader?

done

ilixiaocui · 2020-12-23T08:26:50Z

src/client/io_tracker.cpp

        ret = scheduler_->ScheduleRequest(reqlist_);
+        ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);


用if来判断吧这种方式太隐晦了，读起来很不友好

ilixiaocui · 2020-12-23T08:29:56Z

src/client/libcurve_client.cpp

+        sleeper_.interrupt();
+        closeThread.join();
+    }
+}


这里打一下reader stop的日志

这里打一下reader stop的日志

done

ilixiaocui · 2020-12-23T08:44:39Z

src/client/libcurve_client.cpp

+        }
+        CurveAioCombineContext *curveCombineCtx = new CurveAioCombineContext();
+        curveCombineCtx->done = reqCtx->done_;
+        curveCombineCtx->curveCtx.offset = reqCtx->sourceInfo_.cloneFileOffset


这里为什么需要offset相加

这里为什么需要offset相加

reqCtx->offset_只是chunk内偏移，reqCtx->sourceInfo_.cloneFileOffset 在CalcRequestSourceInfo中会被计算出chunk偏移

ilixiaocui · 2020-12-23T08:46:25Z

include/client/libcurve.h

+    // the mutex lock for fdMap_
+    std::mutex  mtx_;
+    // the mapping from filename to fd_timestamp
+    std::unordered_map<std::string, std::string> fdMap_;


这里注释一下first是什么 second是什么

这里注释一下first是什么 second是什么

done

ilixiaocui · 2020-12-23T09:00:21Z

src/client/libcurve_client.cpp

+            std::unique_lock<std::mutex> lock(mtx_);
+            auto iter = fdMap_.find(fileName);
+            if (iter != fdMap_.end()) {
+                curve::common::SplitString(iter->second, FD_TIME_DELIM,


这里直接封装一个函数出去吧
Getfd()

ilixiaocui · 2020-12-23T09:01:16Z

src/client/libcurve_client.cpp

+                std::vector<std::string> fdtimestamp;
+                curve::common::SplitString(iter->second, FD_TIME_DELIM,
+                                                        &fdtimestamp);
+                if (fdtimestamp.size() != 2) {


这里是第三次重复了

这里是第三次重复了

done

ilixiaocui · 2020-12-23T09:02:16Z

src/client/libcurve_client.cpp

+
+                int fd = std::stoi(fdtimestamp[0]);
+                time_t timestamp = std::stol(fdtimestamp[1]);
+                if (time(0) - timestamp > FD_TIMEOUT) {


这样减出来不是负的吗

这样减出来不是负的吗

不会，time(0)是现在的时间，timestamp是之前访问时保存的，所以肯定大于timestamp

wu-hanqing · 2020-12-23T09:38:08Z

src/client/splitor.cpp

@@ -180,10 +180,12 @@ bool Splitor::AssignInternal(IOTracker* iotracker, MetaCache* metaCache,
        metaCache->GetChunkInfoByIndex(chunkidx, &chunkIdInfo);

    if (errCode == MetaCacheErrorType::CHUNKINFO_NOT_FOUND) {


这里应该少了一个条件，如果读请求的时候，把chunk标记为chunkExist=false，后续的写请求会拿到一个chunk不存在的ChunkInfo。这种情况下，也需要取分配segment

这里应该少了一个条件，如果读请求的时候，把chunk标记为chunkExist=false，后续的写请求会拿到一个chunk不存在的ChunkInfo。这种情况下，也需要取分配segment

done

wu-hanqing · 2020-12-23T09:45:11Z

include/client/libcurve.h

@@ -491,6 +501,66 @@ class CurveClient {
    FileClient* fileClient_{nullptr};
 };

+class OriginReader {


这个类没必要暴露出去，在client内部维护就可以了。这样就不需要其他模块初始化这个类。
可以在第一次使用的时候，进行初始化，比如IOTracker::ReadFromOrigin这个地方。

done
在curveclient初始化时进行 sourcereader的初始化，如果放到IOTracker::ReadFromSource每次都会去尝试初始化，并且感觉sourcereader和curveclient的级别好像差不多

wu-hanqing · 2020-12-23T09:47:17Z

include/client/libcurve.h

+    // the mutex lock for fdMap_
+    std::mutex  mtx_;
+    // the mapping from filename to fd_timestamp
+    std::unordered_map<std::string, std::string> fdMap_;


这里改成std::unordered_map<std::string, std::pair<int, uint64_t> fdMap_
std::pair<int, uint64_t> 第一个存fd，第二个存timestamp
或者也可以把<fd,timestamp>再封装一次，用string存，太蛮烦了。

这里改成std::unordered_map<std::string, std::pair<int, uint64_t> fdMap_
std::pair<int, uint64_t> 第一个存fd，第二个存timestamp
或者也可以把<fd,timestamp>再封装一次，用string存，太蛮烦了。

done

wu-hanqing · 2020-12-23T09:48:21Z

include/client/libcurve.h


 #define IO_ALIGNED_BLOCK_SIZE 4096
 #define PATH_MAX_SIZE         4096
 #define NAME_MAX_SIZE         256

+#define FD_TIME_DELIM std::string("_")
+#define FD_TIMEOUT             300


这里的参数，用配置文件设置吧。

这里的参数，用配置文件设置吧。

done

wu-hanqing · 2020-12-23T09:50:15Z

src/client/libcurve_client.cpp

+    brpc::ClosureGuard doneGuard(done);
+}
+
+OriginReader::OriginReader() : fileClient_(new FileClient()) {}


这个类的实现，放到一个单独的cpp文件中吧。

这个类的实现，放到一个单独的cpp文件中吧。

done

wu-hanqing · 2020-12-23T10:54:37Z

include/client/libcurve.h

+
+    FileClient *fileClient_{nullptr};
+    // the mutex lock for fdMap_
+    std::mutex  mtx_;


这个锁改成读写锁吧

这个锁改成读写锁吧

done

wu-hanqing · 2020-12-24T06:38:11Z

src/client/libcurve_client.cpp

@@ -124,4 +131,4 @@ void CurveClient::SetFileClient(FileClient* client) {
 }

 }  // namespace client
-}  // namespace curve
+}  // namespace curve


文件末尾应该要有一个空行，没有cpplint会报错

wu-hanqing · 2020-12-24T06:39:43Z

src/client/libcurve_file.h

@@ -269,6 +269,31 @@ class FileClient {
        return openedFileNum_.get_value();
    }

+    /**


这里几个函数的缩进

这里几个函数的缩进

done

wu-hanqing · 2020-12-24T06:46:35Z

src/client/source_reader.cpp

+        std::string fileName = reqCtx->sourceInfo_.cloneFileSource;
+        int fd = 0;
+        {
+            curve::common::WriteLockGuard writeLockGuard(rwLock_);


这个锁的流程，应该是先拿读锁，如fdMap_里面查找，如果找到了，就可以直接用。
如果没找到，拿写锁去新建一个。

wu-hanqing · 2020-12-24T06:51:25Z

src/client/source_reader.cpp

+                    return -1;
+                }
+            }
+            fdMap_[fileName] = std::make_pair(fd, time(0));


这里为什么都要更新成time(0)？
我的理解是，如果新建，或者使用的时候，更新timestap为当前的时间，比如用GetTimeofDayMs来记录。
后台线程在扫描关闭的时候，对比当前的时间戳与fd的时间戳，如果超过阈值，就关闭。

这里为什么都要更新成time(0)？
我的理解是，如果新建，或者使用的时候，更新timestap为当前的时间，比如用GetTimeofDayMs来记录。
后台线程在扫描关闭的时候，对比当前的时间戳与fd的时间戳，如果超过阈值，就关闭。

time(0)就是获取现在时间戳

wu-hanqing · 2020-12-24T06:53:18Z

src/client/splitor.cpp

@@ -194,6 +196,15 @@ bool Splitor::AssignInternal(IOTracker* iotracker, MetaCache* metaCache,
        int ret = 0;
        uint64_t appliedindex_ = 0;

+        // check whether the chunkIdInfo is normal
+        if (!chunkIdInfo.chunkExist && iotracker->Optype() == OpType::WRITE) {


这里的判断，最好放到183行吧

这里的判断，最好放到183行吧

done

ilixiaocui · 2020-12-24T07:24:56Z

src/client/io_tracker.cpp

+int IOTracker::ReadFromSource(std::vector<RequestContext*> reqCtxVec,
+                              const UserInfo_t& userInfo) {
+    SourceReader &sourceReader = SourceReader::GetInstance();
+    return sourceReader.Read(reqCtxVec, userInfo);


SourceReader::GetInstance().Read(reqCtxVec, userInfo)?

SourceReader::GetInstance().Read(reqCtxVec, userInfo)?

done

ilixiaocui · 2020-12-24T07:25:39Z

src/client/io_tracker.h

@@ -246,6 +246,15 @@ class CURVE_CACHELINE_ALIGNMENT IOTracker {
    // perform read operation
    void DoRead(MDSClient* mdsclient, const FInfo_t* fileInfo);

+    /**
+     *  read from the origin


read from source

read from source

done

ilixiaocui · 2020-12-24T09:43:45Z

src/client/libcurve_client.cpp

+       0 == SourceReader::GetInstance().Init(configPath)) {
+        SourceReader::GetInstance().Run();
+        return LIBCURVE_ERROR::OK;
+    } else {


error日志

error日志

done

ilixiaocui · 2020-12-24T10:53:09Z

src/client/source_reader.cpp

+        sleeper_.interrupt();
+        fdCloseThread_.join();
+        LOG(INFO) << "SourceReader fdCloseThread stoped successfully";
+    }


else 也打下日志吧 not running or already stopped

else 也打下日志吧 not running or already stopped

done

ilixiaocui · 2020-12-24T10:53:59Z

src/client/source_reader.cpp

+    }
+    if (fileClient_ != nullptr) {
+        for (auto &pair : fdMap_) {
+            fileClient_->Close(pair.second.first);


close有返回值吗？

close有返回值吗？

有的，修改后增加了判断
if (LIBCURVE_ERROR::OK != fileClient_->Close(pair.second.first)) {
LOG(ERROR) << "Close fd failed, fd = " << pair.second.first;
return;
}

ilixiaocui · 2020-12-24T10:59:42Z

src/client/source_reader.cpp

+        curve::common::ReadLockGuard readLockGuard(rwLock_);
+        auto iter = fdMap_.find(fileName);
+        if (iter != fdMap_.end()) {
+            iter->second.second = time(0);


为什么这里赋值为time(0)

为什么这里赋值为time(0)

time(0)就是当前时间戳

ilixiaocui · 2020-12-24T11:01:06Z

src/client/source_reader.cpp

+    }
+    {
+        int fd = 0;
+        curve::common::WriteLockGuard lk(rwLock_);


跟curve::common::ReadLockGuard readLockGuard(rwLock_); 命名一致吧？

跟curve::common::ReadLockGuard readLockGuard(rwLock_); 命名一致吧？

done

ilixiaocui · 2020-12-24T11:01:25Z

src/client/source_reader.cpp

+    {
+        int fd = 0;
+        curve::common::WriteLockGuard lk(rwLock_);
+        auto iter = fdMap_.find(fileName);


为啥这里还需要再find一次，而不是直接open

为啥这里还需要再find一次，而不是直接open

因为上面拿的是读锁，有可能多个线程同时获取到然后去fdMap中查询结果都为不存在，下面获取写锁时，其中一个首先获得，其他等待，如果不再查询一次就会出现打开多次的问题

ilixiaocui · 2020-12-24T12:12:46Z

test/client/fake/mock_schedule.cpp

+       //     iter->done_->SetFailed(0);
+       //     iter->done_->Run();
+       //     break;
+       // }


不要的代码直接删掉吧

不要的代码直接删掉吧

done

After opencurve#172 and opencurve#209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, this `FileInstance::mdsclient_` become dangling pointer. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After opencurve#172 and opencurve#209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After #172 and #209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After opencurve#172 and opencurve#209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After #172 and #209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After opencurve#172 and opencurve#209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

After #172 and #209, source volume data of the clone volume can be read within the client through SourceReader, and each source volume was also opened and represented by a FileInstance, but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`, so after FileClient is destroyed, `FileInstance::mdsclient_` becomes dangling pointer. To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr, and each FileInstance holds ownership of it. Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

CNCF Community Bridge mentors evaluation form added

wu-hanqing requested changes Nov 26, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch 5 times, most recently from 08fa2f1 to c5f812e Compare December 13, 2020 04:02

bai-charisu reviewed Dec 14, 2020

View reviewed changes

wu-hanqing reviewed Dec 14, 2020

View reviewed changes

ilixiaocui reviewed Dec 14, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch 11 times, most recently from 548340d to 2831bea Compare December 23, 2020 02:06

ilixiaocui reviewed Dec 23, 2020

View reviewed changes

wu-hanqing reviewed Dec 23, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch 2 times, most recently from ce7655a to 70d88df Compare December 24, 2020 05:40

wu-hanqing reviewed Dec 24, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch from 70d88df to db9c4ec Compare December 24, 2020 09:00

ilixiaocui reviewed Dec 24, 2020

View reviewed changes

wu-hanqing approved these changes Dec 24, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch from db9c4ec to b02d604 Compare December 24, 2020 12:05

ilixiaocui reviewed Dec 24, 2020

View reviewed changes

SeanHai force-pushed the curve_client_readNotAllocateSegment branch 2 times, most recently from 3f2d861 to f3292a0 Compare December 24, 2020 12:28

client read not allocate segment

211176c

SeanHai force-pushed the curve_client_readNotAllocateSegment branch from f3292a0 to 211176c Compare December 25, 2020 02:10

ilixiaocui approved these changes Dec 25, 2020

View reviewed changes

ilixiaocui merged commit fcd778a into opencurve:master Dec 25, 2020

SeanHai deleted the curve_client_readNotAllocateSegment branch April 29, 2021 01:48

wu-hanqing mentioned this pull request May 19, 2021

client: fix segment fault in SourceReader #358

Merged

2 tasks

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

Merge pull request opencurve#172 from cncf/idvoretskyi-patch-1

71fb57a

CNCF Community Bridge mentors evaluation form added

		ret = scheduler_->ScheduleRequest(reqlist_);
		ret = ReadFromOrigin(originReadVec, fileInfo->userinfo);

		ret = scheduler_->ScheduleRequest(reqlist_);
		ret += ReadFromOrigin(originReadVec, fileInfo->userinfo);

		@@ -180,10 +180,12 @@ bool Splitor::AssignInternal(IOTracker* iotracker, MetaCache* metaCache,
		metaCache->GetChunkInfoByIndex(chunkidx, &chunkIdInfo);

		if (errCode == MetaCacheErrorType::CHUNKINFO_NOT_FOUND) {

client read not allocate segment #172

client read not allocate segment #172

Conversation

SeanHai commented Nov 26, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

wu-hanqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanHai Nov 26, 2020 • 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

SeanHai Nov 26, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanHai Nov 26, 2020 •

edited

Loading

SeanHai Nov 26, 2020 •

edited

Loading