* Journal::_WriteTransactionToLog()'s return value was ignored previously,

leading BFS to report success on actually failed transactions. Those
  transactions were even kept open, leading to a panic when starting the next
  transaction.
* Transaction::Done() now returns a status - currently, this is only handled
  correctly where this is likely to happen without a disk fault, ie. if the
  transaction was too large to be written back safely.
* Improved "bfs_journal" command output.


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

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

@@ -690,10 +690,9 @@ Journal::_WriteTransactionToLog()
 				fVolume->BlockCache(), fTransactionID) < (int32)fLogSize) {
 			detached = true;
 		} else {
-			// TODO: what are our options here?
+			// We created a transaction larger than one we can write back to
-			// a) abort the transaction - bad, because all changes are lost
+			// disk - the only option we have (besides risking disk corruption
-			// b) carry out the changes, but don't use the log - even worse,
+			// by writing it back anyway), is to let it fail.
-			//    as it potentially creates a corrupted disk.
 			dprintf("transaction too large (%d blocks, %d main, log size %d)!\n",
 				(int)_TransactionSize(), (int)cache_blocks_in_main_transaction(
 				fVolume->BlockCache(), fTransactionID), (int)fLogSize);
@@ -959,15 +958,18 @@ Journal::Lock(Transaction* owner, bool separateSubTransactions)
 }
 
 
-void
+status_t
 Journal::Unlock(Transaction* owner, bool success)
 {
 	if (fSeparateSubTransactions || recursive_lock_get_recursion(&fLock) == 1) {
 		// we only end the transaction if we would really unlock it
 		// TODO: what about failing transactions that do not unlock?
 		// (they must make the parent fail, too)
-		if (fOwner != NULL)
+		if (fOwner != NULL) {
-			_TransactionDone(success);
+			status_t status = _TransactionDone(success);
+			if (status != B_OK)
+				return status;
+		}
 
 		fTimestamp = system_time();
 		fOwner = NULL;
@@ -978,6 +980,7 @@ Journal::Unlock(Transaction* owner, bool success)
 	}
 
 	recursive_lock_unlock(&fLock);
+	return B_OK;
 }
 
 
@@ -1021,7 +1024,11 @@ Journal::_TransactionDone(bool success)
 		return B_OK;
 	}
 
-	return _WriteTransactionToLog();
+	status_t status = _WriteTransactionToLog();
+	if (status != B_OK)
+		fUnwrittenTransactions++;
+
+	return status;
 }
 
 
