From 7723ae9df290e2e6f62e84fa9a75bdf8f9e620f9 Mon Sep 17 00:00:00 2001
From: Ingo Weinhold <ingo_weinhold@gmx.de>
Date: Thu, 21 Jul 2011 20:28:01 +0200
Subject: [PATCH] Volume::_RemoveNodeAndVNode(): Squash TODO

Only get/remove/put the vnode when the node is actually known to the
VFS. This does not only save unnecessary work, it also solves a
(temporary) deadlock -- at least partially. If another thread caused a
call to our get_vnode() hook just before, it would block on the volume
lock we're holding when adding/removing packages. The vnode would be
marked busy until the other thread's request was fulfilled and our call
to get_vnode() would block until timing out. Now we're calling
get_vnode() only, if we already know that the VFS already has a valid
vnode.

There still remains a race condition. If the VFS discards the vnode
right before we call get_vnode(), we essentially have the same situation
as before (i.e. us calling get_vnode() although the vnode is no longer
known to the VFS) with the same potential problem. For a real solution
we need a get_vnode() variant which can be told not to block.
---
 .../kernel/file_systems/packagefs/Volume.cpp  | 33 +++++++++++++++----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/add-ons/kernel/file_systems/packagefs/Volume.cpp b/src/add-ons/kernel/file_systems/packagefs/Volume.cpp
index 8e7c70e576..f4be297e25 100644
--- a/src/add-ons/kernel/file_systems/packagefs/Volume.cpp
+++ b/src/add-ons/kernel/file_systems/packagefs/Volume.cpp
@@ -1474,15 +1474,36 @@ Volume::_RemoveNode(Node* node)
 void
 Volume::_RemoveNodeAndVNode(Node* node)
 {
-	// we get and put the vnode to notify the VFS
-	// TODO: We should probably only do that, if the node is known to the
-	// VFS in the first place.
-	Node* dummyNode;
-	bool gotVNode = GetVNode(node->ID(), dummyNode) == B_OK;
+	// If the node is known to the VFS, we get the vnode, remove it, and put it,
+	// so that the VFS will discard it as soon as possible (i.e. now, if no one
+	// else is using it).
+	NodeWriteLocker nodeWriteLocker(node);
 
+	// Remove the node from its parent and the volume. This makes the node
+	// inaccessible via the get_vnode() and lookup() hooks.
 	_RemoveNode(node);
 
-	if (gotVNode) {
+	bool getVNode = node->IsKnownToVFS();
+
+	nodeWriteLocker.Unlock();
+
+	// Get a vnode reference, if the node is already known to the VFS.
+	Node* dummyNode;
+	if (getVNode && GetVNode(node->ID(), dummyNode) == B_OK) {
+		// TODO: There still is a race condition here which we can't avoid
+		// without more help from the VFS. Right after we drop the write
+		// lock a vnode for the node could be discarded by the VFS. At that
+		// point another thread trying to get the vnode by ID would create
+		// a vnode, mark it busy and call our get_vnode() hook. It would
+		// block since we (i.e. the package loader thread executing this
+		// method) still have the volume write lock. Our get_vnode() call
+		// would block, since it finds the vnode marked busy. It times out
+		// eventually, but until then a good deal of FS operations might
+		// block as well due to us holding the volume lock and probably
+		// several node locks as well. A get_vnode*() variant (e.g.
+		// get_vnode_etc() with flags parameter) that wouldn't block and
+		// only get the vnode, if already loaded and non-busy, would be
+		// perfect here.
 		RemoveVNode(node->ID());
 		PutVNode(node->ID());
 	}