From b99521f213e60f88b3d7700f395fb0c4bf99f63a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Wed, 30 Jul 2008 18:32:14 +0000 Subject: [PATCH] First round of locking fixes: * Moved locking the data stream out of the BPlusTree methods; this fixes at least two locations where another thread could have seen outdated/incorrect data. * Removed some superfluous code in bfs_rename(): we don't have to revert to the previous situation, the transaction does this for us automatically. * Added a simple MultiWriteLocker, even though it's not really necessary anymore (since we always hold the transaction lock first when we lock more than one inode at a time). * Inode::Create() called InodeAllocator::Keep() a bit too early, the file cache and map wasn't created and assigned yet. * InodeAllocator now keeps the inode write locked. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@26689 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- .../kernel/file_systems/bfs/BPlusTree.cpp | 37 +++++----- src/add-ons/kernel/file_systems/bfs/Index.cpp | 4 ++ src/add-ons/kernel/file_systems/bfs/Inode.cpp | 16 +++-- .../file_systems/bfs/kernel_interface.cpp | 67 ++++++++----------- 4 files changed, 60 insertions(+), 64 deletions(-) diff --git a/src/add-ons/kernel/file_systems/bfs/BPlusTree.cpp b/src/add-ons/kernel/file_systems/bfs/BPlusTree.cpp index d4d8fe6d4b..03099a112a 100644 --- a/src/add-ons/kernel/file_systems/bfs/BPlusTree.cpp +++ b/src/add-ons/kernel/file_systems/bfs/BPlusTree.cpp @@ -1242,9 +1242,9 @@ BPlusTree::_SplitNode(bplustree_node *node, off_t nodeOffset, } -/*! - This inserts a key into the tree. The changes made to the tree will +/*! This inserts a key into the tree. The changes made to the tree will all be part of the \a transaction. + You need to have the inode write locked. */ status_t BPlusTree::Insert(Transaction &transaction, const uint8 *key, uint16 keyLength, @@ -1258,8 +1258,7 @@ BPlusTree::Insert(Transaction &transaction, const uint8 *key, uint16 keyLength, panic("tried to insert invalid value %Ld!\n", value); #endif - // lock access to stream - WriteLocker locker(fStream->Lock()); + ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock()); Stack stack; if (_SeekDown(stack, key, keyLength) != B_OK) @@ -1395,8 +1394,7 @@ BPlusTree::Insert(Transaction &transaction, const uint8 *key, uint16 keyLength, } -/*! - Removes the duplicate index/value pair from the tree. +/*! Removes the duplicate index/value pair from the tree. It's part of the private tree interface. */ status_t @@ -1578,8 +1576,7 @@ BPlusTree::_RemoveDuplicate(Transaction &transaction, } -/*! - Removes the key with the given index from the specified node. +/*! Removes the key with the given index from the specified node. Since it has to get the key from the node anyway (to obtain it's pointer), it's not needed to pass the key & its length, although the calling method (BPlusTree::Remove()) have this data. @@ -1639,10 +1636,10 @@ BPlusTree::_RemoveKey(bplustree_node *node, uint16 index) } -/*! - Removes the specified key from the tree. The "value" parameter is only used +/*! Removes the specified key from the tree. The "value" parameter is only used for trees which allow duplicates, so you may safely ignore it. It's not an optional parameter, so at least you have to think about it. + You need to have the inode write locked. */ status_t BPlusTree::Remove(Transaction &transaction, const uint8 *key, uint16 keyLength, @@ -1652,8 +1649,7 @@ BPlusTree::Remove(Transaction &transaction, const uint8 *key, uint16 keyLength, || keyLength > BPLUSTREE_MAX_KEY_LENGTH) RETURN_ERROR(B_BAD_VALUE); - // lock access to stream - WriteLocker locker(fStream->Lock()); + ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock()); Stack stack; if (_SeekDown(stack, key, keyLength) != B_OK) @@ -1749,14 +1745,14 @@ BPlusTree::Remove(Transaction &transaction, const uint8 *key, uint16 keyLength, } -/*! - Replaces the value for the key in the tree. +/*! Replaces the value for the key in the tree. Returns B_OK if the key could be found and its value replaced, B_ENTRY_NOT_FOUND if the key couldn't be found, and other errors to indicate that something went terribly wrong. Note that this doesn't work with duplicates - it will just return B_BAD_TYPE if you call this function on a tree where duplicates are allowed. + You need to have the inode write locked. */ status_t BPlusTree::Replace(Transaction &transaction, const uint8 *key, @@ -1770,8 +1766,7 @@ BPlusTree::Replace(Transaction &transaction, const uint8 *key, if (fAllowDuplicates) RETURN_ERROR(B_BAD_TYPE); - // lock access to stream (a read lock is okay for this purpose) - ReadLocker locker(fStream->Lock()); + ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock()); off_t nodeOffset = fHeader->RootNode(); CachedNode cached(this); @@ -1801,8 +1796,7 @@ BPlusTree::Replace(Transaction &transaction, const uint8 *key, } -/*! - Searches the key in the tree, and stores the offset found in +/*! Searches the key in the tree, and stores the offset found in _value, if successful. It's very similar to BPlusTree::SeekDown(), but doesn't fill a stack while it descends the tree. @@ -1812,6 +1806,7 @@ BPlusTree::Replace(Transaction &transaction, const uint8 *key, Note that this doesn't work with duplicates - it will just return B_BAD_TYPE if you call this function on a tree where duplicates are allowed. + You need to have the inode read or write locked. */ status_t BPlusTree::Find(const uint8 *key, uint16 keyLength, off_t *_value) @@ -1824,8 +1819,7 @@ BPlusTree::Find(const uint8 *key, uint16 keyLength, off_t *_value) if (fAllowDuplicates) RETURN_ERROR(B_BAD_TYPE); - // lock access to stream - ReadLocker locker(fStream->Lock()); + ASSERT_READ_LOCKED_RW_LOCK(&fStream->Lock()); off_t nodeOffset = fHeader->RootNode(); CachedNode cached(this); @@ -2080,8 +2074,7 @@ TreeIterator::Traverse(int8 direction, void *key, uint16 *keyLength, } -/*! - This is more or less a copy of BPlusTree::Find() - but it just +/*! This is more or less a copy of BPlusTree::Find() - but it just sets the current position in the iterator, regardless of if the key could be found or not. */ diff --git a/src/add-ons/kernel/file_systems/bfs/Index.cpp b/src/add-ons/kernel/file_systems/bfs/Index.cpp index 2a3fa17c67..254597540b 100644 --- a/src/add-ons/kernel/file_systems/bfs/Index.cpp +++ b/src/add-ons/kernel/file_systems/bfs/Index.cpp @@ -75,6 +75,8 @@ Index::SetTo(const char *name) if (indices == NULL) return B_ENTRY_NOT_FOUND; + ReadLocker locker(indices->Lock()); + BPlusTree *tree; if (indices->GetTree(&tree) != B_OK) return B_BAD_VALUE; @@ -268,6 +270,8 @@ Index::Update(Transaction &transaction, const char *name, int32 type, // remove the old key from the tree + WriteLocker locker(Node()->Lock()); + if (oldKey != NULL) { status = tree->Remove(transaction, (const uint8 *)oldKey, oldLength, inode->ID()); diff --git a/src/add-ons/kernel/file_systems/bfs/Inode.cpp b/src/add-ons/kernel/file_systems/bfs/Inode.cpp index 142db07687..81a7c91cc0 100644 --- a/src/add-ons/kernel/file_systems/bfs/Inode.cpp +++ b/src/add-ons/kernel/file_systems/bfs/Inode.cpp @@ -169,6 +169,7 @@ InodeAllocator::~InodeAllocator() fInode->Node().flags &= ~HOST_ENDIAN_TO_BFS_INT32(INODE_IN_USE); // this unblocks any pending bfs_read_vnode() calls fInode->Free(*fTransaction); + rw_lock_write_unlock(&fInode->Lock()); remove_vnode(volume->FSVolume(), fInode->ID()); } else volume->Free(*fTransaction, fRun); @@ -208,6 +209,7 @@ InodeAllocator::New(block_run *parentRun, mode_t mode, block_run &run, } } + rw_lock_write_lock(&fInode->Lock()); *_inode = fInode; return B_OK; } @@ -259,6 +261,8 @@ InodeAllocator::Keep(fs_vnode_ops *vnodeOps, uint32 publishFlags) TRANSACTION_ABORTED, &_TransactionListener, fInode); } + rw_lock_write_unlock(&fInode->Lock()); + fTransaction = NULL; fInode = NULL; @@ -1163,6 +1167,8 @@ Inode::GetAttribute(const char *name, Inode **_attribute) BPlusTree *tree; status_t status = attributes->GetTree(&tree); if (status == B_OK) { + ReadLocker locker(attributes->Lock()); + ino_t id; status = tree->Find((uint8 *)name, (uint16)strlen(name), &id); if (status == B_OK) { @@ -2206,6 +2212,8 @@ Inode::Remove(Transaction &transaction, const char *name, ino_t *_id, if (GetTree(&tree) != B_OK) RETURN_ERROR(B_BAD_VALUE); + WriteLocker locker(Lock()); + // does the file even exist? off_t id; if (tree->Find((uint8 *)name, (uint16)strlen(name), &id) < B_OK) @@ -2482,10 +2490,6 @@ Inode::Create(Transaction &transaction, Inode *parent, const char *name, index.InsertLastModified(transaction, inode); } - // Everything worked well until this point, we have a fully - // initialized inode, and we want to keep it - allocator.Keep(vnodeOps, publishFlags); - if (inode->IsFile() || inode->IsAttribute()) { inode->SetFileCache(file_cache_create(volume->ID(), inode->ID(), inode->Size())); @@ -2493,6 +2497,10 @@ Inode::Create(Transaction &transaction, Inode *parent, const char *name, inode->Size())); } + // Everything worked well until this point, we have a fully + // initialized inode, and we want to keep it + allocator.Keep(vnodeOps, publishFlags); + if (_created) *_created = true; if (_id != NULL) 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 56854b5a70..9e385b2bea 100644 --- a/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp +++ b/src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp @@ -24,6 +24,30 @@ struct identify_cookie { disk_super_block super_block; }; +class MultiWriteLocker { +public: + MultiWriteLocker(Inode* inodeA, Inode* inodeB) + { + if (inodeA->ID() < inodeB->ID()) { + Inode* tempInode = inodeA; + inodeA = inodeB; + inodeB = tempInode; + } + + fOuterLocker.SetTo(inodeA->Lock(), false); + if (inodeA != inodeB) + fInnerLocker.SetTo(inodeB->Lock(), false); + } + + ~MultiWriteLocker() + { + } + +private: + WriteLocker fOuterLocker; + WriteLocker fInnerLocker; +}; + extern void fill_stat_buffer(Inode *inode, struct stat &stat); @@ -530,6 +554,8 @@ bfs_lookup(fs_volume *_volume, fs_vnode *_directory, const char *file, if (directory->GetTree(&tree) != B_OK) RETURN_ERROR(B_BAD_VALUE); + ReadLocker locker(directory->Lock()); + status = tree->Find((uint8 *)file, (uint16)strlen(file), _vnodeID); if (status < B_OK) { //PRINT(("bfs_walk() could not find %Ld:\"%s\": %s\n", directory->BlockNumber(), file, strerror(status))); @@ -944,6 +970,9 @@ bfs_rename(fs_volume *_volume, fs_vnode *_oldDir, const char *oldName, if (oldDirectory == newDirectory && !strcmp(oldName, newName)) return B_OK; + Transaction transaction(volume, oldDirectory->BlockNumber()); + MultiWriteLocker locker(oldDirectory, newDirectory); + // are we allowed to do what we've been told? status_t status = oldDirectory->CheckPermissions(W_OK); if (status == B_OK) @@ -951,8 +980,6 @@ bfs_rename(fs_volume *_volume, fs_vnode *_oldDir, const char *oldName, if (status < B_OK) return status; - Transaction transaction(volume, oldDirectory->BlockNumber()); - // Get the directory's tree, and a pointer to the inode which should be // changed BPlusTree *tree; @@ -1041,10 +1068,6 @@ bfs_rename(fs_volume *_volume, fs_vnode *_oldDir, const char *oldName, if (status < B_OK) return status; - // If anything fails now, we have to remove the inode from the - // new directory in any case to restore the previous state - status_t bailStatus = B_OK; - // update the name only when they differ bool nameUpdated = false; if (strcmp(oldName, newName)) { @@ -1082,41 +1105,9 @@ bfs_rename(fs_volume *_volume, fs_vnode *_oldDir, const char *oldName, newDirectory->ID(), newName, id); return B_OK; } - // If we get here, something has gone wrong already! - - // Those better don't fail, or we switch to a read-only - // device for safety reasons (Volume::Panic() does this - // for us) - // Anyway, if we overwrote a file in the target directory - // this is lost now (only in-memory, not on-disk)... - bailStatus = tree->Insert(transaction, (const uint8 *)oldName, - strlen(oldName), id); - if (movedTree != NULL) { - movedTree->Replace(transaction, (const uint8 *)"..", 2, - oldDirectory->ID()); - } } } - if (bailStatus == B_OK && nameUpdated) { - bailStatus = inode->SetName(transaction, oldName); - if (status == B_OK) { - // update inode and index - inode->WriteBack(transaction); - - Index index(volume); - index.UpdateName(transaction, newName, oldName, inode); - } - } - - if (bailStatus == B_OK) { - bailStatus = newTree->Remove(transaction, (const uint8 *)newName, - strlen(newName), id); - } - - if (bailStatus < B_OK) - volume->Panic(); - return status; }