Skip to content
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

mds: check file in-use when Delete/Rename/ChangeOwner #153

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

wu-hanqing
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

Client will not change the file metadata after it gets it from MDS,
Recover creates a new file and rename it to orginal file, so the original file's metadata will be changed.
Therefore, when the file is being used, the recover operation will lead to inconsistency.

What is changed and how it works?

What's Changed:

MDS check whether file is being used when Rename/Delete/ChangeOwner.
This check based on file record, and if file is being used, client RefreshSession will add a file record of current file.

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

@@ -100,10 +100,6 @@ void FileInstance::UnInitialize() {
// 因为如果后台集群重新部署了,需要通过lease续约来获取当前session状态
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这些注释是和下面的leaseExecutor相关的吗?如果是的话删掉吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -173,9 +169,13 @@ int FileInstance::Close() {
return 0;
}

if (leaseExecutor_ != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么把这个从UnInitialize改到了Close里?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client 之前是close,然后 uninit。在unint里面停refresh session。
mds在close里面,清理filerecord。refresh session里面如果没有file record,就添加一个。
所以有可能close之后,还有refresh session请求导致新加了一个file record。

@@ -104,10 +104,20 @@ bool LeaseExecutor::RefreshLease() {
}

if (response.status == LeaseRefreshResult::Status::OK) {
if (iomanager_->InodeId() != response.finfo.id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

什么情况下iNode id会变化?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

正常情况下不会。但是recover的情况下,只有filename是一样的,inodeid会变。

if (timePass < 10 * microseconds(expiredUs)) {
LOG(INFO) << "snapshot is not allowed now, fileName = " << fileName
<< ", time pass = " << timePass.count();
if (!IsStartEnoughTime(10)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个10是硬编码?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的,这里之前的逻辑就是等待10倍的expiredUs时间

LOG(INFO) << "snapshot is not allowed now, fileName = " << fileName
<< ", time pass = " << timePass.count();
if (!IsStartEnoughTime(10)) {
LOG(INFO) << "snapshot is not allowed now, fileName = " << fileName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

快照没判断挂载状态吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

快照不用判断挂载状态。

@@ -718,6 +711,18 @@ StatusCode CurveFS::CheckFileCanChange(const std::string &fileName,
return StatusCode::kDeleteFileBeingCloned;
}

if (!IsStartEnoughTime(1)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1、这里返回kNotSupported之后,会造成相关接口返回失败?这些接口返回失败之后有重试吗? 快照的部分是会重试直到允许打快照为止的。如果因此返回上层失败,对用户有啥影响?
2、这个CheckFileCanChange接口是很多接口所共有的,是否这些接口都需要这个等1s
3、这里最好加上注释,这个等1s是在等啥?这种特殊的逻辑,没有上下文,会对后续维护者造成困扰。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CheckFileCanChange这个接口,目前就3个地方回调用,DeleteFile/RenameFile/ChangeOwner。

@@ -338,7 +338,9 @@ int CurveFsClientImpl::RenameCloneFile(
};
RetryCondition condition = [] (int ret) {
return ret != LIBCURVE_ERROR::OK &&
ret != -LIBCURVE_ERROR::NOTEXIST;
ret != -LIBCURVE_ERROR::NOTEXIST &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT_SUPPORT和FILE_OCCUPIED是否有在其他地方返回这两个错误码?那些场景下是否也需要重试?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@xu-chaojie xu-chaojie merged commit 4747da1 into opencurve:master Jan 18, 2021
@wu-hanqing wu-hanqing deleted the fix-nbd-recover branch January 20, 2021 02:33
ilixiaocui pushed a commit to ilixiaocui/curve that referenced this pull request Feb 6, 2023
This commit adds Rook project ideas for GSOC.

Signed-off-by: Ashish Ranjan <ashishranjan738@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants