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

curvefs/metaserver: fix trash bugs #2942

Closed
wants to merge 1 commit into from

Conversation

wuhongsong
Copy link
Contributor

What problem does this PR solve?

Issue Number: #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

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

@wuhongsong wuhongsong force-pushed the fix_trash.12.01 branch 2 times, most recently from 9f5ead3 to 10e5d20 Compare December 1, 2023 15:05
@wuhongsong
Copy link
Contributor Author

cicheck

@wuhongsong
Copy link
Contributor Author

cicheck

@wuhongsong wuhongsong closed this Dec 2, 2023
@wuhongsong wuhongsong reopened this Dec 2, 2023
@wuhongsong wuhongsong closed this Dec 2, 2023
@wuhongsong wuhongsong reopened this Dec 2, 2023
@wuhongsong
Copy link
Contributor Author

cicheck

1 similar comment
@wuhongsong
Copy link
Contributor Author

cicheck

@wuhongsong wuhongsong closed this Dec 2, 2023
@wuhongsong wuhongsong reopened this Dec 2, 2023
@wuhongsong wuhongsong force-pushed the fix_trash.12.01 branch 2 times, most recently from 8b7ef4b to 2c618ca Compare December 2, 2023 05:23
@wuhongsong
Copy link
Contributor Author

cicheck

@wuhongsong
Copy link
Contributor Author

cicheck

@@ -312,7 +312,7 @@ MetaStatusCode InodeManager::DeleteInode(uint32_t fsId, uint64_t inodeId,
MetaStatusCode InodeManager::UpdateInode(const UpdateInodeRequest& request,
int64_t logIndex) {
CHECK_APPLIED();
VLOG(9) << "update inode, fsid: " << request.fsid()
VLOG(0) << "update inode, fsid: " << request.fsid()
Copy link
Contributor

Choose a reason for hiding this comment

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

VLOG(0) is too often

--(*type2InodeNum_)[old.type()];
}

VLOG(0) << "UpdateInode success, " << request.ShortDebugString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@@ -187,6 +198,11 @@ class InodeStorage {
MetaStatusCode GetAllBlockGroup(
std::vector<DeallocatableBlockGroup>* deallocatableBlockGroupVec);

/*
std::shared_ptr<KVStorage> GetKVStorage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove it if needn't

@@ -92,6 +95,13 @@ class RocksDBStorage : public KVStorage, public StorageTransaction {
const std::string& key,
const ValueType& value) override;

Status HSetDeleting(const std::string& name,
Copy link
Contributor

Choose a reason for hiding this comment

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

The interfaces added is unnecessary, rocksdb_storage only need the most basic interface, and you should deal the logic upper layer

MetaStatusCode InodeStorage::UpdateDeletingKey(
const Inode& inode, int64_t logIndex) {
WriteLockGuard lg(rwLock_);
Key4Inode key(inode.fsid(), inode.inodeid());
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can add a new struct like Key4DeletedInode in which you can add deleted prefix before inode key

@@ -43,6 +43,7 @@
#include "curvefs/src/metaserver/storage/utils.h"
#include "src/common/concurrent/rw_lock.h"

#define DELETING_PREFIX "deleting_"
Copy link
Contributor

Choose a reason for hiding this comment

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

The raw string prefix is inappropriate, you can add a new key type in

enum KEY_TYPE : unsigned char {

LOG(ERROR) << "Begin transaction failed";
return MetaStatusCode::STORAGE_INTERNAL_ERROR;
}
auto rc = txn->HSetDeleting(table4Inode_, skey , inode);
Copy link
Contributor

Choose a reason for hiding this comment

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

The dtime as the value set to rocksdb is enough

@wuhongsong wuhongsong closed this Dec 13, 2023
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.

2 participants