-
Notifications
You must be signed in to change notification settings - Fork 522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
curvefs: support space deallocate for curvebs volume as backend #2313
Conversation
be3376e
to
60cbecd
Compare
d9c6864
to
8a06440
Compare
@@ -50,8 +49,13 @@ FsInfoWrapper::FsInfoWrapper(const ::curvefs::mds::CreateFsRequest* request, | |||
fsInfo.set_enablesumindir(request->enablesumindir()); | |||
fsInfo.set_txsequence(0); | |||
fsInfo.set_txowner(""); | |||
// TODO(@lixiaocui1): Currently, curveadm does not support setting recycling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be supported.
BTW: is there a default value for recyleTimeHour
if user not specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recycleTimeHour is optional
in message FsInfo
. it doesn't have a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recycleTimeHour is
optional
inmessage FsInfo
. it doesn't have a default value.
Okay~ i will give a default value (1 hours) in curveadm.
if (wrapper.GetFsType() == FSType::TYPE_VOLUME) { | ||
auto volumeSpace = spaceManager_->GetVolumeSpace(wrapper.GetFsId()); | ||
if (volumeSpace == nullptr) { | ||
LOG(ERROR) << "handle fs mount point timeout fail, get volume " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be more readable:
LOG(ERROR) << "handle fs mount point timeout fail, get volume "
<< "space fail, fsName = " << fsName
<< ", mountpoint = " << mountpoint.ShortDebugString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the default fomat result.
if (ret != space::SpaceOk) { | ||
LOG(ERROR) | ||
<< "handle fs mount point timeout fail,release block groups " | ||
"fail, fsName = " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -156,8 +172,10 @@ SpaceErrCode VolumeSpace::AllocateBlockGroups( | |||
return err; | |||
} | |||
|
|||
for (auto& group : *blockGroups) { | |||
for (auto &group : *blockGroups) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its better to use const auto&
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
@@ -317,6 +352,8 @@ SpaceErrCode VolumeSpace::ReleaseBlockGroups( | |||
LockGuard lk(mtx_); | |||
|
|||
for (auto& group : blockGroups) { | |||
VLOG(3) << "VolumeSpace need release block group:" | |||
<< group.DebugString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated. fix
@@ -267,6 +286,22 @@ SpaceErrCode VolumeSpace::AcquireBlockGroup(uint64_t blockGroupOffset, | |||
SpaceErrCode VolumeSpace::AcquireBlockGroupInternal(uint64_t blockGroupOffset, | |||
const std::string& owner, | |||
BlockGroup* group) { | |||
if (owner.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be more readable:
if (owner.empty()) {
auto it = deallocatingGroups_.find(blockGroupOffset);
if (it == deallocatingGroups_.end()) {
return SpaceErrNotFound;
}
*group = it->second;
VLOG(6) << "VolumeSpace recieve acquire blockgroup="
<< group->DebugString()
<< " rquest from metaserver, current block group is under "
"deallocating";
return SpaceOk;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. fix.
@@ -571,5 +620,207 @@ MetaStatusCode InodeStorage::GetVolumeExtentByOffset(uint32_t fsId, | |||
return MetaStatusCode::STORAGE_INTERNAL_ERROR; | |||
} | |||
|
|||
MetaStatusCode InodeStorage::GeAllBlockGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: GetAllBlockGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
curvefs/src/metaserver/partition.cpp
Outdated
} | ||
} | ||
|
||
#define PRECHECK(fsId, inodeId) \ | ||
do { \ | ||
if (!IsInodeBelongs((fsId), (inodeId))) { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation for \
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
curvefs/src/metaserver/partition.cpp
Outdated
} \ | ||
} while (0) | ||
|
||
#define PRECHECKFSID(fsId) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe PRECHECK_FSID
is more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
|
||
// key is volume offset | ||
DeallocatableBlockGroupMap increase; | ||
while (iter->Valid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
// do something
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
if (it == spaceManagers_.end()) { | ||
if (!GenerateSpaceManager(fsId)) { | ||
return nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it return spaceManagers_[fsId]
if GenerateSpaceManager
return true
even if the item not exist in the spaceManagers_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. GenerateSpaceManager
will insert new generated manager to spaceManagers_
.
std::string Key4DeallocatableBlockGroup::SerializeToString() const { | ||
return absl::StrCat(keyType_, kDelimiter, fsId, kDelimiter, volumeOffset); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
If willing, please requested review from me again after all problem solved and all suggestion has been handled :) |
a2bfc88
to
b6742eb
Compare
okok |
666ff36
to
40bfa91
Compare
cicheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
8ae2b6a
to
64daf02
Compare
cicheck |
<< MetaStatusCode_Name(st) | ||
<< ", inodeid: " << inode_->GetInodeId(); | ||
inode_->MarkInodeError(); | ||
if (inode_ != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inode may be evicted by mistake, here is a defense.
evict an inode won't cause inode_ to become a nullptr, the reason might be UpdateVolumeExtentClosure was created from a null inode.
} | ||
|
||
if (item.has_increase()) { | ||
if (!out.IsInitialized()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!s.ok() && !s.IsNotFound()
meansstatus wrong
andinode is found
At this point, we either found a DeallocatableBlockGroup or not found and without any error.
And if we found one, then out
must be initialized, otherwise out
is not initialized if it isn't found.
If this is true, using s.IsNotFound()
will be more explicit, because people may not know why we test out is initialized here.
MetaStatusCode Increase(Transaction txn, uint32_t fsId, | ||
const IncreaseDeallocatableBlockGroup &increase, | ||
DeallocatableBlockGroup *out); | ||
|
||
MetaStatusCode Decrease(const DecreaseDeallocatableBlockGroup &decrease, | ||
DeallocatableBlockGroup *out); | ||
|
||
MetaStatusCode Mark(const MarkDeallocatableBlockGroup &mark, | ||
DeallocatableBlockGroup *out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these functions are all
private
, which are used for processing corresponding to different fields inmessage DeallocatableBlockGroup
.
Although they're private, member functions should access or modify data members not just
function parameters. And another good reason to put them outside of this class is it'll be more test friendly.
<< ", blockGroupOffset:" << blockGroupOffset | ||
<< ", to sliceList:" << sliceList.DebugString() | ||
<< " failed"; | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's still leaked in this case, BTW, you don't have to allocate closure on heap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
extents->begin(), extents->end(), | ||
[&](const VolumeExtent &extent) { | ||
bool res = (extent.volumeoffset() >= blockGroupOffset) && | ||
(extent.volumeoffset() < boundary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of the existence of 4k headers, the length of the extent will not cross the block group, so there is no need to add length to judge.
consider add a comment to explain this implicit condition
curvefs/proto/common.proto
Outdated
@@ -31,6 +31,13 @@ enum BitmapLocation { | |||
AtEnd = 2; | |||
} | |||
|
|||
message EmptyMsg {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: ilixiaocui <[email protected]>
64daf02
to
fcdb00e
Compare
cicheck |
What problem does this PR solve?
Issue Number: #2160
Problem Summary: #2294
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List