FuseDaemon: Rework inode tracking.

This change contains two related fixes :

(a) Clean up a couple of places where nodes were being Released but
not tracked. To do this in non-brittle way, the calls to NodeCreated
and NodeDeleted have been moved to the point of creation and deletion
(i.e, the calls to new and delete).

(b) Move  node::Create+NodeCreated to the same critical section,
and the same for node::Release+NodeReleased. Otherwise it's possible to
hit the following race conition:

T1: p1->Release()
T2: p1 = node::Create()
T2: NodeCreated(p1)
T1: NodeDeleted(p1)
T?: Lookup(p1)

(c) This change also tightens up inode tracking by making sure we never
insert a duplicate element into set of known inodes. This would have
made this problem a little more obvious.

(d) Also fix up fuse_node_test to reflect this new API, and fix a bug
in node::DeleteTree that was made very obvious by this fix, given the
extra work we do during deletion.

Test: atest FuseDaemonHostTest
Test: adb shell /data/local/fsstress-run.sh
Bug: 148709965

Change-Id: I6eea19020007eb56e6111f60cc9e5a4924b592a4
(cherry picked from commit fc867dd040ddee561f97d1bf9c1552a0d79153df)

4 files changed, 98 insertions, 58 deletions

diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index f960a9a7d..42fdd0501 100644
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -235,15 +235,14 @@ class FAdviser {
     const size_t target_ = 32 * 1024 * 1024;
 };
 
