[feat]curvefs/client:support warmup sym link #2723

Cyber-SiKu · 2023-08-30T09:14:14Z

What problem does this PR solve?

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-08-31T01:56:54Z

curvefs/src/client/warmup/warmup_manager.cpp

@@ -57,7 +59,7 @@ bool WarmupManagerS3Impl::AddWarmupFilelist(fuse_ino_t key,
    }
    // add warmup Progress
    if (AddWarmupProcess(key, type)) {
-        VLOG(9) << "add warmup list task:" << key;
+        LOG(INFO) << "add warmup list task:" << key;


Will this log too much?

Will this log too much?

Only when preheating is added, one will be output, which feels good.

SeanHai · 2023-08-31T01:57:13Z

curvefs/src/client/warmup/warmup_manager.cpp

@@ -83,7 +85,7 @@ bool WarmupManagerS3Impl::AddWarmupFile(fuse_ino_t key, const std::string &path,
    }
    // add warmup Progress
    if (AddWarmupProcess(key, type)) {
-        VLOG(9) << "add warmup single task:" << key;
+        LOG(INFO) << "add warmup single task:" << key;


ditto

ditto

SeanHai · 2023-08-31T02:05:48Z

curvefs/src/client/warmup/warmup_manager.cpp

+
+    if (!curve::common::StringStartWith(file, "/")) {
+        LOG(ERROR) << fmt::format("{} isn't absolute path", file);
+        file.erase(0, 1);


What if it is not an absolute path? And what is the purpose of file.erase(0, 1);

What if it is not an absolute path? And what is the purpose of file.erase(0, 1);

If it is not an absolute path, the warm-up task will be skipped.
There should be a problem with the latter one, it has been fixed

SeanHai · 2023-08-31T02:13:54Z

curvefs/src/client/warmup/warmup_manager.cpp

-        // skip links
+        std::string symLink;
+        CURVEFS_ERROR statusCode =
+            fuseOpReadLink_(0, dentry.inodeid(), &symLink);


Maybe nullptr is more readable than 0.

Maybe nullptr is more readable than 0.

fix

SeanHai · 2023-08-31T02:20:07Z

curvefs/src/client/warmup/warmup_manager.cpp

@@ -302,7 +297,26 @@ void WarmupManagerS3Impl::FetchChildDentry(fuse_ino_t key, fuse_ino_t ino) {
            };
            AddFetchDentryTask(key, task);
            VLOG(9) << "FetchChildDentry: " << dentry.inodeid();
-        } else if (FsFileType::TYPE_SYM_LINK == dentry.type()) {  // need todo
+        } else if (FsFileType::TYPE_SYM_LINK == dentry.type()) {


The logic of functions FetchDentry and FetchChildDentry is very similar, so you can consider abstracting the common parts.

The logic of functions FetchDentry and FetchChildDentry is very similar, so you can consider abstracting the common parts.

can't agree with you anymore @wuhongsong

SeanHai · 2023-08-31T02:38:44Z

curvefs/src/client/warmup/warmup_manager.cpp

@@ -719,6 +732,74 @@ void WarmupManager::CollectMetrics(InterfaceMetric *interface, int count,
    interface->latency << (butil::cpuwide_time_us() - start);
 }

+bool WarmupManagerS3Impl::GetInodeSubPathParent(


If there are source files and corresponding link files in the preheating directory, will the file be preheated multiple times?

If there are source files and corresponding link files in the preheating directory, will the file be preheated multiple times?

This depends on the situation:
If it is preheated to diskcache, although it will still be preheated, it will be skipped because diskcache has the logic to determine whether the cache exists.
However, there is no corresponding logic for kvcache, and it will be warmed up repeatedly.

wu-hanqing · 2023-08-30T11:22:46Z

curvefs/src/client/warmup/warmup_manager.cpp

+                continue;
+            }
+            std::string lastName = splitSymLink.back();
+            auto task = [this, key, parent, lastName]() {


task isn't being used.

task isn't being used.

fix

wu-hanqing · 2023-08-31T02:14:03Z

curvefs/src/client/warmup/warmup_manager.h

+     * @return false: subPath isn't belong to inode
+     */
+    bool GetInodeSubPathParent(fuse_ino_t inode,
+                               std::vector<std::string> subPath,


Suggested change

std::vector<std::string> subPath,

const std::vector<std::string>& subPath,

subPath will modify in function

wu-hanqing · 2023-08-31T02:40:37Z

curvefs/src/client/warmup/warmup_manager.cpp

+            LOG(ERROR) << "get inode fail, inodeid = " << inode;
+            return false;
+        }
+        curve::common::UniqueLock lck = inodeWrapper->GetUniqueLock();


do we support this case, e.g.,

fsmount/ -> A(symlink to ../fsmount/B) -> B

do we support this case, e.g.,

fsmount/ -> A(symlink to ../fsmount/B) -> B

no

wu-hanqing · 2023-08-31T02:41:34Z

curvefs/src/client/warmup/warmup_manager.cpp

+                           splitSymLink.end());
+            nextInode = inode;
+        } else {
+            nextInode = dentry.inodeid();


It's better to use iterative approach instead of recursion.

It's better to use iterative approach instead of recursion.

Here is to analyze the soft link, I don't think there will be too deep level calls. However, this can be modified. if you insist

Cyber-SiKu · 2023-09-13T08:42:36Z

cicheck

Cyber-SiKu · 2023-09-13T10:29:00Z

cicheck

Cyber-SiKu · 2023-09-13T10:29:26Z

cicheck

add feat support symlink by fuseOpReadLink Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>

Cyber-SiKu · 2023-09-13T13:14:08Z

cicheck

Cyber-SiKu · 2023-09-13T13:14:20Z

cicheck

Cyber-SiKu · 2023-09-14T01:38:17Z

cicheck

Cyber-SiKu · 2023-09-14T01:38:58Z

cicheck

Cyber-SiKu · 2023-09-14T01:39:13Z

cicheck

Cyber-SiKu · 2023-09-14T02:10:41Z

cicheck

Cyber-SiKu · 2023-09-14T02:10:50Z

cicheck

Cyber-SiKu force-pushed the feat/warmup_soft_link branch from c6f18df to f40f641 Compare August 30, 2023 11:06

SeanHai reviewed Aug 31, 2023

View reviewed changes

wu-hanqing reviewed Aug 31, 2023

View reviewed changes

Cyber-SiKu force-pushed the feat/warmup_soft_link branch 14 times, most recently from 9178a68 to 76d61c1 Compare September 13, 2023 08:28

Cyber-SiKu force-pushed the feat/warmup_soft_link branch from 76d61c1 to 01b0968 Compare September 13, 2023 10:28

Cyber-SiKu force-pushed the feat/warmup_soft_link branch from 01b0968 to d0d384f Compare September 13, 2023 12:50

[feat]curvefs/client:support warmup sym link

3be0880

add feat support symlink by fuseOpReadLink Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>

Cyber-SiKu force-pushed the feat/warmup_soft_link branch from d0d384f to 3be0880 Compare September 13, 2023 13:13

SeanHai approved these changes Sep 14, 2023

View reviewed changes

wuhongsong approved these changes Sep 14, 2023

View reviewed changes

wuhongsong merged commit cfb7b17 into opencurve:master Sep 14, 2023
4 checks passed

Cyber-SiKu deleted the feat/warmup_soft_link branch September 14, 2023 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]curvefs/client:support warmup sym link #2723

[feat]curvefs/client:support warmup sym link #2723

Cyber-SiKu commented Aug 30, 2023

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023 •

edited

SeanHai Aug 31, 2023

Cyber-SiKu Sep 8, 2023

wu-hanqing Aug 30, 2023

Cyber-SiKu Sep 8, 2023

wu-hanqing Aug 31, 2023

Cyber-SiKu Sep 8, 2023

wu-hanqing Aug 31, 2023

Cyber-SiKu Sep 8, 2023

wu-hanqing Aug 31, 2023

Cyber-SiKu Sep 8, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

	std::vector<std::string> subPath,
	const std::vector<std::string>& subPath,

[feat]curvefs/client:support warmup sym link #2723

[feat]curvefs/client:support warmup sym link #2723

Conversation

Cyber-SiKu commented Aug 30, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

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

Cyber-SiKu Sep 8, 2023 • edited

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

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 13, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu commented Sep 14, 2023

Cyber-SiKu Sep 8, 2023 •

edited