packagefs: Make directory nodes rw_locked instead of child entries.

This saves 40 bytes from the size of Node (on 64-bit architectures.)
UnpackingDirectory is still at 200 bytes, while UnpackingLeafNode
is now 96 instead of 136. This saves ~5MB of memory on my system
(UnpackingLeafNodes go from 16.2MB to 11.2MB.)

The general strategy is for Directories to use their own locks, while
all other nodes read-lock their parent directory during use. There are
a few edge cases around node creation and removal in the case of
non-directory nodes; see inline comments in Volume's
_RemoveNodeAndVNode as well as packagefs_put_vnode.

Since it's now possible for a node's parent to change or be deleted
when we don't have the lock (but only a reference), we need a lock
protecting just that field to hold while we acquire a reference to
the parent. (Right now, this is just one static rw_lock for all Nodes;
this could be changed in the future if necessary, but it seems performant
enough for the moment.)

Tested with basic system functionality, installing/uninstalling packages,
uninstalling packages with files still in use, HaikuPorter builds, and
more. All still seems to work as expected.

Change-Id: I054187316c66b77ea1951c6d1ea8e5b75715c082
Reviewed-on: https://review.haiku-os.org/c/haiku/+/7930
Tested-by: Commit checker robot <no-reply+buildbot@haiku-os.org>
Reviewed-by: waddlesplash <waddlesplash@gmail.com>

src/add-ons/kernel/file_systems/packagefs/indices/Query.cpp