-// Whether inode tracking is enabled or not. When enabled, we maintain a
-// separate mapping from inode numbers to "live" nodes so we can detect when
-// we receive a request to a node that has been deleted.
-static constexpr bool kEnableInodeTracking = true;
-
 /* Single FUSE mount */
 struct fuse {
     explicit fuse(const std::string& _path)
-        : path(_path), root(node::CreateRoot(_path, &lock)), mp(0), zero_addr(0) {}
+        : path(_path),
+          tracker(mediaprovider::fuse::NodeTracker(&lock)),
+          root(node::CreateRoot(_path, &lock, &tracker)),
+          mp(0),
+          zero_addr(0) {}
 
     inline bool IsRoot(const node* node) const { return node == root; }
 
@@ -256,14 +255,7 @@ struct fuse {
             return root;
         }
 
-        node* node = node::FromInode(inode);
-
-        if (kEnableInodeTracking) {
-            std::lock_guard<std::recursive_mutex> guard(lock);
-            CHECK(inode_tracker_.find(node) != inode_tracker_.end());
-        }
-
-        return node;
+        return node::FromInode(inode, &tracker);
     }
 
     inline __u64 ToInode(node* node) const {
@@ -274,28 +266,10 @@ struct fuse {
         return node::ToInode(node);
     }
 
-    // Notify this FUSE instance that one of its nodes has been deleted.
-    void NodeDeleted(const node* node) {
-        if (kEnableInodeTracking) {
-            LOG(INFO) << "Node: " << node << " deleted.";
-
-            std::lock_guard<std::recursive_mutex> guard(lock);
-            inode_tracker_.erase(node);
-        }
-    }
-
-    // Notify this FUSE instance that a new nodes has been created.
-    void NodeCreated(const node* node) {
-        if (kEnableInodeTracking) {
-            LOG(INFO) << "Node: " << node << " created.";
-
-            std::lock_guard<std::recursive_mutex> guard(lock);
-            inode_tracker_.insert(node);
-        }
-    }
-
     std::recursive_mutex lock;
     const string path;
+    // The Inode tracker associated with this FUSE instance.
+    mediaprovider::fuse::NodeTracker tracker;
     node* const root;
     struct fuse_session* se;
 
@@ -313,8 +287,6 @@ struct fuse {
     /* const */ char* zero_addr;
 
     FAdviser fadviser;
-
-    std::unordered_set<const node*> inode_tracker_;
 };
 
 static inline string safe_name(node* n) {
@@ -413,8 +385,7 @@ static node* make_node_entry(fuse_req_t req, node* parent, const string& name, c
 
     node = parent->LookupChildByName(name, true /* acquire */);
     if (!node) {
-        node = ::node::Create(parent, name, &fuse->lock);
-        fuse->NodeCreated(node);
+        node = ::node::Create(parent, name, &fuse->lock, &fuse->tracker);
     }
 
     TRACE_NODE(node);
@@ -502,9 +473,7 @@ static void do_forget(struct fuse* fuse, fuse_ino_t ino, uint64_t nlookup) {
         // This is a narrowing conversion from an unsigned 64bit to a 32bit value. For
         // some reason we only keep 32 bit refcounts but the kernel issues
         // forget requests with a 64 bit counter.
-        if (node->Release(static_cast<uint32_t>(nlookup))) {
-            fuse->NodeDeleted(node);
-        }
+        node->Release(static_cast<uint32_t>(nlookup));
     }
 }
 
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 2aa3b138f..5098316c4 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -24,6 +24,7 @@
 #include <mutex>
 #include <sstream>
 #include <string>
+#include <unordered_set>
 #include <vector>
 
 #include "libfuse_jni/ReaddirHelper.h"
@@ -64,17 +65,70 @@ struct dirhandle {
     ~dirhandle() { closedir(d); }
 };
 
+// Whether inode tracking is enabled or not. When enabled, we maintain a
+// separate mapping from inode numbers to "live" nodes so we can detect when
+// we receive a request to a node that has been deleted.
+static constexpr bool kEnableInodeTracking = true;
+
+class node;
+
+// Tracks the set of active nodes associated with a FUSE instance so that we
+// can assert that we only ever return an active node in response to a lookup.
+class NodeTracker {
+  public:
+    NodeTracker(std::recursive_mutex* lock) : lock_(lock) {}
+
+    void CheckTracked(__u64 ino) const {
+        if (kEnableInodeTracking) {
+            const node* node = reinterpret_cast<const class node*>(ino);
+            std::lock_guard<std::recursive_mutex> guard(*lock_);
+            CHECK(active_nodes_.find(node) != active_nodes_.end());
+        }
+    }
+
+    void NodeDeleted(const node* node) {
+        if (kEnableInodeTracking) {
+            std::lock_guard<std::recursive_mutex> guard(*lock_);
+            LOG(DEBUG) << "Node: " << reinterpret_cast<uintptr_t>(node) << " deleted.";
+
+            CHECK(active_nodes_.find(node) != active_nodes_.end());
+            active_nodes_.erase(node);
+        }
+    }
+
+    void NodeCreated(const node* node) {
+        if (kEnableInodeTracking) {
+            std::lock_guard<std::recursive_mutex> guard(*lock_);
+            LOG(DEBUG) << "Node: " << reinterpret_cast<uintptr_t>(node) << " created.";
+
+            CHECK(active_nodes_.find(node) == active_nodes_.end());
+            active_nodes_.insert(node);
+        }
+    }
+
+  private:
+    std::recursive_mutex* lock_;
+    std::unordered_set<const node*> active_nodes_;
+};
+
 class node {
   public:
     // Creates a new node with the specified parent, name and lock.
-    static node* Create(node* parent, const std::string& name, std::recursive_mutex* lock) {
-        return new node(parent, name, lock);
+    static node* Create(node* parent, const std::string& name, std::recursive_mutex* lock,
+                        NodeTracker* tracker) {
+        // Place the entire constructor under a critical section to make sure
+        // node creation, tracking (if enabled) and the addition to a parent are
+        // atomic.
+        std::lock_guard<std::recursive_mutex> guard(*lock);
+        return new node(parent, name, lock, tracker);
     }
 
     // Creates a new root node. Root nodes have no parents by definition
     // and their "name" must signify an absolute path.
-    static node* CreateRoot(const std::string& path, std::recursive_mutex* lock) {
-        node* root = new node(nullptr, path, lock);
+    static node* CreateRoot(const std::string& path, std::recursive_mutex* lock,
+                            NodeTracker* tracker) {
+        std::lock_guard<std::recursive_mutex> guard(*lock);
+        node* root = new node(nullptr, path, lock, tracker);
 
         // The root always has one extra reference to avoid it being
         // accidentally collected.
@@ -83,7 +137,8 @@ class node {
     }
 
     // Maps an inode to its associated node.
-    static inline node* FromInode(__u64 ino) {
+    static inline node* FromInode(__u64 ino, const NodeTracker* tracker) {
+        tracker->CheckTracked(ino);
         return reinterpret_cast<node*>(static_cast<uintptr_t>(ino));
     }
 
@@ -218,8 +273,14 @@ class node {
     static const node* LookupAbsolutePath(const node* root, const std::string& absolute_path);
 
   private:
-    node(node* parent, const std::string& name, std::recursive_mutex* lock)
-        : name_(name), refcount_(0), parent_(nullptr), deleted_(false), lock_(lock) {
+    node(node* parent, const std::string& name, std::recursive_mutex* lock, NodeTracker* tracker)
+        : name_(name),
+          refcount_(0),
+          parent_(nullptr),
+          deleted_(false),
+          lock_(lock),
+          tracker_(tracker) {
+        tracker_->NodeCreated(this);
         Acquire();
         // This is a special case for the root node. All other nodes will have a
         // non-null parent.
@@ -287,11 +348,15 @@ class node {
     bool deleted_;
     std::recursive_mutex* lock_;
 
+    NodeTracker* const tracker_;
+
     ~node() {
         RemoveFromParent();
 
         handles_.clear();
         dirhandles_.clear();
+
+        tracker_->NodeDeleted(this);
     }
 
     friend class ::NodeTest;
diff --git a/jni/node.cpp b/jni/node.cpp
index 618777442..e17a9e88a 100644
--- a/jni/node.cpp
+++ b/jni/node.cpp
@@ -97,13 +97,15 @@ void node::DeleteTree(node* tree) {
     std::lock_guard<std::recursive_mutex> guard(*tree->lock_);
 
     if (tree) {
-        for (node* child : tree->children_) {
+        // Make a copy of the list of children because calling Delete tree
+        // will modify the list of children, which will cause issues while
+        // iterating over them.
+        std::vector<node*> children(tree->children_.begin(), tree->children_.end());
+        for (node* child : children) {
             DeleteTree(child);
         }
-        tree->children_.clear();
 
-        LOG(DEBUG) << "DELETE node " << tree->GetName();
-        tree->RemoveFromParent();
+        CHECK(tree->children_.empty());
         delete tree;
     }
 }
diff --git a/jni/node_test.cpp b/jni/node_test.cpp
index ca0405c9a..423af3d37 100644
--- a/jni/node_test.cpp
+++ b/jni/node_test.cpp
@@ -9,15 +9,19 @@
 using mediaprovider::fuse::dirhandle;
 using mediaprovider::fuse::handle;
 using mediaprovider::fuse::node;
+using mediaprovider::fuse::NodeTracker;
 
 // Listed as a friend class to struct node so it can observe implementation
 // details if required. The only implementation detail that is worth writing
 // tests around at the moment is the reference count.
 class NodeTest : public ::testing::Test {
   public:
+    NodeTest() : tracker_(NodeTracker(&lock_)) {}
+
     uint32_t GetRefCount(node* node) { return node->refcount_; }
 
     std::recursive_mutex lock_;
+    NodeTracker tracker_;
 
     // Forward destruction here, as NodeTest is a friend class.
     static void destroy(node* node) { delete node; }
@@ -27,7 +31,7 @@ class NodeTest : public ::testing::Test {
     typedef std::unique_ptr<node, decltype(&NodeTest::destroy)> unique_node_ptr;
 
     unique_node_ptr CreateNode(node* parent, const std::string& path) {
-        return unique_node_ptr(node::Create(parent, path, &lock_), &NodeTest::destroy);
+        return unique_node_ptr(node::Create(parent, path, &lock_, &tracker_), &NodeTest::destroy);
     }
 };
 
@@ -53,7 +57,7 @@ TEST_F(NodeTest, TestCreate_withParent) {
 }
 
 TEST_F(NodeTest, TestRelease) {
-    node* node = node::Create(nullptr, "/path", &lock_);
+    node* node = node::Create(nullptr, "/path", &lock_, &tracker_);
     acquire(node);
     acquire(node);
     ASSERT_EQ(3, GetRefCount(node));
@@ -152,10 +156,10 @@ TEST_F(NodeTest, DeleteTree) {
     unique_node_ptr parent = CreateNode(nullptr, "/path");
 
     // This is the tree that we intend to delete.
-    node* child = node::Create(parent.get(), "subdir", &lock_);
-    node::Create(child, "s1", &lock_);
-    node* subchild2 = node::Create(child, "s2", &lock_);
-    node::Create(subchild2, "sc2", &lock_);
+    node* child = node::Create(parent.get(), "subdir", &lock_, &tracker_);
+    node::Create(child, "s1", &lock_, &tracker_);
+    node* subchild2 = node::Create(child, "s2", &lock_, &tracker_);
+    node::Create(subchild2, "sc2", &lock_, &tracker_);
 
     ASSERT_EQ(child, parent->LookupChildByName("subdir", false /* acquire */));
     node::DeleteTree(child);