mds/client: support discard #189

wu-hanqing · 2020-12-20T10:44:06Z

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

xu-chaojie · 2021-01-06T09:17:38Z

src/client/iomanager4file.cpp

+        aioctx->ret = -LIBCURVE_ERROR::FAILED;
+        aioctx->cb(aioctx);
+        LOG(ERROR) << "allocate tracker failed!";
+        return -LIBCURVE_ERROR::FAILED;


这个地方不需要返回错误，参考write的写法

xu-chaojie · 2021-01-06T11:59:59Z

src/client/metacache.cpp

+    }
+
+    ReadLockGuard lk(rwlock4Segments_);
+    return &segments_.at(segmentIndex);


这里好像跑不到？

Some codes have changed, please re-review this.

xu-chaojie · 2021-01-06T12:08:39Z

src/mds/common/mds_define.cpp

@@ -26,8 +26,8 @@ namespace curve {
 namespace mds {

 // TODO(xuchaojie): these should be in the configuration file later
-uint64_t DefaultSegmentSize = kGB * 1;
-uint64_t kMiniFileLength = DefaultSegmentSize * 10;
+uint64_t DefaultSegmentSize = 128 * kMB;


segmentsize 也一并改小了？这个确定了吗？

it's a mistake, fix

xu-chaojie · 2021-01-06T12:28:25Z

src/mds/nameserver2/curvefs.cpp

+inline bool CurveFS::CheckSegmentOffset(const FileInfo& fileInfo,
+                                        uint64_t offset) const {
+    if (offset % fileInfo.segmentsize() != 0) {
+        LOG(WARNING) << "offset not align with segment, offset = " << offset


这两个日志可以打Error，因为一般不会触发，有助于发现问题

xu-chaojie · 2021-01-06T12:31:33Z

src/mds/nameserver2/curvefs.cpp

+    if (snapShotFiles.size() != 0) {
+        LOG(WARNING) << fileName
+                     << " exist snapshot, num = " << snapShotFiles.size();
+        return StatusCode::kFileUnderSnapShot;


有快照的情况下，DeAllocateSegment失败，后续怎么处理？

Nothing changed including cached segment info and bitmap, and this task will be dropped.

xu-chaojie · 2021-01-06T12:34:43Z

src/mds/nameserver2/curvefs.cpp

+        return StatusCode::kParaError;
+    }
+
+    if (fileInfo.filestatus() == FileStatus::kFileBeingCloned) {


这个条件和下面这个存在快照不能删除的条件，跟CheckFileCanChange函数内容一致，是否可以直接调用CheckFileCanChange

CheckFileCanChange also checks whether the file is mounted, but we don't need it here.

xu-chaojie · 2021-01-06T12:50:01Z

src/mds/nameserver2/namespace_storage.cpp

+    }
+
+    Operation op1{
+        OpType::OpDelete,


OpDelete 为啥需要传value，没有只传key的接口？

@ilixiaocui Can you help to explain why we need value in here?

ilixiaocui · 2021-01-25T03:07:27Z

nebd/src/part2/request_executor_curve.cpp

+int CurveRequestExecutor::Discard(NebdFileInstance* fd,
+                                  NebdServerAioContext* aioctx) {
+    int curveFd = GetCurveFdFromNebdFileInstance(fd);
+    if (curveFd < 0) {


打下error日志

ilixiaocui · 2021-01-25T03:07:52Z

nebd/src/part2/request_executor_curve.cpp

+    curveCombineCtx->nebdCtx = aioctx;
+    int ret = FromNebdCtxToCurveCtx(aioctx, &curveCombineCtx->curveCtx);
+    if (ret < 0) {
+        delete curveCombineCtx;


打印error日志

ilixiaocui · 2021-01-25T03:08:15Z

nebd/src/part2/request_executor_curve.cpp

+    }
+
+    delete curveCombineCtx;
+    return -1;


打印error日志

ilixiaocui · 2021-01-25T03:16:27Z

src/client/config_info.h

+// for discard
+struct DiscardOption {
+    bool enableDiscard = false;
+    uint32_t discardTaskDelayMs = 1000 * 60 * 3;  // 3 min


这个时间是否设置为可配置？

ilixiaocui · 2021-01-25T03:29:04Z

src/mds/nameserver2/namespace_storage.h

+     *
+     * @return StoreStatus: error code
+     */
+    virtual StoreStatus DiscardSegment(const FileInfo& fileInfo,


这里增加metric统计一下需要discard的segment数量

ilixiaocui · 2021-01-25T06:53:03Z

src/client/io_tracker.h

@@ -297,6 +322,8 @@ class CURVE_CACHELINE_ALIGNMENT IOTracker {
    // 大IO被拆分成多个request，这些request放在reqlist中国保存
    std::vector<RequestContext*>   reqlist_;

+    std::vector<SegmentIndex> discardSegments_;


成员函数注释。discard相关的参数是否都封装到 DiscardOption 中

ilixiaocui · 2021-01-25T07:20:02Z

src/client/splitor.cpp

@@ -109,6 +109,45 @@ int Splitor::IO2ChunkRequests(IOTracker* iotracker, MetaCache* metaCache,
    return 0;
 }

+int Splitor::CalcDiscardSegments(IOTracker* iotracker) {
+    if (iotracker == nullptr) {


ilixiaocui · 2021-01-25T07:40:59Z

src/client/metacache_struct.h

+          chunks_() {}
+
+    /**
+     * @brief Test if all bit was discarded


test？confirm?

ilixiaocui · 2021-01-25T07:44:15Z

src/client/metacache_struct.h

+     * @return Return true if if all bits are set, otherwise return false
+     */
+    bool IsAllDiscard() {
+        return discardBitmap_.NextClearBit(0) == curve::common::Bitmap::NO_POS;


bitmap查询不需要加读锁吗

No, before calling this function, the caller has held FileSegment's write lock.

ilixiaocui · 2021-01-25T07:45:59Z

src/client/metacache_struct.h

+    void SetDiscard(const uint64_t offset, const uint32_t length);
+    void ClearDiscard(const uint64_t offset, const uint32_t length);
+
+    void ClearBitmap() {


clearDiscard和ClearBitmap有啥区别，直观上觉得ClearBitmp是ClearDiscard的一个步骤

Renamed some functions here to better explain its self.
And for ClearDiscard and ClearBitmap, ClearBitmap is equal to ClearDiscard when offset == 0 and length == segmentsize;

ilixiaocui · 2021-01-25T08:20:52Z

src/client/io_tracker.cpp

+                      << ", segment index = " << index
+                      << ", segment offset = " << index * GiB;
+        }
+    }


else错误日志？

No need to add log in here.

ilixiaocui · 2021-01-25T08:23:56Z

src/client/io_tracker.cpp

+            if (!taskManager->ScheduleTask(index, mc_, mdsClient, abstime)) {
+                LOG(ERROR) << "Schedule discard task failed";
+                ret = -1;
+                continue;


如果其中一个schedule出错，后面都是正常返回，ret还是设置为-1？

Removed this line, and always return success for discard operations. Because return error may cause filesystem error, and failed discard operations have no bad effect.

ilixiaocui · 2021-01-25T08:29:06Z

src/client/io_tracker.cpp

 int IOTracker::Wait() {
    return iocv_.Wait();
 }

 void IOTracker::Done() {
+    if (type_ == OpType::READ || type_ == OpType::WRITE) {


Optype为Discard的时候不需要释放吗

No, read/write operations will hold the corresponding segment‘s read lock in the whole process. And, discard operations hold segment's write lock when set bitmap, and after that, the write-lock is released.

ilixiaocui · 2021-01-25T08:43:01Z

src/client/io_tracker.h


 // IOTracker用于跟踪一个用户IO，因为一个用户IO可能会跨chunkserver，
 // 因此在真正下发的时候会被拆分成多个小IO并发的向下发送，因此我们需要
 // 跟踪发送的request的执行情况。
 class CURVE_CACHELINE_ALIGNMENT IOTracker {
+    friend class Splitor;


为啥不直接把CalcDiscardSegments作为IOTracker的成员函数？

Please re-review this, CalcDiscardSegments has been deleted.
And for this friend class Splitor, I think functions in Splitor take some many arguments that belong to IOTracker, either we can absorb all Splitor's functions into IOTracker or make Splitor as a friend class of IOTracker.
Now, I just add friend class Splitor in here and leave Splitor's functions parameters unchanged. Later I will refactor Splitor.

ilixiaocui · 2021-01-25T08:49:31Z

src/client/iomanager4file.cpp

@@ -111,6 +112,8 @@ void IOManager4File::UnInitialize() {
        scheduler_->Fini();
    }

+    discardTaskManager_.Stop();


stop并不一定能把任务都停掉

Can you point out which scenario can cause this problem? The Stop function first cancels all timed tasks according to its timerId, and in the last, it will wait until all tasks have been canceled or finished.

ilixiaocui · 2021-01-25T09:41:20Z

src/client/metacache.h

@@ -81,6 +81,14 @@ class MetaCache {
    virtual MetaCacheErrorType GetChunkInfoByIndex(ChunkIndex chunkidx,
                                                   ChunkIDInfo_t* chunkinfo);

+    /**
+     * 通过chunk index更新chunkid信息


英文注释

ilixiaocui · 2021-01-25T11:45:58Z

src/kvstorageclient/etcd_client.h

+     *
+     * @param[in] startKey
+     * @param[in] endKey
+     * @param[out] out key and values between [startKey, endKey)


out中备注一下key value分别代表什么

ilixiaocui · 2021-01-25T11:52:33Z

src/mds/nameserver2/clean_core.cpp

@@ -176,5 +166,80 @@ StatusCode CleanCore::CleanFile(const FileInfo & commonFile,
    progress->SetStatus(TaskStatus::SUCCESS);
    return StatusCode::kOK;
 }
+
+StatusCode CleanCore::CleanDiscardSegment(
+    const std::string& cleanSegmentKey,


加metric统计下当前待清理的segement空间

ilixiaocui · 2021-01-25T11:54:02Z

src/mds/nameserver2/clean_manager.cpp

+        running_ = true;
+        taskThread_ = curve::common::Thread(
+            &CleanDiscardSegmentTask::ScanAndExecTask, this);
+    }


if 和 else 都分别打印下日志吧

ilixiaocui · 2021-01-25T11:54:15Z

src/mds/nameserver2/clean_manager.cpp

+        sleeper_.interrupt();
+        taskThread_.join();
+        LOG(INFO) << "stop CleanDiscardSegmentTask success";
+    }


else也打下日志

ilixiaocui · 2021-01-25T12:13:35Z

src/mds/nameserver2/clean_core.cpp

+    timer.start();
+
+    // delete chunks
+    int ret = DeleteChunksInSegment(segment, seq);


progress在这个过程中不更新吗

No need to update because only progress status is useful for this task.

ilixiaocui · 2021-01-25T12:17:06Z

src/mds/nameserver2/curvefs.cpp

+        return StatusCode::kSegmentNotAllocated;
+    }
+
+    if (segment.startoffset() != offset) {


如果offset不一致，storage_->GetSegment(fileInfo.id(), offset, &segment);会返回ok吗

I think it will return KeyNotExist, but this double check is harmless.

ilixiaocui · 2021-01-25T12:17:53Z

src/mds/nameserver2/curvefs.cpp

+    }
+
+    std::vector<FileInfo> snapShotFiles;
+    if (storage_->ListSnapshotFile(fileInfo.id(), fileInfo.id() + 1,


上面已经判断了状态，这里为什么还需要listsnapshotfile

FileStatus doesn't have snapshot info.

wu-hanqing · 2021-04-27T03:09:59Z

recheck

wu-hanqing · 2021-04-27T07:08:10Z

recheck

wu-hanqing · 2021-04-28T03:10:51Z

recheck

wu-hanqing · 2021-04-28T06:15:42Z

recheck

ilixiaocui · 2021-05-10T11:42:18Z

src/client/discard_task.cpp

+        for (auto& kv : unfinishedTasks_) {
+            currentTasks.emplace(kv.first);
+        }
+    }


Why need make a copy？

if didn't make a copy here, I have to hold the lock before the for-loop, and release it after the for-loop, and it will block other tasks that going to modify unfinishedTasks_.

ilixiaocui · 2021-05-10T11:43:59Z

src/client/discard_task.cpp

+        } else if (ret == EINVAL) {
+            LOG(WARNING)
+                << "Task has been completed or some error occurs, taskid = "
+                << timerId;


if error occurs, Whether the related resources have been recycled？

The log message is changed, according to bthread_timer_del's return code, EINVAL means id does not exist. So, two scenarios can cause this, first is task is finished and task id is removed, second is task id is invalid.

ilixiaocui · 2021-05-10T12:20:52Z

src/client/io_tracker.cpp

+            timespec abstime =
+                butil::milliseconds_from_now(discardOption_.taskDelayMs);
+            if (!taskManager->ScheduleTask(index, mc_, mdsClient, abstime)) {
+                LOG(ERROR) << "Schedule discard task failed";


print filename、offset here

ilixiaocui · 2021-05-12T03:05:20Z

src/client/iomanager4file.h

@@ -66,6 +67,11 @@ class IOManager4File : public IOManager {
                    const IOOption& ioOpt,
                    MDSClient* mdsclient);

+    /**
+     * 析构，回收资源


* Project Idea #### Developer Experience ##### Kruise Deployment Wizard Proposed Project * Update project to focus on Argo Signed-off-by: Chris Aniszczyk <caniszczyk@gmail.com> Co-authored-by: Chris Aniszczyk <caniszczyk@gmail.com>

wu-hanqing force-pushed the discard-v2 branch 2 times, most recently from e67ade3 to 4617fd2 Compare December 20, 2020 12:50

wu-hanqing closed this Dec 20, 2020

wu-hanqing reopened this Dec 20, 2020

wu-hanqing force-pushed the discard-v2 branch 10 times, most recently from 2a0a318 to 8c0af56 Compare December 23, 2020 11:36

wu-hanqing marked this pull request as ready for review December 23, 2020 11:36

wu-hanqing force-pushed the discard-v2 branch from 8c0af56 to 7b0799f Compare December 23, 2020 14:21

wu-hanqing closed this Dec 23, 2020

wu-hanqing reopened this Dec 23, 2020

wu-hanqing closed this Dec 23, 2020

wu-hanqing reopened this Dec 23, 2020