curvefs/metaserver: fix trash bugs #2948

wuhongsong · 2023-12-06T11:04:24Z

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 · 2023-12-06T11:18:26Z

cicheck

SeanHai · 2023-12-07T03:15:03Z

curvefs/src/metaserver/inode_storage.h

+     * @param[in] logIndex: the index of raft log
+     * @return
+     */
+    MetaStatusCode UpdateDeletingKey(const Inode& inode, int64_t logIndex);


UpdateDeletingKey to AddDeletedInode maybe more clearer

SeanHai · 2023-12-07T03:15:22Z

curvefs/src/metaserver/inode_storage.h

+     */
+    MetaStatusCode UpdateDeletingKey(const Inode& inode, int64_t logIndex);
+
+    MetaStatusCode ClearDelKey(const Key4Inode& key);


Ditto RemoveDeletedInode

SeanHai · 2023-12-07T03:18:49Z

curvefs/src/metaserver/metastore.cpp

@@ -866,5 +869,27 @@ bool MetaStoreImpl::InitStorage() {
    return kvStorage_->Open();
 }

+void MetaStoreImpl::LoadDeletedInodes() {


LoadDeletedInodes is the responsibility of InodeManager, Each partition has one InodeManager and InodeStorage, you can load deleted inodes with InodeManager and add to trash_ in InodeManager by pass and the deleted inode loaded are all belong to this partition and trash.

SeanHai · 2023-12-07T03:21:35Z

curvefs/src/metaserver/inode_storage.cpp

+        LOG(ERROR) << "Begin transaction failed";
+        return MetaStatusCode::STORAGE_INTERNAL_ERROR;
+    }
+    auto rc = txn->HSet(table4DelInode_, skey , inode);


The value does not need to be the entire inode, dtime is enough

SeanHai · 2023-12-07T03:24:05Z

curvefs/src/metaserver/trash.cpp

