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

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
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock());
-	WriteLocker locker(fStream->Lock());
 
 	Stack<node_and_key> 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
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock());
-	WriteLocker locker(fStream->Lock());
 
 	Stack<node_and_key> 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)
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fStream->Lock());
-	ReadLocker locker(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
+	ASSERT_READ_LOCKED_RW_LOCK(&fStream->Lock());
-	ReadLocker locker(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.
 */

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());

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)

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;
 }