[feat]curvefs: client: optimize rmdir #2516

201341 · 2023-06-07T02:37:02Z

What problem does this PR solve?

when rmdir, it uses ListDentry to check if dir is empty, which cost to mush.

Issue Number: #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

SeanHai · 2023-06-07T08:34:58Z

Currently, the statistical information recorded in xattr is updated to the metaserver asynchronously. In the scenario where the client exits abnormally, the recorded information may be inaccurate. It is not suitable to use this in the scenario where the deletion directory is judged to be empty and has strong consistency requirements.

201341 · 2023-06-07T10:04:51Z

Currently, the statistical information recorded in xattr is updated to the metaserver asynchronously. In the scenario where the client exits abnormally, the recorded information may be inaccurate. It is not suitable to use this in the scenario where the deletion directory is judged to be empty and has strong consistency requirements.

Yes, I forget the xattr is updated asynchronously, But the dentryCacheManager method Listdentry is too heavy for rmdir, maybe we should add one method IsEmpty which call Listdentry once?

curvefs/src/client/filesystem/filesystem.h

201341 · 2023-06-14T02:45:55Z

cicheck

wuhongsong · 2023-06-19T06:09:03Z

@Wine93

Wine93 · 2023-06-20T02:28:54Z

Hi @201341, thank you for your contribute, there is a bit complex for readdir(), we cannot directly limit the entries of the directory cache even if there are many entries due to the performance considerations, but it's a pretty good solution for rmdir(), you can add a RPC which maybe named CheckDirEmpty() to check whether the directory is empty, and we also have a solution to solve how to read the large directory, we will publish the new version in the near future.
Thank you for your contribute again, it helps us to improve the performance of rmdir(), you can beautify the code and requested review from us again :)

201341 · 2023-06-20T02:48:52Z

Hi @201341, thank you for your contribute, there is a bit complex for readdir(), we cannot directly limit the entries of the directory cache even if there are many entries due to the performance considerations, but it's a pretty good solution for rmdir(), you can add a RPC which maybe named CheckDirEmpty() to check wether the directory is empty, and we also have a solution to solve how to read the large directory, we will publish the new version in the near future. Thank you for your contribute again, it helps us to improve the performance of rmdir(), you can beautify the code and requested review from us agin :)

Got it, Looking forward to the new solution.

201341 · 2023-06-25T07:12:12Z

cicheck

201341 · 2023-06-26T02:19:27Z

@Wine93 The code is ok.

Wine93 · 2023-06-30T08:32:11Z

curvefs/src/client/dentry_cache_manager.cpp

+    MetaStatusCode ret = MetaStatusCode::OK;
+    dentryList->clear();
+    ret = metaClient_->ListDentry(fsId_, parent, marker, 1, false,
+                                    dentryList);


indentation

Wine93 · 2023-06-30T08:33:14Z

curvefs/src/client/dentry_cache_manager.cpp

@@ -156,5 +156,26 @@ CURVEFS_ERROR DentryCacheManagerImpl::ListDentry(uint64_t parent,
    return CURVEFS_ERROR::OK;
 }

