curvefs: fix batch get inodeattr when readdir

Signed-off-by: wanghai01 <[email protected]>

curvefs/src/client/fuse_client.cpp

@@ -881,7 +881,7 @@ CURVEFS_ERROR FuseClient::FuseOpReadDirPlus(fuse_req_t req, fuse_ino_t ino,
                 inodeIds.emplace(dentry.inodeid());
             }
             VLOG(3) << "batch get inode size = " << inodeIds.size();
-            ret = inodeManager_->BatchGetInodeAttrAsync(ino, inodeIds,
+            ret = inodeManager_->BatchGetInodeAttrAsync(ino, &inodeIds,
                                                         &inodeAttrMap);
             if (ret != CURVEFS_ERROR::OK) {
                 LOG(ERROR) << "BatchGetInodeAttr failed when FuseOpReadDir"

curvefs/src/client/inode_cache_manager.cpp

@@ -206,16 +206,16 @@ CURVEFS_ERROR InodeCacheManagerImpl::GetInodeAttr(uint64_t inodeId,
 
 CURVEFS_ERROR InodeCacheManagerImpl::BatchGetInodeAttr(
     std::set<uint64_t> *inodeIds,
-    std::list<InodeAttr> *attr) {
-    // get some inode in icache
+    std::list<InodeAttr> *attrs) {
+    // get some inode attr in icache
     for (auto iter = inodeIds->begin(); iter != inodeIds->end();) {
         std::shared_ptr<InodeWrapper> inodeWrapper;
         NameLockGuard lock(nameLock_, std::to_string(*iter));
         bool ok = iCache_->Get(*iter, &inodeWrapper);
         if (ok) {
             InodeAttr tmpAttr;
             inodeWrapper->GetInodeAttr(&tmpAttr);
-            attr->emplace_back(tmpAttr);
+            attrs->emplace_back(tmpAttr);
             iter = inodeIds->erase(iter);
         } else {
             ++iter;
@@ -226,7 +226,8 @@ CURVEFS_ERROR InodeCacheManagerImpl::BatchGetInodeAttr(
         return CURVEFS_ERROR::OK;
     }
 
-    MetaStatusCode ret = metaClient_->BatchGetInodeAttr(fsId_, *inodeIds, attr);
+    MetaStatusCode ret = metaClient_->BatchGetInodeAttr(fsId_, *inodeIds,
+        attrs);
     if (MetaStatusCode::OK != ret) {
         LOG(ERROR) << "metaClient BatchGetInodeAttr failed, MetaStatusCode = "
                    << ret << ", MetaStatusCode_Name = "
@@ -237,21 +238,37 @@ CURVEFS_ERROR InodeCacheManagerImpl::BatchGetInodeAttr(
 
 CURVEFS_ERROR InodeCacheManagerImpl::BatchGetInodeAttrAsync(
     uint64_t parentId,
-    const std::set<uint64_t> &inodeIds,
+    std::set<uint64_t> *inodeIds,
     std::map<uint64_t, InodeAttr> *attrs) {
-    if (inodeIds.empty()) {
-        return CURVEFS_ERROR::OK;
+    NameLockGuard lg(asyncNameLock_, std::to_string(parentId));
+    std::map<uint64_t, InodeAttr> cachedAttr;
+    bool ok  = iAttrCache_->Get(parentId, &cachedAttr);
+
+    // get some inode attr in icache
+    for (auto iter = inodeIds->begin(); iter != inodeIds->end();) {
+        std::shared_ptr<InodeWrapper> inodeWrapper;
+        NameLockGuard lock(nameLock_, std::to_string(*iter));
+        bool ok = iCache_->Get(*iter, &inodeWrapper);
+        if (ok) {
+            InodeAttr tmpAttr;
+            inodeWrapper->GetInodeAttr(&tmpAttr);
+            attrs->emplace(*iter, tmpAttr);
+            iter = inodeIds->erase(iter);
+        } else if (ok && cachedAttr.find(*iter) != cachedAttr.end()) {
+            attrs->emplace(*iter, cachedAttr[*iter]);
+            iter = inodeIds->erase(iter);
+        } else {
+            ++iter;
+        }
     }
 
-    NameLockGuard lg(asyncNameLock_, std::to_string(parentId));
-    bool ok  = iAttrCache_->Get(parentId, attrs);
-    if (ok) {
+    if (inodeIds->empty()) {
         return CURVEFS_ERROR::OK;
     }
 
     // split inodeIds by partitionId and batch limit
     std::vector<std::vector<uint64_t>> inodeGroups;
-    if (!metaClient_->SplitRequestInodes(fsId_, inodeIds, &inodeGroups)) {
+    if (!metaClient_->SplitRequestInodes(fsId_, *inodeIds, &inodeGroups)) {
         return CURVEFS_ERROR::NOTEXIST;
     }
 
@@ -461,6 +478,11 @@ void InodeCacheManagerImpl::TrimIcache(uint64_t trimSize) {
                 iCache_->Remove(inodeId);
             }
             trimSize--;
+            // remove the attr of the inode in iattrcache
+            auto parents = inodeWrapper->GetParentLocked();
+            for (uint64_t parent : parents) {
+                iAttrCache_->Remove(parent, inodeId);
+            }
         } else {
             LOG(ERROR) << "icache size " << iCache_->Size();
             assert(0);

curvefs/src/client/inode_cache_manager.h

@@ -74,7 +74,7 @@ class InodeAttrCache {
         curve::common::LockGuard lg(iAttrCacheMutex_);
         auto iter = iAttrCache_.find(parentId);
         if (iter != iAttrCache_.end()) {
-            *imap = iter->second;
+            imap->insert(iter->second.begin(), iter->second.end());
             return true;
         }
         return false;
@@ -102,6 +102,14 @@ class InodeAttrCache {
                 << size << ", after = " << iAttrCache_.size();
     }
 
+    void Remove(uint64_t parentId, uint64_t inodeId) {
+        curve::common::LockGuard lg(iAttrCacheMutex_);
+        auto iter = iAttrCache_.find(parentId);
+        if (iter != iAttrCache_.end()) {
+            iter->second.erase(inodeId);
+        }
+    }
+
  private:
     // inodeAttr cache; <parentId, <inodeId, inodeAttr>>
     std::map<uint64_t, std::map<uint64_t, InodeAttr>> iAttrCache_;
@@ -140,7 +148,7 @@ class InodeCacheManager {
 
     virtual CURVEFS_ERROR BatchGetInodeAttrAsync(
         uint64_t parentId,
-        const std::set<uint64_t> &inodeIds,
+        std::set<uint64_t> *inodeIds,
         std::map<uint64_t, InodeAttr> *attrs) = 0;
 
     virtual CURVEFS_ERROR BatchGetXAttr(std::set<uint64_t> *inodeIds,
@@ -233,7 +241,7 @@ class InodeCacheManagerImpl : public InodeCacheManager,
         std::list<InodeAttr> *attrs) override;
 
     CURVEFS_ERROR BatchGetInodeAttrAsync(uint64_t parentId,
-        const std::set<uint64_t> &inodeIds,
+        std::set<uint64_t> *inodeIds,
         std::map<uint64_t, InodeAttr> *attrs = nullptr) override;
 
     CURVEFS_ERROR BatchGetXAttr(std::set<uint64_t> *inodeIds,

curvefs/src/client/inode_wrapper.h

@@ -203,6 +203,10 @@ class InodeWrapper : public std::enable_shared_from_this<InodeWrapper> {
         return curve::common::UniqueLock(mtx_);
     }
 
+    const google::protobuf::RepeatedField<uint64_t>& GetParentLocked() {
+        return inode_.parent();
+    }
+
     CURVEFS_ERROR UpdateParent(uint64_t oldParent, uint64_t newParent);
 
     // dir will not update parent

curvefs/test/client/mock_inode_cache_manager.h

@@ -62,7 +62,7 @@ class MockInodeCacheManager : public InodeCacheManager {
         std::set<uint64_t> *inodeIds, std::list<InodeAttr> *attrs));
 
     MOCK_METHOD3(BatchGetInodeAttrAsync,
-        CURVEFS_ERROR(uint64_t parentId, const std::set<uint64_t> &inodeIds,
+        CURVEFS_ERROR(uint64_t parentId, std::set<uint64_t> *inodeIds,
         std::map<uint64_t, InodeAttr> *attrs));
 
     MOCK_METHOD2(BatchGetXAttr, CURVEFS_ERROR(

curvefs/test/client/test_inode_cache_manager.cpp

@@ -20,12 +20,14 @@
  * Author: xuchaojie
  */
 
+#include <gmock/gmock-spec-builders.h>
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 #include <chrono>
 #include <cstdint>
 #include <thread>
 
+#include "curvefs/src/client/inode_wrapper.h"
 #include "curvefs/src/client/rpcclient/metaserver_client.h"
 #include "curvefs/test/client/mock_metaserver_client.h"
 #include "curvefs/src/client/inode_cache_manager.h"
@@ -402,6 +404,58 @@ TEST_F(TestInodeCacheManager, BatchGetInodeAttr) {
     ASSERT_EQ(getAttrs.begin()->length(), fileLength);
 }
 
+TEST_F(TestInodeCacheManager, BatchGetInodeAttrAsync) {
+    uint64_t parentId = 1;
+    uint64_t inodeId1 = 100;
+    uint64_t inodeId2 = 200;
+
+    // in
+    std::set<uint64_t> inodeIds;
+    inodeIds.emplace(inodeId1);
+    inodeIds.emplace(inodeId2);
+
+    // out
+    Inode inode;
+    inode.set_inodeid(inodeId1);
+
+    std::vector<std::vector<uint64_t>> inodeGroups;
+    inodeGroups.emplace_back(std::vector<uint64_t>{inodeId2});
+
+    std::map<uint64_t, InodeAttr> attrs;
+
+    RepeatedPtrField<InodeAttr> inodeAttrs;
+    inodeAttrs.Add()->set_inodeid(inodeId2);
+
+    // fill icache
+    EXPECT_CALL(*metaClient_, GetInode(_, inodeId1, _, _))
+        .WillOnce(DoAll(SetArgPointee<2>(inode),
+                Return(MetaStatusCode::OK)));
+    std::shared_ptr<InodeWrapper> wrapper;
+    iCacheManager_->GetInode(inodeId1, wrapper);
+
+    EXPECT_CALL(*metaClient_, SplitRequestInodes(_, _, _))
+        .WillOnce(DoAll(SetArgPointee<2>(inodeGroups),
+                Return(true)));
+    EXPECT_CALL(*metaClient_, BatchGetInodeAttrAsync(_, _, _))
+        .WillOnce(Invoke([inodeAttrs](uint32_t fsId,
+            const std::vector<uint64_t> &inodeIds,
+            MetaServerClientDone *done) {
+                done->SetMetaStatusCode(MetaStatusCode::OK);
+                static_cast<BatchGetInodeAttrDone *>(done)
+                    ->SetInodeAttrs(inodeAttrs);
+                done->Run();
+                return MetaStatusCode::OK;
+            }));
+
+    CURVEFS_ERROR ret = iCacheManager_->BatchGetInodeAttrAsync(parentId,
+        &inodeIds, &attrs);
+
+    ASSERT_EQ(CURVEFS_ERROR::OK, ret);
+    ASSERT_EQ(attrs.size(), 2);
+    ASSERT_TRUE(attrs.find(inodeId1) != attrs.end());
+    ASSERT_TRUE(attrs.find(inodeId2) != attrs.end());
+}
+
 TEST_F(TestInodeCacheManager, BatchGetXAttr) {
     uint64_t inodeId1 = 100;
     uint64_t inodeId2 = 200;