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

Atomize RenameFile in KeyManagedEncryptedEnv #222

Merged

Conversation

hunterlxt
Copy link
Member

@hunterlxt hunterlxt commented Dec 16, 2020

fix bug: tikv/tikv#9115

Summary: we need to update encryption metadata via encryption::DataKeyManager, which cannot combine with the actual file operation into one atomic operation. In RenameFile, when the src_file has been removed, power is off, then we may lost the file info of src_file next restart.

Signed-off-by: Xintao hunterlxt@live.com

@hunterlxt hunterlxt force-pushed the xt/handle-encryption-logic-correctly branch from b286c61 to 51fdfa9 Compare December 16, 2020 08:56
@hunterlxt
Copy link
Member Author

@tabokie @Connor1996 PTAL

Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Looks good, but I want to make sure future implementation won't embed critical logic inside Rename method, e.g. file-specific state transfer. Maybe add a comment or simply deprecate the function in KeyManager?

// Ignore error
key_manager_->RenameFile(dst_fname, src_fname);
key_manager_->DeleteFile(dst_fname);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do me a favor, update the code like this. also update the same logic for ::LinkFile()

Status delete_status __attribute__((__unused__)) = key_manager_->DeleteFile(dst_name);
assert(delete_stauts.ok());

encryption/encryption.cc Outdated Show resolved Hide resolved
@yiwu-arbug
Copy link
Collaborator

Looks good, but I want to make sure future implementation won't embed critical logic inside Rename method, e.g. file-specific state transfer. Maybe add a comment or simply deprecate the function in KeyManager?

Great suggestion. Let's remove KeyManager::RenameFile in encryption.h.

@yiwu-arbug
Copy link
Collaborator

revert redundant formatter change?

@hunterlxt hunterlxt force-pushed the xt/handle-encryption-logic-correctly branch 3 times, most recently from 3da8411 to 6cdcaa9 Compare December 17, 2020 08:16
Signed-off-by: Xintao <hunterlxt@live.com>
@hunterlxt hunterlxt force-pushed the xt/handle-encryption-logic-correctly branch from eb6dd19 to 0da0b57 Compare December 18, 2020 05:01
Copy link
Collaborator

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

LGTM

@hunterlxt hunterlxt merged commit 93e89a5 into tikv:6.4.tikv Dec 18, 2020
@hunterlxt hunterlxt deleted the xt/handle-encryption-logic-correctly branch December 18, 2020 07:17
yiwu-arbug pushed a commit that referenced this pull request Jan 27, 2021
Cherry-picking #224.

#222 used `LinkFile` instead of `RenameFile` api of key manager. But `LinkFile` needs check the dst file information, in `RenameFile` logic, we don't care about that. So just skip encryption for current file.

Signed-off-by: Xintao <hunterlxt@live.com>
yiwu-arbug pushed a commit that referenced this pull request Jul 9, 2021
* adjust logic in KeyManagedEncryptedEnv::RenameFile to avoid poweroff

Signed-off-by: Xintao <hunterlxt@live.com>
@tabokie tabokie mentioned this pull request May 9, 2022
39 tasks
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request May 12, 2022
* adjust logic in KeyManagedEncryptedEnv::RenameFile to avoid poweroff

Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie pushed a commit that referenced this pull request May 12, 2022
* adjust logic in KeyManagedEncryptedEnv::RenameFile to avoid poweroff

Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie pushed a commit that referenced this pull request May 25, 2022
* adjust logic in KeyManagedEncryptedEnv::RenameFile to avoid poweroff

Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.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.

4 participants