+CURVEFS_ERROR DentryCacheManagerImpl::CheckDirEmpty(uint64_t parent,
+                                            std::list<Dentry> *dentryList) {


indentation

the second line is larger than 80 characters?

Wine93 · 2023-06-30T08:36:33Z

curvefs/src/client/dentry_cache_manager.cpp

@@ -156,5 +156,26 @@ CURVEFS_ERROR DentryCacheManagerImpl::ListDentry(uint64_t parent,
    return CURVEFS_ERROR::OK;
 }

+CURVEFS_ERROR DentryCacheManagerImpl::CheckDirEmpty(uint64_t parent,
+                                            std::list<Dentry> *dentryList) {
+    std::string marker = "";


The initial value is empty string already.

Wine93 · 2023-06-30T08:39:03Z

curvefs/src/client/dentry_cache_manager.cpp

+    ret = metaClient_->ListDentry(fsId_, parent, marker, 1, false,
+                                    dentryList);
+    VLOG(6) << "ListDentry fsId = " << fsId_ << ", parent = " << parent
+            << ", last = " << marker << ", count = 1 "


Maybe you should trim the trailing whitespace:

<< ", last = " << marker << ", count = 1"

Wine93 · 2023-06-30T08:48:18Z

curvefs/src/client/client_operator.cpp

 CURVEFS_ERROR RenameOperator::CheckOverwrite() {
    if (dstDentry_.flag() & DentryFlag::TYPE_FILE_FLAG) {
        return CURVEFS_ERROR::OK;
    }

    std::list<Dentry> dentrys;
-    auto rc = dentryManager_->ListDentry(dstDentry_.inodeid(), &dentrys, 1);
+    auto rc = dentryManager_->CheckDirEmpty(dstDentry_.inodeid(),


The purpose we add the CheckDirEmpty is to check whether the directory is empty, it no need to list entries, so the behavior maybe like:

bool yes; auto rc = dentryManager_->CheckDirEmpty(ino, &yes); if (rc != CURVEFS_ERROR::OK) { // handler error } else if (yes) { // do something for empty directory } else { // do something for non-empty directory }

Maybe you should add a RPC to do it, the checker should handled in metaserver and it should only return yes or no.

cc @SeanHai

OK, Got it.

Signed-off-by: swj <1186093704@qq.com>

201341 · 2023-07-19T06:35:52Z

cicheck

201341 · 2023-07-20T06:49:34Z

cicheck

wuhongsong · 2023-08-14T03:21:13Z

#2680

Cyber-SiKu · 2023-08-22T02:19:47Z

@201341 please resolve the conflict.

201341 force-pushed the swj/opt-rmdir branch from b866baa to 567251a Compare June 7, 2023 03:36

201341 changed the title ~~[feat]curvefs: client: optimal rmdir~~ Jun 7, 2023

201341 force-pushed the swj/opt-rmdir branch from 567251a to ae298a0 Compare June 8, 2023 12:19

201341 changed the title ~~[feat]curvefs: client: optimize rmdir~~ Jun 8, 2023

Wine93 reviewed Jun 9, 2023

View reviewed changes

curvefs/src/client/filesystem/filesystem.h Outdated Show resolved Hide resolved

201341 force-pushed the swj/opt-rmdir branch 11 times, most recently from d0a06f8 to 8d9f5e1 Compare June 13, 2023 15:34

201341 requested review from Wine93 and ilixiaocui June 14, 2023 04:21

201341 changed the title ~~[feat]curvefs: client: optimize rmdir/readdir~~ Jun 20, 2023

201341 force-pushed the swj/opt-rmdir branch 3 times, most recently from d40f03f to 6b03012 Compare June 25, 2023 06:47

Wine93 reviewed Jun 30, 2023

View reviewed changes

Wine93 assigned Wine93 and 201341 Jul 3, 2023

201341 force-pushed the swj/opt-rmdir branch 7 times, most recently from 6e77268 to 8f918e6 Compare July 18, 2023 09:11

[feat]curvefs: client: optimize rmdir

cbc7716

Signed-off-by: swj <1186093704@qq.com>

201341 force-pushed the swj/opt-rmdir branch from 8f918e6 to cbc7716 Compare July 18, 2023 11:22

opencurve deleted a comment from Cyber-SiKu Jul 18, 2023

caoxianfei1 requested a review from SeanHai July 25, 2023 02:04

wuhongsong added the pending label Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]curvefs: client: optimize rmdir #2516

[feat]curvefs: client: optimize rmdir #2516

201341 commented Jun 7, 2023 •

edited

Loading

SeanHai commented Jun 7, 2023

201341 commented Jun 7, 2023

201341 commented Jun 14, 2023

wuhongsong commented Jun 19, 2023

Wine93 commented Jun 20, 2023 •

edited

Loading

201341 commented Jun 20, 2023

201341 commented Jun 25, 2023

201341 commented Jun 26, 2023 •

edited

Loading

Wine93 Jun 30, 2023

Wine93 Jun 30, 2023

201341 Jul 3, 2023

Wine93 Jun 30, 2023

Wine93 Jun 30, 2023 •

edited

Loading

Wine93 Jun 30, 2023 •

edited

Loading

201341 Jul 3, 2023

201341 commented Jul 19, 2023

201341 commented Jul 20, 2023

wuhongsong commented Aug 14, 2023

Cyber-SiKu commented Aug 22, 2023

[feat]curvefs: client: optimize rmdir #2516

Are you sure you want to change the base?

[feat]curvefs: client: optimize rmdir #2516

Conversation

201341 commented Jun 7, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

SeanHai commented Jun 7, 2023

201341 commented Jun 7, 2023

201341 commented Jun 14, 2023

wuhongsong commented Jun 19, 2023

Wine93 commented Jun 20, 2023 • edited Loading

201341 commented Jun 20, 2023

201341 commented Jun 25, 2023

201341 commented Jun 26, 2023 • edited Loading

Wine93 Jun 30, 2023

Choose a reason for hiding this comment

Wine93 Jun 30, 2023

Choose a reason for hiding this comment

201341 Jul 3, 2023

Choose a reason for hiding this comment

Wine93 Jun 30, 2023

Choose a reason for hiding this comment

Wine93 Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Wine93 Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

201341 Jul 3, 2023

Choose a reason for hiding this comment

201341 commented Jul 19, 2023

201341 commented Jul 20, 2023

wuhongsong commented Aug 14, 2023

Cyber-SiKu commented Aug 22, 2023

201341 commented Jun 7, 2023 •

edited

Loading

Wine93 commented Jun 20, 2023 •

edited

Loading

201341 commented Jun 26, 2023 •

edited

Loading

Wine93 Jun 30, 2023 •

edited

Loading

Wine93 Jun 30, 2023 •

edited

Loading