From 3caec2871a079cb604982ce7e5fc56865a7b438d Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Fri, 1 Jan 2010 20:20:11 +0000 Subject: [PATCH] * 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 | 15 +++++++++++++-- src/system/kernel/cache/vnode_store.h | 7 +++++-- src/system/kernel/fs/vfs.cpp | 23 +++++++++++++++-------- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/system/kernel/cache/vnode_store.cpp b/src/system/kernel/cache/vnode_store.cpp index bda4fa509e..239de16134 100644 --- a/src/system/kernel/cache/vnode_store.cpp +++ b/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) - fVnode = vnode; + if (status == B_OK && fVnodeDeleted) { + vfs_put_vnode(fVnode); + status = B_BUSY; + } return status; } diff --git a/src/system/kernel/cache/vnode_store.h b/src/system/kernel/cache/vnode_store.h index e51c8f2601..d4056682e7 100644 --- a/src/system/kernel/cache/vnode_store.h +++ b/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; }; diff --git a/src/system/kernel/fs/vfs.cpp b/src/system/kernel/fs/vfs.cpp index 14e06e9404..e0f154e106 100644 --- a/src/system/kernel/fs/vfs.cpp +++ b/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; }