@@ -52,7 +52,7 @@ struct Query::QueryPolicy {
 
 	static ino_t EntryGetParentID(Entry* entry)
 	{
-		return entry->Parent()->ID();
+		return entry->GetParentUnchecked()->ID();
 	}
 
 	static Node* EntryGetNode(Entry* entry)

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

@@ -41,6 +41,22 @@ static const uint32 kOptimalIOSize = 64 * 1024;
 // #pragma mark - helper functions
 
 
+static bool
+lock_directory_for_node(Volume* volume, Node* node, DirectoryReadLocker& locker)
+{
+	if (Directory* directory = dynamic_cast<Directory*>(node)) {
+		locker.SetTo(directory, false, true);
+		return locker.IsLocked();
+	} else {
+		BReference<Directory> parentRef = node->GetParent();
+		if (!parentRef.IsSet())
+			return false;
+		locker.SetTo(parentRef.Get(), false, true);
+		return locker.IsLocked() && node->GetParentUnchecked() == locker.Get();
+	}
+}
+
+
 static status_t
 check_access(Node* node, int mode)
 {
@@ -76,6 +92,7 @@ packagefs_mount(fs_volume* fsVolume, const char* device, uint32 flags,
 
 	status_t error = volume->Mount(parameters);
 	if (error != B_OK) {
+		volumeWriteLocker.Unlock();
 		delete volume;
 		return error;
 	}
@@ -128,40 +145,42 @@ packagefs_lookup(fs_volume* fsVolume, fs_vnode* fsDir, const char* entryName,
 	ino_t* _vnid)
 {
 	Volume* volume = (Volume*)fsVolume->private_volume;
-	Node* dir = (Node*)fsDir->private_node;
+	Node* node = (Node*)fsDir->private_node;
 
 	FUNCTION("volume: %p, dir: %p (%" B_PRId64 "), entry: \"%s\"\n", volume,
 		dir, dir->ID(), entryName);
 
-	if (!S_ISDIR(dir->Mode()))
+	if (!S_ISDIR(node->Mode()))
 		return B_NOT_A_DIRECTORY;
 
+	Directory* dir = dynamic_cast<Directory*>(node);
+
 	// resolve "."
 	if (strcmp(entryName, ".") == 0) {
-		Node* node;
+		Node* self;
 		*_vnid = dir->ID();
-		return volume->GetVNode(*_vnid, node);
+		return volume->GetVNode(*_vnid, self);
 	}
 
 	// resolve ".."
 	if (strcmp(entryName, "..") == 0) {
-		Node* node;
-		*_vnid = dir->Parent()->ID();
-		return volume->GetVNode(*_vnid, node);
+		Node* parent;
+		*_vnid = dir->GetParentUnchecked()->ID();
+		return volume->GetVNode(*_vnid, parent);
 	}
 
 	// resolve normal entries -- look up the node
-	NodeReadLocker dirLocker(dir);
+	DirectoryReadLocker dirLocker(dir);
 	String entryNameString;
-	Node* node = dynamic_cast<Directory*>(dir)->FindChild(StringKey(entryName));
-	if (node == NULL)
+	Node* child = dir->FindChild(StringKey(entryName));
+	if (child == NULL)
 		return B_ENTRY_NOT_FOUND;
-	BReference<Node> nodeReference(node);
+	BReference<Node> childReference(child);
 	dirLocker.Unlock();
 
 	// get the vnode reference
-	*_vnid = node->ID();
-	RETURN_ERROR(volume->GetVNode(*_vnid, node));
+	*_vnid = child->ID();
+	RETURN_ERROR(volume->GetVNode(*_vnid, child));
 }
 
 
@@ -194,13 +213,23 @@ packagefs_get_vnode(fs_volume* fsVolume, ino_t vnid, fs_vnode* fsNode,
 	if (node == NULL)
 		return B_ENTRY_NOT_FOUND;
 	BReference<Node> nodeReference(node);
+
+	DirectoryWriteLocker dirLocker;
+	if (Directory* directory = dynamic_cast<Directory*>(node)) {
+		dirLocker.SetTo(directory, false, true);
+		if (!dirLocker.IsLocked())
+			return B_NO_INIT;
+	} else {
+		dirLocker.SetTo(node->GetParentUnchecked(), false, true);
+		if (!dirLocker.IsLocked())
+			return B_NO_INIT;
+	}
 	volumeLocker.Unlock();
 
-	NodeWriteLocker nodeLocker(node);
 	status_t error = node->VFSInit(volume->ID());
 	if (error != B_OK)
 		RETURN_ERROR(error);
-	nodeLocker.Unlock();
+	dirLocker.Unlock();
 
 	fsNode->private_node = nodeReference.Detach();
 	fsNode->ops = &gPackageFSVnodeOps;
@@ -220,9 +249,24 @@ packagefs_put_vnode(fs_volume* fsVolume, fs_vnode* fsNode, bool reenter)
 	FUNCTION("volume: %p, node: %p\n", volume, node);
 	TOUCH(volume);
 
-	NodeWriteLocker nodeLocker(node);
+	VolumeReadLocker volumeLocker(volume);
+	DirectoryWriteLocker dirLocker;
+	if (Directory* directory = dynamic_cast<Directory*>(node)) {
+		dirLocker.SetTo(directory, false, true);
+		ASSERT(dirLocker.IsLocked());
+	} else {
+		dirLocker.SetTo(node->GetParentUnchecked(), false, true);
+		if (dirLocker.Get() == NULL) {
+			// This node does not have a parent. This should only happen during
+			// removal, in which case we should either have the Volume write lock,
+			// or the node should have only one reference remaining.
+			ASSERT(volume->IsWriteLocked() || node->CountReferences() == 1);
+		}
+	}
+	volumeLocker.Unlock();
+
 	node->VFSUninit();
-	nodeLocker.Unlock();
+	dirLocker.Unlock();
 
 	node->ReleaseReference();
 
@@ -283,7 +327,9 @@ packagefs_read_symlink(fs_volume* fsVolume, fs_vnode* fsNode, char* buffer,
 		node->ID());
 	TOUCH(volume);
 
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
 
 	if (!S_ISLNK(node->Mode()))
 		return B_BAD_VALUE;
@@ -302,7 +348,10 @@ packagefs_access(fs_volume* fsVolume, fs_vnode* fsNode, int mode)
 		node->ID());
 	TOUCH(volume);
 
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
+
 	return check_access(node, mode);
 }
 
@@ -317,7 +366,9 @@ packagefs_read_stat(fs_volume* fsVolume, fs_vnode* fsNode, struct stat* st)
 		node->ID());
 	TOUCH(volume);
 
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
 
 	st->st_mode = node->Mode();
 	st->st_nlink = 1;
@@ -361,7 +412,9 @@ packagefs_open(fs_volume* fsVolume, fs_vnode* fsNode, int openMode,
 		volume, node, node->ID(), openMode);
 	TOUCH(volume);
 
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
 
 	// check the open mode and permissions
 	if (S_ISDIR(node->Mode()) && (openMode & O_RWMASK) != O_RDONLY)
@@ -482,7 +535,7 @@ struct DirectoryCookie : DirectoryIterator {
 	{
 		if (state == 0) {
 			state = 1;
-			node = directory->Parent();
+			node = directory->GetParentUnchecked();
 			if (node == NULL)
 				node = directory;
 			return node;
@@ -535,7 +588,7 @@ packagefs_open_dir(fs_volume* fsVolume, fs_vnode* fsNode, void** _cookie)
 		return error;
 
 	// create a cookie
-	NodeWriteLocker dirLocker(dir);
+	DirectoryWriteLocker dirLocker(dir);
 	DirectoryCookie* cookie = new(std::nothrow) DirectoryCookie(dir);
 	if (cookie == NULL)
 		RETURN_ERROR(B_NO_MEMORY);
@@ -564,7 +617,7 @@ packagefs_free_dir_cookie(fs_volume* fsVolume, fs_vnode* fsNode, void* _cookie)
 	TOUCH(volume);
 	TOUCH(node);
 
-	NodeWriteLocker dirLocker(node);
+	DirectoryWriteLocker dirLocker(dynamic_cast<Directory*>(node));
 	delete cookie;
 
 	return B_OK;
@@ -584,7 +637,7 @@ packagefs_read_dir(fs_volume* fsVolume, fs_vnode* fsNode, void* _cookie,
 	TOUCH(volume);
 	TOUCH(node);
 
-	NodeWriteLocker dirLocker(cookie->directory);
+	DirectoryWriteLocker dirLocker(cookie->directory);
 
 	uint32 maxCount = *_count;
 	uint32 count = 0;
@@ -648,7 +701,7 @@ packagefs_rewind_dir(fs_volume* fsVolume, fs_vnode* fsNode, void* _cookie)
 	TOUCH(volume);
 	TOUCH(node);
 
-	NodeWriteLocker dirLocker(node);
+	DirectoryWriteLocker dirLocker(dynamic_cast<Directory*>(node));
 	cookie->Rewind();
 
 	return B_OK;
@@ -673,7 +726,10 @@ packagefs_open_attr_dir(fs_volume* fsVolume, fs_vnode* fsNode, void** _cookie)
 		return error;
 
 	// create a cookie
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
+
 	AttributeDirectoryCookie* cookie;
 	error = node->OpenAttributeDirectory(cookie);
 	if (error != B_OK)
@@ -758,7 +814,9 @@ packagefs_open_attr(fs_volume* fsVolume, fs_vnode* fsNode, const char* name,
 			"%#x\n", volume, node, node->ID(), name, openMode);
 	TOUCH(volume);
 
-	NodeReadLocker nodeLocker(node);
+	DirectoryReadLocker dirLocker;
+	if (!lock_directory_for_node(volume, node, dirLocker))
+		return B_NO_INIT;
 
 	// check the open mode and permissions
 	if ((openMode & O_RWMASK) != O_RDONLY)

src/add-ons/kernel/file_systems/packagefs/nodes/Directory.cpp

@@ -16,6 +16,7 @@ Directory::Directory(ino_t id)
 	:
 	Node(id)
 {
+	rw_lock_init(&fLock, "packagefs directory");
 }
 
 
@@ -24,9 +25,12 @@ Directory::~Directory()
 	Node* child = fChildTable.Clear(true);
 	while (child != NULL) {
 		Node* next = child->NameHashTableNext();
+		child->_SetParent(NULL);
 		child->ReleaseReference();
 		child = next;
 	}
+
+	rw_lock_destroy(&fLock);
 }
 
 
@@ -80,7 +84,7 @@ void
 Directory::AddChild(Node* node)
 {
 	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
-	ASSERT(node->Parent() == NULL);
+	ASSERT(node->fParent == NULL);
 
 	fChildTable.Insert(node);
 	fChildList.Add(node);
@@ -93,7 +97,7 @@ void
 Directory::RemoveChild(Node* node)
 {
 	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
-	ASSERT(node->Parent() == this);
+	ASSERT(node->fParent == this);
 
 	Node* nextNode = fChildList.GetNext(node);
 

src/add-ons/kernel/file_systems/packagefs/nodes/Directory.h

@@ -8,6 +8,9 @@
 
 #include "Node.h"
 
+#include <lock.h>
+#include <AutoLocker.h>
+
 
 struct DirectoryIterator : DoublyLinkedListLinkImpl<DirectoryIterator> {
 	Node*	node;
@@ -27,6 +30,11 @@ public:
 								Directory(ino_t id);
 	virtual						~Directory();
 
+	inline	bool				ReadLock();
+	inline	void				ReadUnlock();
+	inline	bool				WriteLock();
+	inline	void				WriteUnlock();
+
 	virtual	status_t			Init(const String& name);
 
 	virtual	mode_t				Mode() const;
@@ -50,6 +58,9 @@ public:
 			void				RemoveDirectoryIterator(
 									DirectoryIterator* iterator);
 
+protected:
+			rw_lock				fLock;
+
 private:
 			NodeNameHashTable	fChildTable;
 			NodeList			fChildList;
@@ -57,6 +68,34 @@ private:
 };
 
 
+bool
+Directory::ReadLock()
+{
+	return rw_lock_read_lock(&fLock) == B_OK;
+}
+
+
+void
+Directory::ReadUnlock()
+{
+	rw_lock_read_unlock(&fLock);
+}
+
+
+bool
+Directory::WriteLock()
+{
+	return rw_lock_write_lock(&fLock) == B_OK;
+}
+
+
+void
+Directory::WriteUnlock()
+{
+	rw_lock_write_unlock(&fLock);
+}
+
+
 Node*
 Directory::FirstChild() const
 {
@@ -71,4 +110,8 @@ Directory::NextChild(Node* node) const
 }
 
 
+typedef AutoLocker<Directory, AutoLockerReadLocking<Directory> > DirectoryReadLocker;
+typedef AutoLocker<Directory, AutoLockerWriteLocking<Directory> > DirectoryWriteLocker;
+
+
 #endif	// DIRECTORY_H

src/add-ons/kernel/file_systems/packagefs/nodes/Node.cpp

@@ -9,10 +9,17 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include <AutoLocker.h>
+#include <lock.h>
+
 #include "DebugSupport.h"
+#include "Directory.h"
 #include "EmptyAttributeDirectoryCookie.h"
 
 
+static rw_lock sParentChangeLock = RW_LOCK_INITIALIZER("packagefs node parent change");
+
+
 DEFINE_INLINE_REFERENCEABLE_METHODS(Node, fReferenceable);
 
 
@@ -23,13 +30,29 @@ Node::Node(ino_t id)
 	fName(),
 	fFlags(0)
 {
-	rw_lock_init(&fLock, "packagefs node");
 }
 
 
 Node::~Node()
 {
-	rw_lock_destroy(&fLock);
+}
+
+
+BReference<Directory>
+Node::GetParent() const
+{
+	ReadLocker parentChangeLocker(sParentChangeLock);
+	if (fParent == NULL)
+		return NULL;
+	return BReference<Directory>(fParent, false);
+}
+
+
+void
+Node::_SetParent(Directory* parent)
+{
+	WriteLocker parentChangeLocker(sParentChangeLock);
+	fParent = parent;
 }
 
 
@@ -49,13 +72,6 @@ Node::SetID(ino_t id)
 }
 
 
-void
-Node::_SetParent(Directory* parent)
-{
-	fParent = parent;
-}
-
-
 status_t
 Node::VFSInit(dev_t deviceID)
 {

src/add-ons/kernel/file_systems/packagefs/nodes/Node.h

@@ -8,9 +8,8 @@
 
 #include <fs_interface.h>
 
-#include <AutoLocker.h>
+#include <Referenceable.h>
 
-#include <lock.h>
 #include <util/DoublyLinkedList.h>
 #include <util/OpenHashTable.h>
 
@@ -43,13 +42,10 @@ public:
 			void				ReleaseReference();
 			int32				CountReferences();
 
-	inline	bool				ReadLock();
-	inline	void				ReadUnlock();
-	inline	bool				WriteLock();
-	inline	void				WriteUnlock();
+			BReference<Directory> GetParent() const;
+			Directory*			GetParentUnchecked() const	{ return fParent; }
 
 			ino_t				ID() const		{ return fID; }
-			Directory*			Parent() const	{ return fParent; }
 			const String&		Name() const	{ return fName; }
 
 			Node*&				NameHashTableNext()
@@ -96,7 +92,6 @@ private:
 			void				_SetParent(Directory* parent);
 
 protected:
-			rw_lock				fLock;
 			ino_t				fID;
 			Directory*			fParent;
 			String				fName;
@@ -107,34 +102,6 @@ protected:
 };
 
 
-bool
-Node::ReadLock()
-{
-	return rw_lock_read_lock(&fLock) == B_OK;
-}
-
-
-void
-Node::ReadUnlock()
-{
-	rw_lock_read_unlock(&fLock);
-}
-
-
-bool
-Node::WriteLock()
-{
-	return rw_lock_write_lock(&fLock) == B_OK;
-}
-
-
-void
-Node::WriteUnlock()
-{
-	rw_lock_write_unlock(&fLock);
-}
-
-
 bool
 Node::IsKnownToVFS() const
 {
@@ -208,8 +175,5 @@ typedef DoublyLinkedList<Node> NodeList;
 typedef BOpenHashTable<NodeNameHashDefinition> NodeNameHashTable;
 typedef BOpenHashTable<NodeIDHashDefinition> NodeIDHashTable;
 
-typedef AutoLocker<Node, AutoLockerReadLocking<Node> > NodeReadLocker;
-typedef AutoLocker<Node, AutoLockerWriteLocking<Node> > NodeWriteLocker;
-
 
 #endif	// NODE_H

src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinkDirectory.cpp

@@ -106,7 +106,7 @@ void
 PackageLinkDirectory::AddPackage(Package* package,
 	PackageLinksListener* listener)
 {
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 
 	// Find the insertion point in the list. We sort by mount type -- the more
 	// specific the higher the priority.
@@ -132,7 +132,7 @@ PackageLinkDirectory::RemovePackage(Package* package,
 {
 	ASSERT(package->LinkDirectory() == this);
 
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 
 	bool firstPackage = package == fPackages.Head();
 
@@ -150,7 +150,7 @@ PackageLinkDirectory::UpdatePackageDependencies(Package* package,
 {
 	ASSERT(package->LinkDirectory() == this);
 
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 
 	// We only need to update, if that head package is affected.
 	if (package != fPackages.Head())
@@ -201,6 +201,8 @@ PackageLinkDirectory::_Update(PackageLinksListener* listener)
 status_t
 PackageLinkDirectory::_UpdateDependencies(PackageLinksListener* listener)
 {
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
 	Package* package = fPackages.Head();
 	if (package == NULL)
 		return B_OK;
@@ -217,8 +219,6 @@ PackageLinkDirectory::_UpdateDependencies(PackageLinksListener* listener)
 		if (node != NULL) {
 			// link already exists -- update
 			DependencyLink* link = static_cast<DependencyLink*>(node);
-
-			NodeWriteLocker linkLocker(link);
 			link->Update(resolvablePackage, listener);
 		} else {
 			// no link for the dependency yet -- create one
@@ -236,10 +236,8 @@ PackageLinkDirectory::_UpdateDependencies(PackageLinksListener* listener)
 			AddChild(link);
 			fDependencyLinks.Add(link);
 
-			if (listener != NULL) {
-				NodeWriteLocker linkLocker(link);
+			if (listener != NULL)
 				listener->PackageLinkNodeAdded(link);
-			}
 		}
 	}
 
@@ -250,13 +248,13 @@ PackageLinkDirectory::_UpdateDependencies(PackageLinksListener* listener)
 void
 PackageLinkDirectory::_RemoveLink(Link* link, PackageLinksListener* listener)
 {
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
 	if (link != NULL) {
-		NodeWriteLocker linkLocker(link);
 		if (listener != NULL)
 			listener->PackageLinkNodeRemoved(link);
 
 		RemoveChild(link);
-		linkLocker.Unlock();
 		link->ReleaseReference();
 	}
 }
@@ -266,6 +264,8 @@ status_t
 PackageLinkDirectory::_CreateOrUpdateLink(Link*& link, Package* package,
 	Link::Type type, const String& name, PackageLinksListener* listener)
 {
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
 	if (link == NULL) {
 		link = new(std::nothrow) Link(package, type);
 		if (link == NULL)
@@ -277,15 +277,11 @@ PackageLinkDirectory::_CreateOrUpdateLink(Link*& link, Package* package,
 
 		AddChild(link);
 
-		if (listener != NULL) {
-			NodeWriteLocker lLinkLocker(link);
+		if (listener != NULL)
 			listener->PackageLinkNodeAdded(link);
-		}
 	} else {
-		NodeWriteLocker lLinkLocker(link);
 		link->Update(package, listener);
 	}
 
-
 	return B_OK;
 }

src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinksDirectory.cpp

@@ -53,7 +53,7 @@ PackageLinksDirectory::AddPackage(Package* package)
 		RETURN_ERROR(error);
 
 	// add the link directory
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 	if (Node* child = FindChild(linkDirectory->Name())) {
 		// There already is an entry with the name.
 		PackageLinkDirectory* otherLinkDirectory
@@ -64,16 +64,16 @@ PackageLinksDirectory::AddPackage(Package* package)
 		// There's already a package link directory. Delete the one we created
 		// and add the package to the pre-existing one.
 		linkDirectory->RemovePackage(package, NULL);
+		linkDirectoryReference.Unset();
+
 		linkDirectory = otherLinkDirectory;
 		linkDirectory->AddPackage(package, fListener);
 	} else {
 		// No entry is in the way, so just add the link directory.
 		AddChild(linkDirectory);
 
-		if (fListener != NULL) {
-			NodeWriteLocker writeLocker(linkDirectory);
+		if (fListener != NULL)
 			fListener->PackageLinkNodeAdded(linkDirectory);
-		}
 	}
 
 	return B_OK;
@@ -83,7 +83,7 @@ PackageLinksDirectory::AddPackage(Package* package)
 void
 PackageLinksDirectory::RemovePackage(Package* package)
 {
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 
 	// get the package's link directory and remove the package from it
 	PackageLinkDirectory* linkDirectory = package->LinkDirectory();
@@ -97,7 +97,7 @@ PackageLinksDirectory::RemovePackage(Package* package)
 	// if empty, remove the link directory itself
 	if (linkDirectory->IsEmpty()) {
 		if (fListener != NULL) {
-			NodeWriteLocker linkDirectoryWriteLocker(linkDirectory);
+			DirectoryWriteLocker linkDirectoryWriteLocker(linkDirectory);
 			fListener->PackageLinkNodeRemoved(linkDirectory);
 		}
 
@@ -109,7 +109,7 @@ PackageLinksDirectory::RemovePackage(Package* package)
 void
 PackageLinksDirectory::UpdatePackageDependencies(Package* package)
 {
-	NodeWriteLocker writeLocker(this);
+	DirectoryWriteLocker writeLocker(this);
 
 	PackageLinkDirectory* linkDirectory = package->LinkDirectory();
 	if (linkDirectory == NULL)

src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp

@@ -664,9 +664,11 @@ Volume::PublishVNode(Node* node)
 void
 Volume::PackageLinkNodeAdded(Node* node)
 {
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
 	_AddPackageLinksNode(node);
 
-	notify_entry_created(ID(), node->Parent()->ID(), node->Name(), node->ID());
+	notify_entry_created(ID(), node->GetParentUnchecked()->ID(), node->Name(), node->ID());
 	_NotifyNodeAdded(node);
 }
 
@@ -674,9 +676,11 @@ Volume::PackageLinkNodeAdded(Node* node)
 void
 Volume::PackageLinkNodeRemoved(Node* node)
 {
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
 	_RemovePackageLinksNode(node);
 
-	notify_entry_removed(ID(), node->Parent()->ID(), node->Name(), node->ID());
+	notify_entry_removed(ID(), node->GetParentUnchecked()->ID(), node->Name(), node->ID());
 	_NotifyNodeRemoved(node);
 }
 
@@ -685,7 +689,9 @@ void
 Volume::PackageLinkNodeChanged(Node* node, uint32 statFields,
 	const OldNodeAttributes& oldAttributes)
 {
-	Directory* parent = node->Parent();
+	ASSERT_WRITE_LOCKED_RW_LOCK(&fLock);
+
+	Directory* parent = node->GetParentUnchecked();
 	notify_stat_changed(ID(), parent != NULL ? parent->ID() : -1, node->ID(),
 		statFields);
 	_NotifyNodeChanged(node, statFields, oldAttributes);
@@ -1064,7 +1070,7 @@ Volume::_AddPackageContentRootNode(Package* package,
 			// unlock all directories
 			while (directory != NULL) {
 				directory->WriteUnlock();
-				directory = directory->Parent();
+				directory = directory->GetParentUnchecked();
 			}
 
 			// remove the added package nodes
@@ -1100,7 +1106,7 @@ Volume::_AddPackageContentRootNode(Package* package,
 			// no more siblings -- go back up the tree
 			packageNode = packageDirectory;
 			directory->WriteUnlock();
-			directory = directory->Parent();
+			directory = directory->GetParentUnchecked();
 				// the parent is still locked, so this is safe
 		} while (packageNode != NULL);
 	} while (packageNode != NULL);
@@ -1132,7 +1138,7 @@ Volume::_RemovePackageContentRootNode(Package* package,
 			// unlock all directories
 			while (directory != NULL) {
 				directory->WriteUnlock();
-				directory = directory->Parent();
+				directory = directory->GetParentUnchecked();
 			}
 			break;
 		}
@@ -1169,7 +1175,7 @@ Volume::_RemovePackageContentRootNode(Package* package,
 			// no more siblings -- go back up the tree
 			packageNode = packageDirectory;
 			directory->WriteUnlock();
-			directory = directory->Parent();
+			directory = directory->GetParentUnchecked();
 				// the parent is still locked, so this is safe
 		} while (packageNode != NULL/* && packageNode != rootPackageNode*/);
 	} while (packageNode != NULL/* && packageNode != rootPackageNode*/);
@@ -1205,10 +1211,12 @@ Volume::_AddPackageNode(Directory* directory, PackageNode* packageNode,
 	}
 
 	BReference<Node> nodeReference(node);
-	NodeWriteLocker nodeWriteLocker(node);
+	DirectoryWriteLocker directoryNodeWriteLocker;
+	if (Directory* directory = dynamic_cast<Directory*>(node))
+		directoryNodeWriteLocker.SetTo(directory, false, true);
 
 	BReference<Node> newNodeReference;
-	NodeWriteLocker newNodeWriteLocker;
+	DirectoryWriteLocker newDirectoryNodeWriteLocker;
 	Node* oldNode = NULL;
 
 	if (!newNode && !S_ISDIR(node->Mode()) && oldPackageNode != NULL
@@ -1234,7 +1242,8 @@ Volume::_AddPackageNode(Directory* directory, PackageNode* packageNode,
 		unpackingNode = newUnpackingNode;
 		node = unpackingNode->GetNode();
 		newNodeReference.SetTo(node);
-		newNodeWriteLocker.SetTo(node, false);
+		if (Directory* newDirectory = dynamic_cast<Directory*>(node))
+			newDirectoryNodeWriteLocker.SetTo(newDirectory, false, true);
 
 		directory->AddChild(node);
 		fNodes.Insert(node);
@@ -1303,14 +1312,16 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 		return;
 
 	BReference<Node> nodeReference(node);
-	NodeWriteLocker nodeWriteLocker(node);
+	DirectoryWriteLocker directoryNodeWriteLocker;
+	if (Directory* directory = dynamic_cast<Directory*>(node))
+		directoryNodeWriteLocker.SetTo(directory, false, true);
 
 	PackageNode* headPackageNode = unpackingNode->GetPackageNode();
 	bool nodeRemoved = false;
 	Node* newNode = NULL;
 
 	BReference<Node> newNodeReference;
-	NodeWriteLocker newNodeWriteLocker;
+	DirectoryWriteLocker newDirectoryNodeWriteLocker;
 
 	// If this is the last package node of the node, remove it completely.
 	if (unpackingNode->IsOnlyPackageNode(packageNode)) {
@@ -1347,7 +1358,8 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 				// add the new node
 				newNode = newUnpackingNode->GetNode();
 				newNodeReference.SetTo(newNode);
-				newNodeWriteLocker.SetTo(newNode, false);
+				if (Directory* newDirectory = dynamic_cast<Directory*>(newNode))
+					newDirectoryNodeWriteLocker.SetTo(newDirectory, false, true);
 
 				directory->AddChild(newNode);
 				fNodes.Insert(newNode);
@@ -1429,7 +1441,7 @@ void
 Volume::_RemoveNode(Node* node)
 {
 	// remove from parent
-	Directory* parent = node->Parent();
+	Directory* parent = node->GetParentUnchecked();
 	parent->RemoveChild(node);
 
 	// remove from node table
@@ -1441,22 +1453,35 @@ Volume::_RemoveNode(Node* node)
 void
 Volume::_RemoveNodeAndVNode(Node* node)
 {
+	Directory* parent = NULL;
+	DirectoryWriteLocker nodeWriteLocker;
+	if (Directory* directory = dynamic_cast<Directory*>(node))
+		nodeWriteLocker.SetTo(directory, false, true);
+	else
+		parent = node->GetParentUnchecked();
+
+	// Remove the node from its parent and the volume. This makes the node
+	// inaccessible via the get_vnode() and lookup() hooks, and (if this
+	// is not a directory) prevents any future accesses, since regular Nodes
+	// do not have a lock of their own.
+	_RemoveNode(node);
+
+	const bool isKnownToVFS = node->IsKnownToVFS();
+
+	if (parent != NULL) {
+		// This node is not a directory. In order to avoid deadlocks, unlock
+		// its parent while we invoke the VFS, so that any threads trying
+		// to acquire the directory's lock wake up and exit.
+		parent->WriteUnlock();
+	} else {
+		nodeWriteLocker.Unlock();
+	}
+
 	// If the node is known to the VFS, we get the vnode, remove it, and put it,
 	// so that the VFS will discard it as soon as possible (i.e. now, if no one
 	// else is using it).
-	NodeWriteLocker nodeWriteLocker(node);
-
-	// Remove the node from its parent and the volume. This makes the node
-	// inaccessible via the get_vnode() and lookup() hooks.
-	_RemoveNode(node);
-
-	bool getVNode = node->IsKnownToVFS();
-
-	nodeWriteLocker.Unlock();
-
-	// Get a vnode reference, if the node is already known to the VFS.
 	Node* dummyNode;
-	if (getVNode && GetVNode(node->ID(), dummyNode) == B_OK) {
+	if (isKnownToVFS && GetVNode(node->ID(), dummyNode) == B_OK) {
 		// TODO: There still is a race condition here which we can't avoid
 		// without more help from the VFS. Right after we drop the write
 		// lock a vnode for the node could be discarded by the VFS. At that
@@ -1474,6 +1499,9 @@ Volume::_RemoveNodeAndVNode(Node* node)
 		RemoveVNode(node->ID());
 		PutVNode(node->ID());
 	}
+
+	if (parent != NULL)
+		parent->WriteLock();
 }
 
 
@@ -1756,7 +1784,7 @@ Volume::_CreateShineThroughDirectories(const char* shineThroughSetting)
 	if (directories == NULL)
 		return B_OK;
 
-	NodeWriteLocker rootDirectoryWriteLocker(fRootDirectory);
+	DirectoryWriteLocker rootDirectoryWriteLocker(fRootDirectory);
 
 	// iterate through the directory list and create the directories
 	while (const char* directoryName = *(directories++)) {
@@ -1775,7 +1803,7 @@ Volume::_CreateShineThroughDirectories(const char* shineThroughSetting)
 status_t
 Volume::_PublishShineThroughDirectories()
 {
-	NodeWriteLocker rootDirectoryWriteLocker(fRootDirectory);
+	DirectoryWriteLocker rootDirectoryWriteLocker(fRootDirectory);
 
 	// Iterate through the root directory children and bind the shine-through
 	// directories to the respective mount point subdirectories.
@@ -1854,8 +1882,8 @@ Volume::_AddPackageLinksDirectory()
 	PackageLinksDirectory* packageLinksDirectory
 		= fPackageFSRoot->GetPackageLinksDirectory();
 
-	NodeWriteLocker rootDirectoryWriteLocker(fRootDirectory);
-	NodeWriteLocker packageLinksDirectoryWriteLocker(packageLinksDirectory);
+	DirectoryWriteLocker rootDirectoryWriteLocker(fRootDirectory);
+	DirectoryWriteLocker packageLinksDirectoryWriteLocker(packageLinksDirectory);
 
 	fRootDirectory->AddChild(packageLinksDirectory);
 
@@ -1874,10 +1902,10 @@ Volume::_RemovePackageLinksDirectory()
 		= fPackageFSRoot->GetPackageLinksDirectory();
 
 	VolumeWriteLocker volumeLocker(this);
-	NodeWriteLocker rootDirectoryWriteLocker(fRootDirectory);
-	NodeWriteLocker packageLinksDirectoryWriteLocker(packageLinksDirectory);
+	DirectoryWriteLocker rootDirectoryWriteLocker(fRootDirectory);
+	DirectoryWriteLocker packageLinksDirectoryWriteLocker(packageLinksDirectory);
 
-	if (packageLinksDirectory->Parent() == fRootDirectory) {
+	if (packageLinksDirectory->GetParentUnchecked() == fRootDirectory) {
 		packageLinksDirectory->SetListener(NULL);
 		fRootDirectory->RemoveChild(packageLinksDirectory);
 	}
@@ -1897,7 +1925,6 @@ Volume::_AddPackageLinksNode(Node* node)
 	if (Directory* directory = dynamic_cast<Directory*>(node)) {
 		for (Node* child = directory->FirstChild(); child != NULL;
 				child = directory->NextChild(child)) {
-			NodeWriteLocker childWriteLocker(child);
 			_AddPackageLinksNode(child);
 		}
 	}
@@ -1912,7 +1939,6 @@ Volume::_RemovePackageLinksNode(Node* node)
 	if (Directory* directory = dynamic_cast<Directory*>(node)) {
 		for (Node* child = directory->FirstChild(); child != NULL;
 				child = directory->NextChild(child)) {
-			NodeWriteLocker childWriteLocker(child);
 			_RemovePackageLinksNode(child);
 		}
 	}

src/add-ons/kernel/file_systems/packagefs/volume/Volume.h

@@ -47,6 +47,7 @@ public:
 	inline	void				ReadUnlock() const;
 	inline	bool				WriteLock();
 	inline	void				WriteUnlock();
+	inline	bool				IsWriteLocked() const;
 
 			fs_volume*			FSVolume() const	{ return fFSVolume; }
 			dev_t				ID() const			{ return fFSVolume->id; }
@@ -233,6 +234,13 @@ Volume::WriteUnlock()
 }
 
 
+bool
+Volume::IsWriteLocked() const
+{
+	return find_thread(NULL) == fLock.holder;
+}
+
+
 typedef AutoLocker<const Volume, AutoLockerReadLocking<const Volume> >
 	VolumeReadLocker;
 typedef AutoLocker<Volume, AutoLockerWriteLocking<Volume> > VolumeWriteLocker;