Curvefs: enable async read for FileCacheManager::ReadKVRequest
#2421
Conversation
|
cicheck |
1 similar comment
|
cicheck |
|
cicheck |
|
cicheck |
|
cicheck |
|
|
||
| // read from kv cluster (localcache -> remote kv cluster -> s3) | ||
| // localcache/remote kv cluster fail will not return error code. | ||
| // Failure to read from s3 will eventually return failure. | ||
| int ret = ReadKVRequest(kvRequest, dataBuf, inodeWrapper->GetLength()); | ||
| int ret = ReadKVRequest(kvRequests, dataBuf, inodeWrapper->GetLength()); |
There was a problem hiding this comment.
Declare the return value of ReadKVRequest as an enum here? @ilixiaocui
| kvClientManager_(std::move(kvClientManager)) {} | ||
| FileCacheManager() {} | ||
| kvClientManager_(std::move(kvClientManager)) { | ||
| readTaskPool_.Start(4); |
| uint64_t fileLen, | ||
| std::once_flag &cancelFlag, | ||
| std::atomic<bool> &isCanceled, | ||
| std::atomic<int> &ret) { |
There was a problem hiding this comment.
It will be set when read from s3 failed.
| auto defer = absl::MakeCleanup([&]() { counter.DecrementCount(); }); | ||
| if (isCanceled) { | ||
| LOG(WARNING) << "kv request is canceled " << req.DebugString(); | ||
| return; |
There was a problem hiding this comment.
we need retry in FileCacheManager::Read , so if its failed here, we need return a meaningful value
Ziy1-Tan
changed the title
FileCacheManager::ProcessKVRequestFileCacheManager::ReadKVRequest
|
cicheck |
|
cicheck |
|
cicheck |
|
cicheck |
Ziy1-Tan
force-pushed
the
async
branch
2 times, most recently
from
851ed2f
to
6f50947
Compare
|
cicheck |
|
unittest run timeout, you can run it in you testing environment and look why |
|
cicheck |
|
@wuhongsong @ilixiaocui Mind take a look? |
| const std::vector<S3ReadRequest> &kvRequests, char *dataBuf, | ||
| uint64_t fileLen) { | ||
| readTaskPool_.Start(readCacheThreads_); |
There was a problem hiding this comment.
maybe the thread pool shouldn't be in FileCacheManager, If a large number of files are read at the same time, the number of threads can get out of hand?
There was a problem hiding this comment.
Maybe bthread can work? Bthread is an M:N thread, multiple Bthreads can be declared in the same Read thread
There was a problem hiding this comment.
Maybe bthread can work? Bthread is an M:N thread, multiple Bthreads can be declared in the same Read thread
sorry, I don't think it's a good way, we just need a configurable number of threads, so i think move the thread pool toFsCacheManagermaybe better?
There was a problem hiding this comment.
Do you mean that all FileCacheManager share a thread pool?
There was a problem hiding this comment.
Do you mean that all
FileCacheManagershare a thread pool?
yes
|
cicheck |
|
cicheck |
1 similar comment
|
cicheck |
|
cicheck |
1 similar comment
|
cicheck |
|
|
||
| ReadStatus st = ReadStatus::OK; | ||
| if (retCode.load() < 0) { | ||
| st = retCode.load() == -2 ? ReadStatus::S3_NOT_EXIST |
There was a problem hiding this comment.
It is recommended to encapsulate a function like ReadStatus ToReadStatus(int ret)
|
|
||
| // prefetch | ||
| if (s3ClientAdaptor_->HasDiskCache()) { |
There was a problem hiding this comment.
Prefetch() locate at line 583.
Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>
|
cicheck |
What problem does this PR solve?
Issue Number: #2070
Problem Summary:
FileCacheManager::ProcessKVRequestprocesses kvRequests one by one, maybe we can increase the concurrencyWhat is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List