* The previous locking order "inode write" -> "journal" was not only not

honoured (in the Inode attribute code), it also couldn't be honoured there.
  Therefore, the locking order is now reversed, that is "journal" comes first,
  then the Inode write lock. This should also help with the lock ups that
  appeared after r22886.
* Got rid of INODE_NO_TRANSACTION; it doesn't make any sense to use.
* When Inode::WriteAttribute() changed the type, the type field was actually
  never written back within the current transaction.
* If the attribute data fit into the inode again, the attribute's write lock
  was never released.
* Since r22886, Inode::ReadAt() does its own locking, so we must no longer lock
  the inode before calling it.


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

src/add-ons/kernel/file_systems/bfs/Inode.cpp

@@ -55,11 +55,13 @@ InodeAllocator::~InodeAllocator()
 
 
 status_t 
-InodeAllocator::New(block_run *parentRun, mode_t mode, block_run &run, Inode **_inode)
+InodeAllocator::New(block_run *parentRun, mode_t mode, block_run &run,
+	Inode **_inode)
 {
 	Volume *volume = fTransaction->GetVolume();
 
-	status_t status = volume->AllocateForInode(*fTransaction, parentRun, mode, fRun);
+	status_t status = volume->AllocateForInode(*fTransaction, parentRun, mode,
+		fRun);
 	if (status < B_OK) {
 		// don't free the space in the destructor, because
 		// the allocation failed
@@ -101,7 +103,8 @@ InodeAllocator::CreateTree()
 
 	if (fInode->IsRegularNode()) {
 		if (tree->Insert(*fTransaction, ".", fInode->ID()) < B_OK
-			|| tree->Insert(*fTransaction, "..", volume->ToVnode(fInode->Parent())) < B_OK)
+			|| tree->Insert(*fTransaction, "..",
+					volume->ToVnode(fInode->Parent())) < B_OK)
 			return B_ERROR;
 	}
 	return B_OK;
@@ -833,11 +836,7 @@ Inode::ReadAttribute(const char *name, int32 type, off_t pos, uint8 *buffer,
 	Inode *attribute;
 	status_t status = GetAttribute(name, &attribute);
 	if (status == B_OK) {
-		if (attribute->Lock().Lock() == B_OK) {
+		status = attribute->ReadAt(pos, (uint8 *)buffer, _length);
-			status = attribute->ReadAt(pos, (uint8 *)buffer, _length);
-			attribute->Lock().Unlock();
-		} else
-			status = B_ERROR;
 
 		ReleaseAttribute(attribute);
 	}
@@ -862,6 +861,8 @@ Inode::WriteAttribute(Transaction &transaction, const char *name, int32 type,
 	// TODO: we actually depend on that the contents of "buffer" are constant.
 	// If they get changed during the write (hey, user programs), we may mess
 	// up our index trees!
+	// TODO: for attribute files, we need to log the first BPLUSTREE_MAX_KEY_LENGTH
+	// bytes of the data stream, or the same as above might happen.
 
 	Index index(fVolume);
 	index.SetTo(name);
@@ -916,19 +917,24 @@ Inode::WriteAttribute(Transaction &transaction, const char *name, int32 type,
 
 			// check if the data fits into the small_data section again
 			NodeGetter node(fVolume, transaction, this);
-			status = _AddSmallData(transaction, node, name, type, buffer, *_length);
+			status = _AddSmallData(transaction, node, name, type, buffer,
+				*_length);
+
 			if (status == B_OK) {
 				// it does - remove its file
+				attribute->Lock().UnlockWrite();
 				status = _RemoveAttribute(transaction, name, false, NULL);
 			} else {
-				status = attribute->WriteAt(transaction, pos, buffer, _length);
+				// The attribute type might have been changed - we need to
-				if (status == B_OK) {
+				// adopt the new one
-					// The attribute type might have been changed - we need to adopt
+				attribute->Node().type = HOST_ENDIAN_TO_BFS_INT32(type);
-					// the new one - the attribute's inode will be written back on close
+				status = attribute->WriteBack(transaction);
-					attribute->Node().type = HOST_ENDIAN_TO_BFS_INT32(type);
-				}
-
 				attribute->Lock().UnlockWrite();
+
+				if (status == B_OK) {
+					status = attribute->WriteAt(transaction, pos, buffer,
+						_length);
+				}
 			}
 		} else
 			status = B_ERROR;
@@ -976,8 +982,8 @@ Inode::RemoveAttribute(Transaction &transaction, const char *name)
 			uint32 length = smallData->DataSize();
 			if (length > BPLUSTREE_MAX_KEY_LENGTH)
 				length = BPLUSTREE_MAX_KEY_LENGTH;
-			index.Update(transaction, name, smallData->Type(), smallData->Data(),
+			index.Update(transaction, name, smallData->Type(),
-				length, NULL, 0, this);
+				smallData->Data(), length, NULL, 0, this);
 		}
 		fSmallDataLock.Unlock();
 	}
@@ -1224,14 +1230,6 @@ Inode::FindBlockRun(off_t pos, block_run &run, off_t &offset)
 status_t
 Inode::ReadAt(off_t pos, uint8 *buffer, size_t *_length)
 {
-#if 0
-	// call the right ReadAt() method, depending on the inode flags
-
-	if (Flags() & INODE_LOGGED)
-		return ((Stream<Access::Logged> *)this)->ReadAt(pos, buffer, _length);
-
-	return ((Stream<Access::Cached> *)this)->ReadAt(pos, buffer, _length);
-#endif
 	size_t length = *_length;
 
 	// set/check boundaries for pos/length
@@ -1266,27 +1264,28 @@ Inode::WriteAt(Transaction &transaction, off_t pos, const uint8 *buffer,
 		<< INODE_TIME_SHIFT);
 
 	// TODO: support INODE_LOGGED!
-#if 0
-	if (Flags() & INODE_LOGGED)
-		return ((Stream<Access::Logged> *)this)->WriteAt(transaction, pos, buffer, _length);
 
-	return ((Stream<Access::Cached> *)this)->WriteAt(transaction, pos, buffer, _length);
-#endif
 	size_t length = *_length;
+	bool changeSize = false;
 
 	// set/check boundaries for pos/length
 	if (pos < 0)
 		return B_BAD_VALUE;
 
+	if (pos + length > Size())
+		changeSize = true;
+
+	locker.Unlock();
+
+	// the transaction doesn't have to be started already
+	if (changeSize && !transaction.IsStarted())
+		transaction.Start(fVolume, BlockNumber());
+
+	locker.Lock();
+
 	if (pos + length > Size()) {
 		off_t oldSize = Size();
 
-		// the transaction doesn't have to be started already
-		// ToDo: what's that INODE_NO_TRANSACTION flag good for again?
-		if ((Flags() & INODE_NO_TRANSACTION) == 0
-			&& !transaction.IsStarted())
-			transaction.Start(fVolume, BlockNumber());
-
 		// let's grow the data stream to the size needed
 		status_t status = SetFileSize(transaction, pos + length);
 		if (status < B_OK) {

src/add-ons/kernel/file_systems/bfs/Lock.h

@@ -479,7 +479,7 @@ class WriteLocked {
 			:
 			fLock(lock)
 		{
-			fStatus = lock != NULL ? lock->LockWrite() : B_ERROR;
+			fStatus = lock != NULL ? lock->LockWrite() : B_NO_INIT;
 		}
 
 		~WriteLocked()
@@ -488,6 +488,14 @@ class WriteLocked {
 				fLock->UnlockWrite();
 		}
 
+		status_t
+		Lock()
+		{
+			if (fStatus == B_OK)
+				return B_ERROR;
+			return fStatus = fLock->LockWrite();
+		}
+
 		status_t
 		IsLocked()
 		{
@@ -497,8 +505,9 @@ class WriteLocked {
 		void
 		Unlock()
 		{
-			fLock->UnlockWrite();
+			if (fStatus == B_OK)
-			fStatus = B_NO_INIT;
+				fLock->UnlockWrite();
+			fStatus = B_ERROR;
 		}
 
 	private:

src/add-ons/kernel/file_systems/bfs/bfs.h

@@ -217,7 +217,6 @@ enum inode_flags {
 	INODE_PERMANENT_FLAGS	= 0x0000ffff,
 
 	INODE_WAS_WRITTEN		= 0x00020000,
-	INODE_NO_TRANSACTION	= 0x00040000,
 	INODE_DONT_FREE_SPACE	= 0x00080000,	// only used by the "chkbfs" functionality
 	INODE_CHKBFS_RUNNING	= 0x00200000,
 };

src/add-ons/kernel/file_systems/bfs/kernel_interface.cpp

@@ -694,20 +694,19 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask)
 	Volume *volume = (Volume *)_ns;
 	Inode *inode = (Inode *)_node;
 
-	// that may be incorrect here - I don't think we need write access to
+	// TODO: we should definitely check a bit more if the new stats are
-	// change most of the stat...
+	//	valid - or even better, the VFS should check this before calling us
-	// we should definitely check a bit more if the new stats are correct and valid...
 	
 	status_t status = inode->CheckPermissions(W_OK);
 	if (status < B_OK)
 		RETURN_ERROR(status);
 
+	Transaction transaction(volume, inode->BlockNumber());
+
 	WriteLocked locked(inode->Lock());
 	if (locked.IsLocked() < B_OK)
 		RETURN_ERROR(B_ERROR);
 
-	Transaction transaction(volume, inode->BlockNumber());
-
 	bfs_inode &node = inode->Node();
 
 	if (mask & B_STAT_SIZE) {
@@ -737,7 +736,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask)
 
 	if (mask & B_STAT_MODE) {
 		PRINT(("original mode = %ld, stat->st_mode = %d\n", node.Mode(), stat->st_mode));
-		node.mode = HOST_ENDIAN_TO_BFS_INT32(node.Mode() & ~S_IUMSK | stat->st_mode & S_IUMSK);
+		node.mode = HOST_ENDIAN_TO_BFS_INT32(node.Mode() & ~S_IUMSK
+			| stat->st_mode & S_IUMSK);
 	}
 
 	if (mask & B_STAT_UID)
@@ -762,7 +762,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask)
 			(bigtime_t)stat->st_crtime << INODE_TIME_SHIFT);
 	}
 
-	if ((status = inode->WriteBack(transaction)) == B_OK)
+	status = inode->WriteBack(transaction);
+	if (status == B_OK)
 		transaction.Done();
 
 	notify_stat_changed(volume->ID(), inode->ID(), mask);
@@ -772,8 +773,8 @@ bfs_write_stat(void *_ns, void *_node, const struct stat *stat, uint32 mask)
 
 
 status_t 
-bfs_create(void *_ns, void *_directory, const char *name, int openMode, int mode,
+bfs_create(void *_ns, void *_directory, const char *name, int openMode,
-	void **_cookie, ino_t *_vnodeID)
+	int mode, void **_cookie, ino_t *_vnodeID)
 {
 	FUNCTION_START(("name = \"%s\", perms = %d, openMode = %d\n", name, mode, openMode));
 
@@ -1147,8 +1148,8 @@ bfs_open(void *_fs, void *_node, int openMode, void **_cookie)
 
 	// Should we truncate the file?
 	if (openMode & O_TRUNC) {
-		WriteLocked locked(inode->Lock());
 		Transaction transaction(volume, inode->BlockNumber());
+		WriteLocked locked(inode->Lock());
 
 		status_t status = inode->SetFileSize(transaction, 0);
 		if (status >= B_OK)
@@ -1255,25 +1256,32 @@ bfs_free_cookie(void *_ns, void *_node, void *_cookie)
 		return B_BAD_VALUE;
 
 	file_cookie *cookie = (file_cookie *)_cookie;
-
 	Volume *volume = (Volume *)_ns;
 	Inode *inode = (Inode *)_node;
 
-	WriteLocked locked(inode->Lock());
+	Transaction transaction;
+	bool needsTrimming;
 
-	bool needsTrimming = inode->NeedsTrimming();
+	{
+		ReadLocked locker(inode->Lock());
+		needsTrimming = inode->NeedsTrimming();
 
-	if ((cookie->open_mode & O_RWMASK) != 0
+		if ((cookie->open_mode & O_RWMASK) != 0
-		&& !inode->IsDeleted()
+			&& !inode->IsDeleted()
-		&& (needsTrimming
+			&& (needsTrimming
-			|| inode->OldLastModified() != inode->LastModified()
+				|| inode->OldLastModified() != inode->LastModified()
-			|| inode->OldSize() != inode->Size())) {
+				|| inode->OldSize() != inode->Size())) {
+			locker.Unlock();
+			transaction.Start(volume, inode->BlockNumber());
+		}
+	}
 
+	WriteLocked locker(inode->Lock());
+	status_t status = B_ERROR;
+
+	if (transaction.IsStarted()) {
 		// trim the preallocated blocks and update the size,
 		// and last_modified indices if needed
-
-		Transaction transaction(volume, inode->BlockNumber());
-		status_t status = B_OK;
 		bool changedSize = false, changedTime = false;
 		Index index(volume);
 
@@ -1299,14 +1307,14 @@ bfs_free_cookie(void *_ns, void *_node, void *_cookie)
 			inode->WriteBack(transaction);
 		}
 
-		if (status == B_OK)
-			transaction.Done();
-
 		if (changedSize || changedTime) {
 			notify_stat_changed(volume->ID(), inode->ID(),
-				(changedTime ? B_STAT_MODIFICATION_TIME : 0) | (changedSize ? B_STAT_SIZE : 0));
+				(changedTime ? B_STAT_MODIFICATION_TIME : 0)
+				| (changedSize ? B_STAT_SIZE : 0));
 		}
 	}
+	if (status == B_OK)
+		transaction.Done();
 
 	if (inode->Flags() & INODE_CHKBFS_RUNNING) {
 		// "chkbfs" exited abnormally, so we have to stop it here...