* Resolved TODO in free_vnode(): There was a race condition between vnode

destruction and VMVnodeCache::AcquireUnreferencedStoreRef(). Solved by
  adding a flag to VMVnodeCache and letting AcquireUnreferencedStoreRef()
  fail, if set.
* Added TODO regarding replacing the snooze() waiting for busy vnodes.
* get_vnode(): Unlock sVnodeMutex while calling the put_vnode() hook on
  error.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@34841 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/system/kernel/cache/vnode_store.cpp

@@ -25,6 +25,7 @@ VMVnodeCache::Init(struct vnode *vnode)
 
 	fVnode = vnode;
 	fFileCacheRef = NULL;
+	fVnodeDeleted = false;
 
 	vfs_vnode_to_node_ref(fVnode, &fDevice, &fInode);
 
@@ -117,13 +118,23 @@ VMVnodeCache::CanWritePage(off_t offset)
 status_t
 VMVnodeCache::AcquireUnreferencedStoreRef()
 {
+	// Quick check whether getting a vnode reference is still allowed. Only
+	// after a successful vfs_get_vnode() the check is safe (since then we've
+	// either got the reference to our vnode, or have been notified that it is
+	// toast), but the check is cheap and saves quite a bit of work in case the
+	// condition holds.
+	if (fVnodeDeleted)
+		return B_BUSY;
+
 	struct vnode *vnode;
 	status_t status = vfs_get_vnode(fDevice, fInode, false, &vnode);
 
 	// If successful, update the store's vnode pointer, so that release_ref()
 	// won't use a stale pointer.
-	if (status == B_OK)
+	if (status == B_OK && fVnodeDeleted) {
-		fVnode = vnode;
+		vfs_put_vnode(fVnode);
+		status = B_BUSY;
+	}
 
 	return status;
 }

src/system/kernel/cache/vnode_store.h

@@ -39,11 +39,14 @@ public:
 	file_cache_ref*		FileCacheRef() const
 							{ return fFileCacheRef; }
 
+			void		VnodeDeleted()	{ fVnodeDeleted = true; }
+
 private:
 	struct vnode*	fVnode;
-	dev_t			fDevice;
-	ino_t			fInode;
 	file_cache_ref*	fFileCacheRef;
+	ino_t			fInode;
+	dev_t			fDevice;
+	volatile bool	fVnodeDeleted;
 };
 
 

src/system/kernel/fs/vfs.cpp

@@ -56,6 +56,7 @@
 
 #include "fifo.h"
 #include "IORequest.h"
+#include "../cache/vnode_store.h"
 
 
 //#define TRACE_VFS
@@ -1092,12 +1093,6 @@ free_vnode(struct vnode* vnode, bool reenter)
 	// count, so that it will neither become negative nor 0.
 	vnode->ref_count = 2;
 
-	// TODO: Usually, when the vnode is unreferenced, no one can get hold of the
-	// cache either (i.e. no one can get a cache reference while we're deleting
-	// the vnode).. This is, however, not the case for the page daemon. It gets
-	// its cache references via the pages it scans, so it can in fact get a
-	// vnode reference while we're deleting the vnode.
-
 	if (!vnode->unpublished) {
 		if (vnode->remove)
 			FS_CALL(vnode, remove_vnode, reenter);
@@ -1105,8 +1100,16 @@ free_vnode(struct vnode* vnode, bool reenter)
 			FS_CALL(vnode, put_vnode, reenter);
 	}
 
+	// If the vnode has a VMCache attached, make sure that it won't try to get
+	// another reference via VMVnodeCache::AcquireUnreferencedStoreRef(). As
+	// long as the vnode is busy and in the hash, that won't happen, but as
+	// soon as we've removed it from the hash, it could reload the vnode -- with
+	// a new cache attached!
+	if (vnode->cache != NULL)
+		((VMVnodeCache*)vnode->cache)->VnodeDeleted();
+
 	// The file system has removed the resources of the vnode now, so we can
-	// make it available again (and remove the busy vnode from the hash)
+	// make it available again (by removing the busy vnode from the hash).
 	mutex_lock(&sVnodeMutex);
 	hash_remove(sVnodeTable, vnode);
 	mutex_unlock(&sVnodeMutex);
@@ -1277,6 +1280,7 @@ restart:
 				vnodeID);
 			return B_BUSY;
 		}
+// TODO: Replace this mechanism!
 		snooze(10000); // 10 ms
 		mutex_lock(&sVnodeMutex);
 		goto restart;
@@ -1323,8 +1327,11 @@ restart:
 		mutex_lock(&sVnodeMutex);
 
 		if (status != B_OK) {
-			if (gotNode)
+			if (gotNode) {
+				mutex_unlock(&sVnodeMutex);
 				FS_CALL(vnode, put_vnode, reenter);
+				mutex_lock(&sVnodeMutex);
+			}
 
 			goto err1;
 		}