From 0059775c1d6454bba41a9f0defebc4b6151838fa Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Wed, 11 Sep 2024 16:59:39 -0400 Subject: [PATCH] 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 Reviewed-by: waddlesplash --- .../file_systems/packagefs/indices/Query.cpp | 2 +- .../packagefs/kernel_interface.cpp | 114 +++++++++++++----- .../packagefs/nodes/Directory.cpp | 8 +- .../file_systems/packagefs/nodes/Directory.h | 43 +++++++ .../file_systems/packagefs/nodes/Node.cpp | 34 ++++-- .../file_systems/packagefs/nodes/Node.h | 42 +------ .../package_links/PackageLinkDirectory.cpp | 26 ++-- .../package_links/PackageLinksDirectory.cpp | 14 +-- .../file_systems/packagefs/volume/Volume.cpp | 96 +++++++++------ .../file_systems/packagefs/volume/Volume.h | 8 ++ 10 files changed, 251 insertions(+), 136 deletions(-) diff --git a/src/add-ons/kernel/file_systems/packagefs/indices/Query.cpp b/src/add-ons/kernel/file_systems/packagefs/indices/Query.cpp index 7f7c71e04d..fea391125d 100644 --- a/src/add-ons/kernel/file_systems/packagefs/indices/Query.cpp +++ b/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) diff --git a/src/add-ons/kernel/file_systems/packagefs/kernel_interface.cpp b/src/add-ons/kernel/file_systems/packagefs/kernel_interface.cpp index 7c147933d9..b0d02cbffd 100644 --- a/src/add-ons/kernel/file_systems/packagefs/kernel_interface.cpp +++ b/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(node)) { + locker.SetTo(directory, false, true); + return locker.IsLocked(); + } else { + BReference 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(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(dir)->FindChild(StringKey(entryName)); - if (node == NULL) + Node* child = dir->FindChild(StringKey(entryName)); + if (child == NULL) return B_ENTRY_NOT_FOUND; - BReference nodeReference(node); + BReference 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 nodeReference(node); + + DirectoryWriteLocker dirLocker; + if (Directory* directory = dynamic_cast(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(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(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(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) diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.cpp b/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.cpp index a809561193..85f4ef22ba 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.cpp +++ b/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); diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.h b/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.h index d269bd4620..00141d85df 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/Directory.h @@ -8,6 +8,9 @@ #include "Node.h" +#include +#include + struct DirectoryIterator : DoublyLinkedListLinkImpl { 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 > DirectoryReadLocker; +typedef AutoLocker > DirectoryWriteLocker; + + #endif // DIRECTORY_H diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.cpp b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.cpp index f0e853dde3..e31bcdf4ac 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.cpp @@ -9,10 +9,17 @@ #include #include +#include +#include + #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 +Node::GetParent() const +{ + ReadLocker parentChangeLocker(sParentChangeLock); + if (fParent == NULL) + return NULL; + return BReference(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) { diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h index 04aa9a7c32..bbaa58febe 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h @@ -8,9 +8,8 @@ #include -#include +#include -#include #include #include @@ -43,13 +42,10 @@ public: void ReleaseReference(); int32 CountReferences(); - inline bool ReadLock(); - inline void ReadUnlock(); - inline bool WriteLock(); - inline void WriteUnlock(); + BReference 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 NodeList; typedef BOpenHashTable NodeNameHashTable; typedef BOpenHashTable NodeIDHashTable; -typedef AutoLocker > NodeReadLocker; -typedef AutoLocker > NodeWriteLocker; - #endif // NODE_H diff --git a/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinkDirectory.cpp b/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinkDirectory.cpp index 8861c9e747..fcf24391c6 100644 --- a/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinkDirectory.cpp +++ b/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(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; } diff --git a/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinksDirectory.cpp b/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinksDirectory.cpp index f614d1a3c3..cbc0ff67f8 100644 --- a/src/add-ons/kernel/file_systems/packagefs/package_links/PackageLinksDirectory.cpp +++ b/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) diff --git a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp b/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp index db5e417f29..188b0d3a31 100644 --- a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp +++ b/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 nodeReference(node); - NodeWriteLocker nodeWriteLocker(node); + DirectoryWriteLocker directoryNodeWriteLocker; + if (Directory* directory = dynamic_cast(node)) + directoryNodeWriteLocker.SetTo(directory, false, true); BReference 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(node)) + newDirectoryNodeWriteLocker.SetTo(newDirectory, false, true); directory->AddChild(node); fNodes.Insert(node); @@ -1303,14 +1312,16 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode, return; BReference nodeReference(node); - NodeWriteLocker nodeWriteLocker(node); + DirectoryWriteLocker directoryNodeWriteLocker; + if (Directory* directory = dynamic_cast(node)) + directoryNodeWriteLocker.SetTo(directory, false, true); PackageNode* headPackageNode = unpackingNode->GetPackageNode(); bool nodeRemoved = false; Node* newNode = NULL; BReference 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(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(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(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(node)) { for (Node* child = directory->FirstChild(); child != NULL; child = directory->NextChild(child)) { - NodeWriteLocker childWriteLocker(child); _RemovePackageLinksNode(child); } } diff --git a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.h b/src/add-ons/kernel/file_systems/packagefs/volume/Volume.h index 869b76f619..6fe6f103ef 100644 --- a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.h +++ b/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 > VolumeReadLocker; typedef AutoLocker > VolumeWriteLocker;