diff --git a/src/add-ons/kernel/file_systems/bfs/Inode.cpp b/src/add-ons/kernel/file_systems/bfs/Inode.cpp index efb49c319f..a79506050a 100644 --- a/src/add-ons/kernel/file_systems/bfs/Inode.cpp +++ b/src/add-ons/kernel/file_systems/bfs/Inode.cpp @@ -1788,6 +1788,8 @@ Inode::Remove(Transaction *transaction, const char *name, off_t *_id, bool isDir if (GetTree(&tree) != B_OK) RETURN_ERROR(B_BAD_VALUE); + RecursiveLocker locker(fVolume->Lock()); + // does the file even exists? off_t id; if (tree->Find((uint8 *)name, (uint16)strlen(name), &id) < B_OK) @@ -1880,6 +1882,10 @@ Inode::Create(Transaction *transaction, Inode *parent, const char *name, int32 m Volume *volume = transaction->GetVolume(); BPlusTree *tree = NULL; + RecursiveLocker locker(volume->Lock()); + // ToDo: it would be nicer to only lock the parent directory, if possible + // (but that lock will already be held during any B+tree action) + if (parent && (mode & S_ATTR_DIR) == 0 && parent->IsContainer()) { // check if the file already exists in the directory if (parent->GetTree(&tree) != B_OK) diff --git a/src/add-ons/kernel/file_systems/bfs/Lock.h b/src/add-ons/kernel/file_systems/bfs/Lock.h index f7925648b1..8411cae583 100644 --- a/src/add-ons/kernel/file_systems/bfs/Lock.h +++ b/src/add-ons/kernel/file_systems/bfs/Lock.h @@ -15,11 +15,9 @@ // Configure here if and when real benaphores should be used -// Benaphores doesn't make much sense in the kernel, so they are only -// enabled for the user-space test application +#define USE_BENAPHORE + // if defined, benaphores are used for the Semaphore/RecursiveLock classes #ifdef USER -# define USE_BENAPHORE - // if defined, benaphores are used for the Semaphore class //# define FAST_LOCK // the ReadWriteLock class uses a second Semaphore to // speed up locking - only makes sense if USE_BENAPHORE @@ -85,7 +83,7 @@ class Semaphore { #endif }; -// a convenience class to lock the benaphore +// a convenience class to lock a Semaphore object class Locker { public: @@ -113,6 +111,103 @@ class Locker { }; +//**** Recursive Lock + +class RecursiveLock { + public: + RecursiveLock(const char *name) + : +#ifdef USE_BENAPHORE + fSemaphore(create_sem(0, name)), + fCount(1), +#else + fSemaphore(create_sem(1, name)), +#endif + fOwner(-1) + { +#ifndef USER + set_sem_owner(fSemaphore, B_SYSTEM_TEAM); +#endif + } + + status_t Lock() + { + thread_id thread = find_thread(NULL); + if (thread == fOwner) { + fOwnerCount++; + return B_OK; + } + + status_t status; +#ifdef USE_BENAPHORE + if (atomic_add(&fCount, -1) > 0) + status = B_OK; + else +#endif + status = acquire_sem(fSemaphore); + + if (status == B_OK) { + fOwner = thread; + fOwnerCount = 1; + } + + return status; + } + + status_t Unlock() + { + thread_id thread = find_thread(NULL); + if (thread != fOwner) + panic("RecursiveLock unlocked by %ld, owned by %ld\n", thread, fOwner); + + if (--fOwnerCount == 0) { + fOwner = -1; +#ifdef USE_BENAPHORE + if (atomic_add(&fCount, 1) < 0) +#endif + return release_sem(fSemaphore); + } + + return B_OK; + } + + private: + sem_id fSemaphore; +#ifdef USE_BENAPHORE + vint32 fCount; +#endif + thread_id fOwner; + int32 fOwnerCount; +}; + +// a convenience class to lock an RecursiveLock object + +class RecursiveLocker { + public: + RecursiveLocker(RecursiveLock &lock) + : fLock(lock) + { + fStatus = lock.Lock(); + ASSERT(fStatus == B_OK); + } + + ~RecursiveLocker() + { + if (fStatus == B_OK) + fLock.Unlock(); + } + + status_t Status() const + { + return fStatus; + } + + private: + RecursiveLock &fLock; + status_t fStatus; +}; + + //**** Many Reader/Single Writer Lock // This is a "fast" implementation of a single writer/many reader @@ -144,7 +239,7 @@ class Locker { // have to be called when there is no one waiting. // The disadvantage is the use of 2 real semaphores which is quite // expensive regarding that BeOS only allows for a total of 64k -// semaphores. +// semaphores (since every open BFS inode has such a lock). #ifdef FAST_LOCK class ReadWriteLock { @@ -211,7 +306,7 @@ class ReadWriteLock { // Acquire sem for all readers currently not using a semaphore. // But if we are not the only write lock in the queue, just get // the one for us - status = acquire_sem_etc(fSemaphore,readers <= 0 ? 1 : MAX_READERS - readers,0,0); + status = acquire_sem_etc(fSemaphore, readers <= 0 ? 1 : MAX_READERS - readers, 0, 0); } fWriteLock.Unlock(); @@ -220,10 +315,10 @@ class ReadWriteLock { void UnlockWrite() { - int32 readers = atomic_add(&fCount,MAX_READERS); + int32 readers = atomic_add(&fCount, MAX_READERS); if (readers < 0) { // release sem for all readers only when we were the only writer - release_sem_etc(fSemaphore,readers <= -MAX_READERS ? 1 : -readers,0); + release_sem_etc(fSemaphore, readers <= -MAX_READERS ? 1 : -readers, 0); } } @@ -281,12 +376,12 @@ class ReadWriteLock { status_t LockWrite() { - return acquire_sem_etc(fSemaphore,MAX_READERS,0,0); + return acquire_sem_etc(fSemaphore, MAX_READERS, 0, 0); } void UnlockWrite() { - release_sem_etc(fSemaphore,MAX_READERS,0); + release_sem_etc(fSemaphore, MAX_READERS, 0); } private: diff --git a/src/add-ons/kernel/file_systems/bfs/ToDo b/src/add-ons/kernel/file_systems/bfs/ToDo index 5ea926f3ae..b5e965d739 100644 --- a/src/add-ons/kernel/file_systems/bfs/ToDo +++ b/src/add-ons/kernel/file_systems/bfs/ToDo @@ -49,6 +49,7 @@ Inode - exchange Inode::OldLastModified() with Inode::NewLastModified(), and don't change the last_modified field directly in Inode::WriteAt() for consistency in case of a crash - the size is only updated in bfs_close() - but if the system crashes before, the entry in the size index doesn't match the one in the inode anymore - it would be better to let the data.size not reflect the real file size in this case (since the max_xxx_range entries are always correct) - Inode::FillGapWithZeros() currently disabled; apart from being slow, it really shouldn't be executed while a transaction is running, because that stops all other threads from doing anything (which can be a long time for a 100 MB file) + - need better locking mechanism in combination with B+trees etc.! Indices diff --git a/src/add-ons/kernel/file_systems/bfs/Volume.h b/src/add-ons/kernel/file_systems/bfs/Volume.h index 9fc0e06906..a4ece321a9 100644 --- a/src/add-ons/kernel/file_systems/bfs/Volume.h +++ b/src/add-ons/kernel/file_systems/bfs/Volume.h @@ -43,7 +43,7 @@ class Volume { bool IsValidSuperBlock(); bool IsReadOnly() const; void Panic(); - Semaphore &Lock(); + RecursiveLock &Lock(); block_run Root() const { return fSuperBlock.root_dir; } Inode *RootNode() const { return fRootNode; } @@ -115,7 +115,7 @@ class Volume { int fDevice; disk_super_block fSuperBlock; BlockAllocator fBlockAllocator; - Semaphore fLock; + RecursiveLock fLock; Journal *fJournal; vint32 fLogStart, fLogEnd; @@ -143,7 +143,7 @@ Volume::IsReadOnly() const } -inline Semaphore & +inline RecursiveLock & Volume::Lock() { return fLock; diff --git a/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp b/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp index 7fe435cb8b..3fff0188d3 100644 --- a/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp +++ b/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp @@ -274,6 +274,8 @@ bfs_read_fs_stat(void *_ns, struct fs_info *info) Volume *volume = (Volume *)_ns; + RecursiveLocker locker(volume->Lock()); + // File system flags. info->flags = B_FS_IS_PERSISTENT | B_FS_HAS_ATTR | B_FS_HAS_MIME | B_FS_HAS_QUERY | (volume->IsReadOnly() ? B_FS_IS_READONLY : 0); @@ -302,7 +304,7 @@ bfs_write_fs_stat(void *_ns, struct fs_info *info, long mask) Volume *volume = (Volume *)_ns; disk_super_block &superBlock = volume->SuperBlock(); - Locker locker(volume->Lock()); + RecursiveLocker locker(volume->Lock()); status_t status = B_BAD_VALUE; @@ -1012,6 +1014,8 @@ bfs_rename(void *_ns, void *_oldDir, const char *oldName, void *_newDir, const c if (oldDirectory == newDirectory && !strcmp(oldName, newName)) return B_OK; + RecursiveLocker locker(volume->Lock()); + // get the directory's tree, and a pointer to the inode which should be changed BPlusTree *tree; status_t status = oldDirectory->GetTree(&tree);