@@ -1033,8 +1040,18 @@ Journal::_TransactionDone(bool success)
 void
 Journal::Dump()
 {
-	kprintf("log start: %ld\n", fVolume->LogStart());
+	kprintf("Journal %p\n", this);
-	kprintf("log end:   %ld\n", fVolume->LogEnd());
+	kprintf("  log start:            %ld\n", fVolume->LogStart());
+	kprintf("  log end:              %ld\n", fVolume->LogEnd());
+	kprintf("  owner:                %p\n", fOwner);
+	kprintf("  log size:             %lu\n", fLogSize);
+	kprintf("  max transaction size: %lu\n", fMaxTransactionSize);
+	kprintf("  used:                 %lu\n", fUsed);
+	kprintf("  unwritten:            %ld\n", fUnwrittenTransactions);
+	kprintf("  timestamp:            %lld\n", fTimestamp);
+	kprintf("  transaction ID:       %ld\n", fTransactionID);
+	kprintf("  has subtransaction:   %d\n", fHasSubtransaction);
+	kprintf("  separate sub-trans.:  %d\n", fSeparateSubTransactions);
 	kprintf("entries:\n");
 	kprintf("  address        id  start length\n");
 

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

@@ -27,7 +27,7 @@ public:
 
 			status_t		Lock(Transaction* owner,
 								bool separateSubTransactions);
-			void			Unlock(Transaction* owner, bool success);
+			status_t		Unlock(Transaction* owner, bool success);
 
 			status_t		ReplayLog();
 
@@ -115,13 +115,16 @@ public:
 	status_t Start(Volume* volume, off_t refBlock);
 	bool IsStarted() const { return fJournal != NULL; }
 
-	void Done()
+	status_t Done()
 	{
+		status_t status = B_OK;
 		if (fJournal != NULL) {
 			_UnlockInodes();
-			fJournal->Unlock(this, true);
+			status = fJournal->Unlock(this, true);
+			if (status == B_OK)
+				fJournal = NULL;
 		}
-		fJournal = NULL;
+		return status;
 	}
 
 	bool HasParent()

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

@@ -359,10 +359,10 @@ bfs_remove_vnode(fs_volume* _volume, fs_vnode* _node, bool reenter)
 	// transaction which has already deleted the inode.
 	Transaction transaction(volume, volume->ToBlock(inode->Parent()));
 
-	// The "chkbfs" functionality uses this flag to prevent the space used
+	// The file system check functionality uses this flag to prevent the space
-	// up by the inode from being freed - this flag is set only in situations
+	// used up by the inode from being freed - this flag is set only in
-	// where this is a good idea... (the block bitmap will get fixed anyway
+	// situations where this does not cause any harm as the block bitmap will
-	// in this case).
+	// get fixed anyway in this case).
 	if ((inode->Flags() & INODE_DONT_FREE_SPACE) != 0) {
 		delete inode;
 		return B_OK;
@@ -370,14 +370,15 @@ bfs_remove_vnode(fs_volume* _volume, fs_vnode* _node, bool reenter)
 
 	status_t status = inode->Free(transaction);
 	if (status == B_OK) {
-		transaction.Done();
+		status = transaction.Done();
-
-		delete inode;
 	} else if (transaction.HasParent()) {
 		// TODO: for now, we don't let sub-transactions fail
-		transaction.Done();
+		status = transaction.Done();
 	}
 
+	// TODO: the VFS currently does not allow this to fail
+	delete inode;
+
 	return status;
 }
 
@@ -777,7 +778,7 @@ bfs_write_stat(fs_volume* _volume, fs_vnode* _node, const struct stat* stat,
 			off_t oldSize = inode->Size();
 
 			status = inode->SetFileSize(transaction, stat->st_size);
-			if (status < B_OK)
+			if (status != B_OK)
 				return status;
 
 			// fill the new blocks (if any) with zeros
@@ -843,9 +844,9 @@ bfs_write_stat(fs_volume* _volume, fs_vnode* _node, const struct stat* stat,
 
 	status = inode->WriteBack(transaction);
 	if (status == B_OK)
-		transaction.Done();
+		status = transaction.Done();
-
+	if (status == B_OK)
-	notify_stat_changed(volume->ID(), inode->ID(), mask);
+		notify_stat_changed(volume->ID(), inode->ID(), mask);
 
 	return status;
 }
@@ -891,10 +892,11 @@ bfs_create(fs_volume* _volume, fs_vnode* _directory, const char* name,
 		status = file_cache_disable(inode->FileCache());
 	}
 
-	if (status >= B_OK) {
+	if (status == B_OK)
-		entry_cache_add(volume->ID(), directory->ID(), name, *_vnodeID);
+		status = transaction.Done();
 
-		transaction.Done();
+	if (status == B_OK) {
+		entry_cache_add(volume->ID(), directory->ID(), name, *_vnodeID);
 
 		// register the cookie
 		*_cookie = cookie;
@@ -1017,7 +1019,8 @@ status_t
 bfs_rename(fs_volume* _volume, fs_vnode* _oldDir, const char* oldName,
 	fs_vnode* _newDir, const char* newName)
 {
-	FUNCTION_START(("oldDir = %p, oldName = \"%s\", newDir = %p, newName = \"%s\"\n", _oldDir, oldName, _newDir, newName));
+	FUNCTION_START(("oldDir = %p, oldName = \"%s\", newDir = %p, newName = "
+		"\"%s\"\n", _oldDir, oldName, _newDir, newName));
 
 	// there might be some more tests needed?!
 	if (!strcmp(oldName, ".") || !strcmp(oldName, "..")
@@ -1236,14 +1239,12 @@ bfs_open(fs_volume* _volume, fs_vnode* _node, int openMode, void** _cookie)
 		inode->WriteLockInTransaction(transaction);
 
 		status_t status = inode->SetFileSize(transaction, 0);
-		if (status < B_OK)
+		if (status == B_OK)
+			status = inode->WriteBack(transaction);
+		if (status == B_OK)
+			status = transaction.Done();
+		if (status != B_OK)
 			return status;
-
-		status = inode->WriteBack(transaction);
-		if (status < B_OK)
-			return status;
-
-		transaction.Done();
 	}
 
 	fileCacheEnabler.Detach();