-}
-
-void TrashImpl::Add(uint64_t inodeId, uint64_t dtime) {
+void TrashImpl::Add(uint64_t inodeId, uint64_t dtime, bool deleted) {


Why deleted needed?

tell reboot or not

SeanHai · 2023-12-07T03:25:12Z

curvefs/src/metaserver/trash.cpp

    LockGuard lg(itemsMutex_);
-    trashItems_.erase(inodeId);
+    auto it = std::find(trashItems_.begin(), trashItems_.end(), item);
+    if (it != trashItems_.end()) {


The remove of rocksdb data can do here also.

SeanHai · 2023-12-07T03:36:47Z

curvefs/src/metaserver/inode_manager.cpp

@@ -446,6 +441,13 @@ MetaStatusCode InodeManager::UpdateInode(const UpdateInodeRequest& request,
            return MetaStatusCode::STORAGE_INTERNAL_ERROR;
        }
    }
+
+    if (needAddTrash) {
+        inodeStorage_->UpdateDeletingKey(old, logIndex);


add and delete op all deal in trash is better.

wuhongsong · 2023-12-12T07:25:24Z

cicheck

wuhongsong · 2023-12-12T08:12:54Z

cicheck

SeanHai · 2023-12-12T08:10:03Z

curvefs/src/metaserver/storage/converter.h

 namespace storage {

+#define DELETEPREFIXINKV "deleting:"


this is needn't

SeanHai · 2023-12-12T08:10:13Z

curvefs/src/metaserver/storage/converter.cpp

@@ -123,6 +129,13 @@ std::string NameGenerator::Format(KEY_TYPE type, uint32_t partitionId) {
    return absl::StrCat(type, kDelimiter, absl::string_view(buf, sizeof(buf)));
 }

+std::string NameGenerator::FormatDel(KEY_TYPE type, uint32_t partitionId) {


SeanHai · 2023-12-12T08:11:19Z

curvefs/src/metaserver/storage/converter.cpp

@@ -54,6 +55,7 @@ static bool CompareType(const std::string& str, KEY_TYPE keyType) {

 NameGenerator::NameGenerator(uint32_t partitionId)
    : tableName4Inode_(Format(kTypeInode, partitionId)),
+      tableName4DelInode_(FormatDel(kTypeInode, partitionId)),


Add a new type like 'kTypeDelInode'

wuhongsong · 2023-12-12T08:24:57Z

cicheck

wuhongsong · 2023-12-12T09:16:37Z

cicheck

wuhongsong · 2023-12-12T09:19:31Z

cicheck

wuhongsong · 2023-12-12T12:21:57Z

cicheck

wuhongsong · 2023-12-12T13:35:33Z

cicheck

wuhongsong · 2023-12-12T14:23:14Z

cicheck

wuhongsong · 2023-12-12T15:02:41Z

cicheck

SeanHai · 2023-12-13T01:53:17Z

curvefs/src/metaserver/trash.cpp

    {
        LockGuard lgItems(itemsMutex_);
-        trashItems_.insert(temp.begin(), temp.end());
+        trashItems_.swap(temp);


Can not swap, the trashItem already has some new item

SeanHai · 2023-12-13T01:54:33Z

curvefs/src/metaserver/trash.cpp

@@ -100,7 +109,7 @@ void TrashImpl::ScanTrash() {
        return;
    }

-    std::unordered_map<uint64_t, uint64_t> temp;
+    std::map<uint64_t, uint64_t> temp;


unordered_map is space friendly here

SeanHai · 2023-12-13T01:57:45Z

curvefs/src/metaserver/trash.cpp

 }

 void TrashImpl::Remove(uint64_t inodeId) {
    if (isStop_) {
        return;
    }
    LockGuard lg(itemsMutex_);
-    trashItems_.erase(inodeId);
+    auto it = trashItems_.find(inodeId);


don't need find here and can erase directly

SeanHai · 2023-12-13T01:58:36Z

curvefs/src/metaserver/trash.cpp

-            << ", partitionId = " << partitionId_
+    trashItems_.emplace(inodeId, dtime);
+
+    VLOG(6) << "Add trash item success"


Don't remove fsId and partitionId, inodeId can be same in different fs

SeanHai · 2023-12-13T01:59:13Z

curvefs/src/metaserver/storage/rocksdb_storage.cpp

+  std::map<std::string, uint64_t>* item, const std::string prefix) {
+    std::string sprefix = absl::StrCat("0", ":", prefix);
+    VLOG(3) << "load deleted inodes from: " << options_.dataDir
+      << ", " << sprefix << ", " << prefix;


code alignment

SeanHai · 2023-12-13T02:00:26Z

curvefs/src/metaserver/storage/rocksdb_storage.cpp

+    }
+    delete it;
+    VLOG(3) << "load deleted inodes end, size is: " << item->size()
+    << ", " << counts << ", " << options_.dataDir;


SeanHai · 2023-12-13T02:01:11Z

curvefs/src/metaserver/inode_storage.cpp

+void InodeStorage::LoadDeletedInodes(std::map<std::string, uint64_t> * inodes) {
+    VLOG(6) << "load deleted key start with: " << table4DelInode_;
+    kvStorage_->GetPrefix(inodes, table4DelInode_);
+    VLOG(6) << "oad deleted over";


SeanHai · 2023-12-13T02:01:44Z

curvefs/src/metaserver/inode_manager.cpp

+    inodeStorage_->LoadDeletedInodes(&items);
+    VLOG(3) << "build trash items size: " << items.size();
+    std::vector<std::string> names;
+    for (auto iter : items) {


for (auto& iter : items)

SeanHai · 2023-12-13T02:03:39Z

curvefs/src/metaserver/trash.cpp

@@ -116,24 +125,30 @@ void TrashImpl::ScanTrash() {
                LOG(ERROR) << "DeleteInodeAndData fail, fsId = " << fsId_
                           << ", inodeId = " << it->first
                           << ", ret = " << MetaStatusCode_Name(ret);
+                RemoveDeletedInode(it->first);


RemoveDeleteInode in func Remove

SeanHai · 2023-12-13T02:05:48Z

curvefs/src/metaserver/trash.cpp

    }
 }

+void TrashImpl::RemoveDeletedInode(uint64_t inodeId) {


Maybe void is not good. Some warning log may need if Remove fail.

wuhongsong · 2023-12-13T02:47:44Z

cicheck

wuhongsong · 2023-12-13T02:51:11Z

cicheck

YunhuiChen · 2023-12-13T03:03:52Z

cicheck

wuhongsong · 2023-12-13T03:10:14Z

cicheck

wuhongsong · 2023-12-13T03:41:35Z

cicheck

wuhongsong · 2023-12-13T04:44:13Z

cicheck

wuhongsong · 2023-12-13T05:01:00Z

cicheck

wuhongsong force-pushed the fix_trash_1205 branch 3 times, most recently from 1e01fc6 to f6cd9d5 Compare December 6, 2023 11:17

wuhongsong closed this Dec 6, 2023

wuhongsong reopened this Dec 6, 2023

wuhongsong force-pushed the fix_trash_1205 branch from f6cd9d5 to 914faf4 Compare December 6, 2023 13:13

wuhongsong closed this Dec 6, 2023

wuhongsong reopened this Dec 6, 2023

wuhongsong force-pushed the fix_trash_1205 branch from 914faf4 to 21bb6a3 Compare December 6, 2023 13:44

wuhongsong closed this Dec 6, 2023

wuhongsong reopened this Dec 6, 2023

SeanHai reviewed Dec 7, 2023

View reviewed changes

wuhongsong force-pushed the fix_trash_1205 branch from 21bb6a3 to 57748b1 Compare December 8, 2023 09:11

wuhongsong closed this Dec 8, 2023

wuhongsong reopened this Dec 8, 2023

wuhongsong closed this Dec 8, 2023

wuhongsong reopened this Dec 8, 2023

wuhongsong force-pushed the fix_trash_1205 branch 2 times, most recently from 1eb1450 to d635cf6 Compare December 8, 2023 13:23

wuhongsong closed this Dec 8, 2023

wuhongsong reopened this Dec 8, 2023

wuhongsong force-pushed the fix_trash_1205 branch 2 times, most recently from 610f6ab to 288bc9c Compare December 8, 2023 14:32

wuhongsong closed this Dec 8, 2023

wuhongsong reopened this Dec 8, 2023

wuhongsong closed this Dec 9, 2023

wuhongsong reopened this Dec 9, 2023

wuhongsong force-pushed the fix_trash_1205 branch from 288bc9c to 2700f65 Compare December 9, 2023 04:15

wuhongsong force-pushed the fix_trash_1205 branch from 0304c53 to 3b5db6a Compare December 12, 2023 08:12

wuhongsong force-pushed the fix_trash_1205 branch from 3b5db6a to e304ccd Compare December 12, 2023 08:22

SeanHai reviewed Dec 12, 2023

View reviewed changes

wuhongsong force-pushed the fix_trash_1205 branch from e304ccd to cee94ad Compare December 12, 2023 09:15

wuhongsong force-pushed the fix_trash_1205 branch from cee94ad to b36a0aa Compare December 12, 2023 09:18

wuhongsong force-pushed the fix_trash_1205 branch from b36a0aa to bf8a57d Compare December 12, 2023 12:21

wuhongsong force-pushed the fix_trash_1205 branch from bf8a57d to e56d6a6 Compare December 12, 2023 14:59

SeanHai reviewed Dec 13, 2023

View reviewed changes

wuhongsong force-pushed the fix_trash_1205 branch from e56d6a6 to 941d39b Compare December 13, 2023 02:45

wuhongsong force-pushed the fix_trash_1205 branch from 941d39b to 54430cf Compare December 13, 2023 03:40

curvefs/metaserver: fix trash bugs

d86bba3

wuhongsong force-pushed the fix_trash_1205 branch from 54430cf to d86bba3 Compare December 13, 2023 05:00

SeanHai approved these changes Dec 13, 2023

View reviewed changes

wuhongsong merged commit 754b19e into opencurve:master Dec 13, 2023
4 checks passed

curvefs/metaserver: fix trash bugs #2948

curvefs/metaserver: fix trash bugs #2948

Conversation

wuhongsong commented Dec 6, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

wuhongsong commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

wuhongsong commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuhongsong commented Dec 13, 2023

wuhongsong commented Dec 13, 2023

YunhuiChen commented Dec 13, 2023

wuhongsong commented Dec 13, 2023

wuhongsong commented Dec 13, 2023

wuhongsong commented Dec 13, 2023

wuhongsong commented Dec 13, 2023