curve-fuse: update attr and extent in single rpc (#1779) #1784
Conversation
|
recheck |
1 similar comment
|
recheck |
|
recheck |
|
recheck |
|
recheck |
3 similar comments
|
recheck |
|
recheck |
|
recheck |
| @@ -960,11 +960,46 @@ void MetaServerClientImpl::UpdateInodeAttrAsync( | |||
| UpdateInodeAsync(request, done); | |||
| } | |||
|
|
|||
| namespace { | |||
| UpdateInodeRequest BuildUpdateInodeRequest(const Inode& inode, | |||
There was a problem hiding this comment.
This function may can reuse BuileUpdateInodeAttrWithOutNlinkRequest after modify it.
- maybe the name may change to "BuileUpdateInodeWithOutNlinkRequest"
- change param 's3chunkinfo' to 'DataIndices'
After this the interface maybe more abstract and unified
curvefs/src/client/inode_wrapper.h
Outdated
| @@ -159,6 +164,12 @@ class InodeWrapper : public std::enable_shared_from_this<InodeWrapper> { | |||
| return inode_.length(); | |||
| } | |||
|
|
|||
| // Get inode's length. | |||
| // REQUIRES: |mtx_| is held | |||
| uint64_t GetLengthLocked() const { | |||
There was a problem hiding this comment.
The name of Locked and Unlocked is mismatch with existing code. You can think how to deal with.
There was a problem hiding this comment.
It's deleted.
I see this problem, and this is because we don't have a coding style, even though we claim that our code is adhering to google c++ coding style, but is not, we broke many rules.
About this problem, the suffix Locked means the caller must hold the lock. This convention is adopted by many famous open source projects, like rocksdb example, chromium example.
| @@ -141,6 +109,10 @@ InodeCacheManagerImpl::GetInode(uint64_t inodeId, | |||
| bool ok = iCache_->Get(inodeId, &out); | |||
| if (ok) { | |||
| curve::common::UniqueLock lgGuard = out->GetUniqueLock(); | |||
| if (out->GetType() == FsFileType::TYPE_FILE) { | |||
There was a problem hiding this comment.
If volumeExtentList may becomes too large like s3chunkinfo in inode, You can add a TODO here and refresh later.
There was a problem hiding this comment.
It's problem but they have different logic, random write will not generate new extent lists, and volume doesn't have compaction now.
| @@ -154,7 +126,10 @@ InodeCacheManagerImpl::GetInode(uint64_t inodeId, | |||
| option_.refreshDataIntervalSec); | |||
|
|
|||
| // refresh data | |||
| REFRESH_DATA_REMOTE(out, streaming); | |||
| { | |||
| curve::common::UniqueLock lgGuard = out->GetUniqueLock(); | |||
There was a problem hiding this comment.
The lock here maybe useless, because this inodeWapper just created and not be used outside.
curvefs/proto/metaserver.proto
Outdated
| @@ -294,6 +294,7 @@ message UpdateInodeRequest { | |||
| map<string, string> xattr = 20; | |||
| repeated uint64 parent = 21; | |||
| map<uint64, S3ChunkInfoList> s3ChunkInfoAdd = 22; | |||
| optional VolumeExtentList extents = 23; | |||
|
recheck |
1 similar comment
|
recheck |
Signed-off-by: Hanqing Wu <[email protected]>
Signed-off-by: Hanqing Wu <[email protected]>
|
recheck |
Signed-off-by: Hanqing Wu [email protected]
What problem does this PR solve?
Issue Number: #1779
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List