* There is now a list of removed vnodes in Volume - while asking the VFS for

this seemed to be a good idea, there is one race condition that cannot be
  solved otherwise (the vnode must be added/removed to that list while holding
  the transaction lock, and we cannot guarantee that in the VFS).
* We are using an unused area of the in-memory bfs_inode to store the list
  links (bfs_inode::pad - this will also work on 64 bit platforms).
* Inode no longer adds a singly linked list link - the transaction list now
  shares the doubly linked list with the removed vnodes list.
* Added an in-memory flag INODE_IN_TRANSACTION to avoid searching an inode
  to be added in the list.
* Removing an attribute directory did not hold its write lock.
* If removing an attribute failed for some reason, the INODE_DELETED flag
  was not removed (the transaction would not have failed because of that).


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

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

@@ -1147,7 +1147,7 @@ BlockAllocator::StartChecking(check_control* control)
 		// Lock the volume's journal
 
 	status_t status = mutex_lock(&fLock);
-	if (status < B_OK) {
+	if (status != B_OK) {
 		fVolume->GetJournal(0)->Unlock(NULL, true);
 		return status;
 	}
@@ -1185,16 +1185,12 @@ BlockAllocator::StartChecking(check_control* control)
 	fCheckCookie = cookie;
 		// to be able to restore nicely if "chkbfs" exited abnormally
 
-#if !BFS_SHELL
 	// Put removed vnodes to the stack -- they are not reachable by traversing
 	// the file system anymore.
-	void* inode = NULL;
+	InodeList::Iterator iterator = fVolume->RemovedInodes().GetIterator();
-	ino_t nodeID;
+	while (Inode* inode = iterator.Next()) {
-	while (get_next_removed_vnode(fVolume->FSVolume(), &nodeID,	 &inode)
+		cookie->stack.Push(inode->BlockRun());
-			== B_OK) {
-		cookie->stack.Push(fVolume->ToBlockRun(nodeID));
 	}
-#endif
 
 	// TODO: check reserved area in bitmap!
 

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

@@ -127,7 +127,6 @@ dump_inode(const bfs_inode* inode)
 	dump_block_run(	"  attributes         = ", inode->attributes);
 	Print("  type               = %u\n", (unsigned)inode->Type());
 	Print("  inode_size         = %u\n", (unsigned)inode->InodeSize());
-	Print("  etc                = %#08x\n", (int)inode->etc);
 	Print("  short_symlink      = %s\n",
 		S_ISLNK(inode->Mode()) && (inode->Flags() & INODE_LONG_SYMLINK) == 0
 			? inode->short_symlink : "-");

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

@@ -335,6 +335,7 @@ Inode::Inode(Volume* volume, ino_t id)
 
 	NodeGetter node(volume, this);
 	memcpy(&fNode, node.Node(), sizeof(bfs_inode));
+	fNode.flags &= HOST_ENDIAN_TO_BFS_INT32(INODE_PERMANENT_FLAGS);
 
 	char lockName[B_OS_NAME_LENGTH];
 	snprintf(lockName, sizeof(lockName), "bfs inode %d.%d",
@@ -411,6 +412,9 @@ Inode::~Inode()
 	file_map_delete(Map());
 	delete fTree;
 
+	if ((Flags() & INODE_DELETED) != 0)
+		fVolume->RemovedInodes().Remove(this);
+
 	rw_lock_destroy(&fLock);
 }
 
@@ -424,7 +428,7 @@ Inode::InitCheck(bool checkNode)
 		if (status == B_BUSY)
 			return B_BUSY;
 
-		if (status < B_OK) {
+		if (status != B_OK) {
 			FATAL(("inode at block %Ld corrupt!\n", BlockNumber()));
 			RETURN_ERROR(B_BAD_DATA);
 		}
@@ -436,7 +440,7 @@ Inode::InitCheck(bool checkNode)
 			RETURN_ERROR(B_NO_MEMORY);
 
 		status_t status = fTree->InitCheck();
-		if (status < B_OK) {
+		if (status != B_OK) {
 			FATAL(("inode tree at block %Ld corrupt!\n", BlockNumber()));
 			RETURN_ERROR(B_BAD_DATA);
 		}
@@ -551,7 +555,7 @@ Inode::_MakeSpaceForSmallData(Transaction& transaction, bfs_inode* node,
 		Inode* attribute;
 		status_t status = CreateAttribute(transaction, item->Name(),
 			item->Type(), &attribute);
-		if (status < B_OK)
+		if (status != B_OK)
 			RETURN_ERROR(status);
 
 		size_t length = item->DataSize();
@@ -559,7 +563,7 @@ Inode::_MakeSpaceForSmallData(Transaction& transaction, bfs_inode* node,
 
 		ReleaseAttribute(attribute);
 
-		if (status < B_OK) {
+		if (status != B_OK) {
 			Vnode vnode(fVolume, Attributes());
 			Inode* attributes;
 			if (vnode.Get(&attributes) < B_OK
@@ -947,6 +951,8 @@ Inode::_RemoveAttribute(Transaction& transaction, const char* name,
 		return status;
 
 	if (attributes->IsEmpty()) {
+		attributes->WriteLockInTransaction(transaction);
+
 		// remove attribute directory (don't fail if that can't be done)
 		if (remove_vnode(fVolume->FSVolume(), attributes->ID()) == B_OK) {
 			// update the inode, so that no one will ever doubt it's deleted :-)
@@ -954,8 +960,11 @@ Inode::_RemoveAttribute(Transaction& transaction, const char* name,
 			if (attributes->WriteBack(transaction) == B_OK) {
 				Attributes().SetTo(0, 0, 0);
 				WriteBack(transaction);
-			} else
+			} else {
 				unremove_vnode(fVolume->FSVolume(), attributes->ID());
+				attributes->Node().flags
+					&= ~HOST_ENDIAN_TO_BFS_INT32(INODE_DELETED);
+			}
 		}
 	}
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2008, Axel Dörfler, [email protected].
+ * Copyright 2001-2009, Axel Dörfler, [email protected].
  * This file may be used under the terms of the MIT License.
  */
 #ifndef INODE_H
@@ -24,7 +24,9 @@ class NodeGetter;
 class Transaction;
 
 
-class Inode : public SinglyLinkedListLinkImpl<Inode> {
+class Inode {
+	typedef DoublyLinkedListLink<Inode> Link;
+
 public:
 							Inode(Volume* volume, ino_t id);
 							Inode(Volume* volume, Transaction& transaction,
@@ -174,6 +176,11 @@ public:
 								{ ASSERT_WRITE_LOCKED_RW_LOCK(&fLock); }
 #endif
 
+			Link*			GetDoublyLinkedListLink()
+								{ return (Link*)&fNode.pad[0]; }
+			const Link*		GetDoublyLinkedListLink() const
+								{ return (Link*)&fNode.pad[0]; }
+
 private:
 							Inode(const Inode& other);
 							Inode& operator=(const Inode& other);

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2008, Axel Dörfler, [email protected].
+ * Copyright 2001-2009, Axel Dörfler, [email protected].
  * This file may be used under the terms of the MIT License.
  */
 
@@ -1091,18 +1091,21 @@ Transaction::AddInode(Inode* inode)
 	if (fJournal == NULL)
 		panic("Transaction is not running!");
 
-	InodeList::Iterator iterator = fLockedInodes.GetIterator();
+	// These flags can only change while holding the transaction lock
-	while (iterator.HasNext()) {
+	if ((inode->Flags() & INODE_IN_TRANSACTION) != 0)
-		if (iterator.Next() == inode) {
+		return;
-			//dprintf("  inode %Ld already in transaction\n", inode->ID());
+
-			return;
+	// We share the same list link with the removed list, so we have to remove
-		}
+	// the inode from that list here (and add it back when we no longer need it)
-	}
+	if ((inode->Flags() & INODE_DELETED) != 0)
+		GetVolume()->RemovedInodes().Remove(inode);
 
 	if (!GetVolume()->IsInitializing())
 		acquire_vnode(GetVolume()->FSVolume(), inode->ID());
+
 	rw_lock_write_lock(&inode->fLock);
 	fLockedInodes.Add(inode);
+	inode->Node().flags |= HOST_ENDIAN_TO_BFS_INT32(INODE_IN_TRANSACTION);
 }
 
 
@@ -1112,8 +1115,14 @@ Transaction::RemoveInode(Inode* inode)
 	if (fJournal == NULL)
 		panic("Transaction is not running!");
 
+	inode->Node().flags &= ~HOST_ENDIAN_TO_BFS_INT32(INODE_IN_TRANSACTION);
 	fLockedInodes.Remove(inode);
 	rw_lock_write_unlock(&inode->fLock);
+
+	// See AddInode() why we do this here
+	if ((inode->Flags() & INODE_DELETED) != 0)
+		GetVolume()->RemovedInodes().Add(inode);
+
 	if (!GetVolume()->IsInitializing())
 		put_vnode(GetVolume()->FSVolume(), inode->ID());
 }
@@ -1123,7 +1132,13 @@ void
 Transaction::_UnlockInodes()
 {
 	while (Inode* inode = fLockedInodes.RemoveHead()) {
+		inode->Node().flags &= ~HOST_ENDIAN_TO_BFS_INT32(INODE_IN_TRANSACTION);
 		rw_lock_write_unlock(&inode->fLock);
+
+		// See AddInode() why we do this here
+		if ((inode->Flags() & INODE_DELETED) != 0)
+			GetVolume()->RemovedInodes().Add(inode);
+
 		if (!GetVolume()->IsInitializing())
 			put_vnode(GetVolume()->FSVolume(), inode->ID());
 	}

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

@@ -16,7 +16,6 @@ struct run_array;
 class Inode;
 class LogEntry;
 typedef DoublyLinkedList<LogEntry> LogEntryList;
-typedef SinglyLinkedList<Inode> InodeList;
 
 
 class Journal {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2008, Axel Dörfler, [email protected].
+ * Copyright 2001-2009, Axel Dörfler, [email protected].
  * This file may be used under the terms of the MIT License.
  */
 #ifndef VOLUME_H
@@ -23,6 +23,8 @@ enum volume_initialize_flags {
 	VOLUME_NO_INDICES	= 0x0001,
 };
 
+typedef DoublyLinkedList<Inode> InodeList;
+
 class Volume {
 public:
 							Volume(fs_volume* volume);
@@ -86,6 +88,9 @@ public:
 
 			status_t		CreateIndicesRoot(Transaction& transaction);
 
+			InodeList&		RemovedInodes() { return fRemovedInodes; }
+				// This list is guarded by the transaction lock
+
 			// block bitmap
 			BlockAllocator&	Allocator();
 			status_t		AllocateForInode(Transaction& transaction,
@@ -127,7 +132,7 @@ public:
 								uint32* _offset = NULL);
 	static	status_t		Identify(int fd, disk_super_block* superBlock);
 
-	protected:
+protected:
 			fs_volume*		fVolume;
 			int				fDevice;
 			disk_super_block fSuperBlock;
@@ -155,6 +160,8 @@ public:
 
 			void*			fBlockCache;
 			thread_id		fCheckingThread;
+
+			InodeList		fRemovedInodes;
 };
 
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2008, Axel Dörfler, [email protected].
+ * Copyright 2001-2009, Axel Dörfler, [email protected].
  * Parts of this code is based on work previously done by Marcus Overhagen.
  *
  * This file may be used under the terms of the MIT License.
@@ -165,7 +165,8 @@ struct small_data {
 
 class Volume;
 
-#define SHORT_SYMLINK_NAME_LENGTH	144 // length incl. terminating '\0'
+#define SHORT_SYMLINK_NAME_LENGTH	144
+	// length incl. terminating '\0'
 
 struct bfs_inode {
 	int32		magic1;
@@ -181,13 +182,14 @@ struct bfs_inode {
 	uint32		type;				// attribute type
 
 	int32		inode_size;
-	uint32		etc;				// a pointer to the Inode object during construction
+	uint32		etc;
 
 	union {
 		data_stream		data;
 		char 			short_symlink[SHORT_SYMLINK_NAME_LENGTH];
 	};
 	int32		pad[4];
+		// we use this member as a doubly linked list link
 
 	small_data	small_data_start[0];
 
@@ -222,6 +224,8 @@ enum inode_flags {
 	INODE_PERMANENT_FLAGS	= 0x0000ffff,
 
 	INODE_WAS_WRITTEN		= 0x00020000,
+	INODE_IN_TRANSACTION	= 0x00040000,
+
 	// The rest is only used by the file system check functionality
 	INODE_DONT_FREE_SPACE	= 0x00080000
 };

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

@@ -266,7 +266,7 @@ bfs_get_vnode(fs_volume* _volume, ino_t id, fs_vnode* _node, int* _type,
 	}
 
 	status_t status = node->InitCheck(volume);
-	if (status < B_OK) {
+	if (status != B_OK) {
 		if ((node->Flags() & INODE_DELETED) != 0) {
 			INFORM(("inode at %Ld is already deleted!\n", id));
 		} else {
@@ -281,7 +281,7 @@ bfs_get_vnode(fs_volume* _volume, ino_t id, fs_vnode* _node, int* _type,
 		return B_NO_MEMORY;
 
 	status = inode->InitCheck(false);
-	if (status < B_OK)
+	if (status != B_OK)
 		delete inode;
 
 	if (status == B_OK) {
@@ -328,20 +328,20 @@ bfs_remove_vnode(fs_volume* _volume, fs_vnode* _node, bool reenter)
 	Volume* volume = (Volume*)_volume->private_volume;
 	Inode* inode = (Inode*)_node->private_node;
 
-	// The "chkbfs" functionality uses this flag to prevent the space used
-	// up by the inode from being freed - this flag is set only in situations
-	// where this is a good idea... (the block bitmap will get fixed anyway
-	// in this case).
-	if (inode->Flags() & INODE_DONT_FREE_SPACE) {
-		delete inode;
-		return B_OK;
-	}
-
 	// If the inode isn't in use anymore, we were called before
 	// bfs_unlink() returns - in this case, we can just use the
 	// 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
+	// up by the inode from being freed - this flag is set only in situations
+	// where this is a good idea... (the block bitmap will get fixed anyway
+	// in this case).
+	if ((inode->Flags() & INODE_DONT_FREE_SPACE) != 0) {
+		delete inode;
+		return B_OK;
+	}
+
 	status_t status = inode->Free(transaction);
 	if (status == B_OK) {
 		transaction.Done();