curvefs/metaserver: fix trash bugs by wuhongsong · Pull Request #2948 · opencurve/curve

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

+     * @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

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


Ditto RemoveDeletedInode

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

    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

+        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

-}
-
-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

    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

    }
+
+    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

-
 namespace storage {

+#define DELETEPREFIXINKV "deleting:"


this is needn't

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

    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


 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

    {
        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

    }

-    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

    }
    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

-            << ", 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

+  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

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


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

+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

+    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

                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

    }
 }

+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

Conversation

wuhongsong commented Dec 6, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

wuhongsong commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

wuhongsong commented Dec 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!