From 44074400991525c0acdc1c13970fedb0fcfe93fd Mon Sep 17 00:00:00 2001 From: Jim906 Date: Fri, 28 Feb 2025 09:31:02 -0500 Subject: [PATCH] nfs4: Remove stale nodes when their ino is reused * Add FileSystem::EnsureNoCollision to check for a stale node with the given inode number, and remove it if it exists. * Call EnsureNoCollision from Inode::CreateObject and Inode::CreateState to cover creation of directory files and regular files, respectively. * Stop using the entry cache. Instead, make use of the driver's DirectoryCache in the lookup hook. * Fixes #16924. When a shared file is deleted, and no clients have the file open, the server considers the file ID number of the deleted file to be eligible for reassignment to new files. If the deletion was performed by the server or by some other client, the Haiku client does't know the file was deleted, and still may hold a vnode for it. This interferes with the creation of a new file by the client, when the file ID / ino of the new file is the same as that of the stale node. The reason for disabling the entry cache is to give the driver a chance to check the server for changes in name-to-ino mapping when the VFS function lookup_dir_entry is called. At least in its current form, the driver isn't able to deal with outdated entry cache information. For example, if the Haiku client has added temp.txt to its entry cache, and then temp.txt is deleted by the server, running 'nano temp.txt' on the client will attempt to open the (non-existent) file instead of creating it. Change-Id: Ic3ec21d18c8616fcfeb9234aafb0e647746d23cd Reviewed-on: https://review.haiku-os.org/c/haiku/+/9068 Reviewed-by: waddlesplash --- .../file_systems/nfs4/DirectoryCache.cpp | 12 ----- .../kernel/file_systems/nfs4/FileSystem.cpp | 54 +++++++++++++++++-- .../kernel/file_systems/nfs4/FileSystem.h | 8 ++- .../kernel/file_systems/nfs4/Inode.cpp | 37 ++++++++++++- src/add-ons/kernel/file_systems/nfs4/Inode.h | 19 +++++++ .../kernel/file_systems/nfs4/InodeRegular.cpp | 2 + .../file_systems/nfs4/kernel_interface.cpp | 15 ++++-- 7 files changed, 125 insertions(+), 22 deletions(-) diff --git a/src/add-ons/kernel/file_systems/nfs4/DirectoryCache.cpp b/src/add-ons/kernel/file_systems/nfs4/DirectoryCache.cpp index af328f1db2..fab2d20d0d 100644 --- a/src/add-ons/kernel/file_systems/nfs4/DirectoryCache.cpp +++ b/src/add-ons/kernel/file_systems/nfs4/DirectoryCache.cpp @@ -110,8 +110,6 @@ DirectoryCache::Trash() { while (!fNameCache.IsEmpty()) { NameCacheEntry* current = fNameCache.RemoveHead(); - entry_cache_remove(fInode->GetFileSystem()->DevId(), fInode->ID(), - current->fName); delete current; } @@ -149,11 +147,6 @@ DirectoryCache::AddEntry(const char* name, ino_t node, bool created) fDirectoryCache->fEntries.Add(entry); } - if (!fAttrDir) { - return entry_cache_add(fInode->GetFileSystem()->DevId(), fInode->ID(), - name, node); - } - return B_OK; } @@ -194,11 +187,6 @@ DirectoryCache::RemoveEntry(const char* name) current = iterator.Next(); } } - - if (!fAttrDir) { - entry_cache_remove(fInode->GetFileSystem()->DevId(), fInode->ID(), - name); - } } diff --git a/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp b/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp index 4a48019138..8f5ed2e375 100644 --- a/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp +++ b/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp @@ -17,6 +17,7 @@ #include "Request.h" #include "RootInode.h" +#include "VnodeToInode.h" #define ERROR(x...) dprintf("nfs4: " x) @@ -142,8 +143,8 @@ GetInodeNames(const char** root, const char* _path) status_t -FileSystem::Mount(FileSystem** _fs, RPC::Server* serv, const char* serverName, - const char* fsPath, dev_t id, const MountConfiguration& configuration) +FileSystem::Mount(FileSystem** _fs, RPC::Server* serv, const char* serverName, const char* fsPath, + fs_volume* volume, const MountConfiguration& configuration) { CALLED(); @@ -223,8 +224,9 @@ FileSystem::Mount(FileSystem** _fs, RPC::Server* serv, const char* serverName, FileInfo fi; fs->fServer = serv; - fs->fDevId = id; + fs->fDevId = volume->id; fs->fFsId = *fsid; + fs->fFsVolume = volume; fi.fHandle = fh; @@ -474,6 +476,52 @@ FileSystem::GetDelegation(const FileHandle& handle) } +/*! Used when creating a file to check for a stale node with the same ino. If it exists, + the stale node is deleted. + @param newID The file ID assigned by the server to a file now being created. + @param handle The handle assigned by the server to the same file. + @pre The caller has not yet updated fInoIdMap with the FileInfo of the file that we are + creating. The VFS list of vnodes has also not been updated yet. + @post Any stale node object with this ID is gone. Any stale entry in fInoIdMap is still + present, and will be replaced when the caller calls fInoIdMap->AddName. +*/ +void +FileSystem::EnsureNoCollision(ino_t newID, const FileHandle& handle) +{ + FileInfo existingInfo; + status_t result = fInoIdMap.GetFileInfo(&existingInfo, newID); + if (result == B_OK && existingInfo.fHandle != handle) { + // We are already using this file ID for a previously existing file. If the server has + // assigned that ID to the file that we are now creating, it means someone else must have + // deleted the other file from the server, and the server is recycling the file ID. + result = acquire_vnode(fFsVolume, newID); + if (result == B_OK) { + // we still have a vnode for the stale file present in memory + + // mark it as stale + VnodeToInode* vti; + result = get_vnode(fFsVolume, newID, reinterpret_cast(&vti)); + ASSERT(result == B_OK); + Inode* inode = vti->GetPointer(); + if (inode != NULL) + inode->SetStale(); + put_vnode(fFsVolume, newID); + + // delete it + result = remove_vnode(fFsVolume, newID); + ASSERT(result == B_OK); + put_vnode(fFsVolume, newID); + + // verify it is gone + result = acquire_vnode(fFsVolume, newID); + ASSERT(result != B_OK); + } + } + + return; +} + + status_t FileSystem::_ParsePath(RequestBuilder& req, uint32& count, const char* _path) { diff --git a/src/add-ons/kernel/file_systems/nfs4/FileSystem.h b/src/add-ons/kernel/file_systems/nfs4/FileSystem.h index c8e77d4ae9..bcf13e7a3e 100644 --- a/src/add-ons/kernel/file_systems/nfs4/FileSystem.h +++ b/src/add-ons/kernel/file_systems/nfs4/FileSystem.h @@ -9,6 +9,8 @@ #define FILESYSTEM_H +#include + #include "Delegation.h" #include "InodeIdMap.h" #include "NFS4Defs.h" @@ -33,7 +35,7 @@ class FileSystem : public DoublyLinkedListLinkImpl { public: static status_t Mount(FileSystem** pfs, RPC::Server* serv, const char* serverName, const char* path, - dev_t id, + fs_volume* volume, const MountConfiguration& configuration); ~FileSystem(); @@ -78,6 +80,8 @@ public: inline const MountConfiguration& GetConfiguration(); inline mutex& CreateFileLock(); + + void EnsureNoCollision(ino_t newID, const FileHandle& handle); private: FileSystem(const MountConfiguration& config); @@ -115,6 +119,8 @@ private: InodeIdMap fInoIdMap; MountConfiguration fConfiguration; + + fs_volume* fFsVolume; }; diff --git a/src/add-ons/kernel/file_systems/nfs4/Inode.cpp b/src/add-ons/kernel/file_systems/nfs4/Inode.cpp index 9e186fc02b..f2ca133ab6 100644 --- a/src/add-ons/kernel/file_systems/nfs4/Inode.cpp +++ b/src/add-ons/kernel/file_systems/nfs4/Inode.cpp @@ -32,7 +32,8 @@ Inode::Inode() fOpenState(NULL), fWriteDirty(false), fAIOWait(create_sem(1, NULL)), - fAIOCount(0) + fAIOCount(0), + fStale(false) { rw_lock_init(&fDelegationLock, NULL); mutex_init(&fStateLock, NULL); @@ -195,10 +196,39 @@ Inode::LookUp(const char* name, ino_t* id) if (fType != NF4DIR) return B_NOT_A_DIRECTORY; + // attempt to get the id from the DirectoryCache + fCache->Lock(); + status_t result = fCache->Revalidate(); + // At times, this will do a full readdir, in order to sync the DirectoryCache + // with server changes. However, if the directory contents have not changed since + // the last readdir, it will either perform no RPC, or an RPC that just checks + // FATTR4_CHANGE. + if (result == B_OK) { + SinglyLinkedList& entriesList = fCache->EntriesList(); + NameCacheEntry* entry = entriesList.Head(); + while (entry != NULL) { + if (strcmp(name, entry->fName) == 0) + break; + entry = entriesList.GetNext(entry); + } + if (entry != NULL) { + // we are skipping ChildAdded(); verify that it is not needed because the InoIdMap + // already has this entry + FileInfo info; + result = fFileSystem->InoIdMap()->GetFileInfo(&info, entry->fNode); + ASSERT(result == B_OK); + + *id = entry->fNode; + fCache->Unlock(); + return B_OK; + } + } + fCache->Unlock(); + uint64 change; uint64 fileID; FileHandle handle; - status_t result = NFS4Inode::LookUp(name, &change, &fileID, &handle); + result = NFS4Inode::LookUp(name, &change, &fileID, &handle); if (result != B_OK) return result; @@ -272,6 +302,7 @@ Inode::Remove(const char* name, FileType type, ino_t* id) ChangeInfo changeInfo; uint64 fileID; + status_t result = NFS4Inode::RemoveObject(name, type, &changeInfo, &fileID); if (result != B_OK) return result; @@ -418,6 +449,8 @@ Inode::CreateObject(const char* name, const char* path, int mode, FileType type, if (result != B_OK) return result; + fFileSystem->EnsureNoCollision(FileIdToInoT(fileID), handle); + fFileSystem->Root()->MakeInfoInvalid(); result = ChildAdded(name, fileID, handle); diff --git a/src/add-ons/kernel/file_systems/nfs4/Inode.h b/src/add-ons/kernel/file_systems/nfs4/Inode.h index 1e6d47bd31..98155231b4 100644 --- a/src/add-ons/kernel/file_systems/nfs4/Inode.h +++ b/src/add-ons/kernel/file_systems/nfs4/Inode.h @@ -118,6 +118,9 @@ public: void BeginAIOOp(); void EndAIOOp(); inline void WaitAIOComplete(); + + inline void SetStale(bool stale = true); + inline bool IsStale() const; protected: Inode(); @@ -168,6 +171,8 @@ private: sem_id fAIOWait; uint32 fAIOCount; mutex fAIOLock; + + bool fStale; }; @@ -255,5 +260,19 @@ Inode::GetOpenState() } +inline void +Inode::SetStale(bool stale) +{ + fStale = stale; +} + + +inline bool +Inode::IsStale() const +{ + return fStale; +} + + #endif // INODE_H diff --git a/src/add-ons/kernel/file_systems/nfs4/InodeRegular.cpp b/src/add-ons/kernel/file_systems/nfs4/InodeRegular.cpp index 30c5a55c0f..c97ae59613 100644 --- a/src/add-ons/kernel/file_systems/nfs4/InodeRegular.cpp +++ b/src/add-ons/kernel/file_systems/nfs4/InodeRegular.cpp @@ -40,6 +40,8 @@ Inode::CreateState(const char* name, int mode, int perms, OpenState* state, fileInfo.fFileId = fileID; fileInfo.fHandle = handle; + fFileSystem->EnsureNoCollision(FileIdToInoT(fileID), handle); + fFileSystem->InoIdMap()->AddName(fileInfo, fInfo.fNames, name, FileIdToInoT(fileID)); diff --git a/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp b/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp index a74d9faf33..0668c0920c 100644 --- a/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp +++ b/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp @@ -191,10 +191,9 @@ nfs4_mount(fs_volume* volume, const char* device, uint32 flags, ERROR("Unable to Acquire RPCServerManager!\n"); return result; } - + FileSystem* fs; - result = FileSystem::Mount(&fs, server, serverName, path, volume->id, - config); + result = FileSystem::Mount(&fs, server, serverName, path, volume, config); if (result != B_OK) { ERROR("Error mounting filesystem: %s\n", strerror(result)); gRPCServerManager->Release(server); @@ -347,9 +346,17 @@ nfs4_remove_vnode(fs_volume* volume, fs_vnode* vnode, bool reenter) VnodeToInode* vti = reinterpret_cast(vnode->private_node); TRACE("volume = %p, vnode = %" B_PRIi64 "\n", volume, vti->ID()); - if (fs->Root() == vti->GetPointer()) + Inode* node = vti->GetPointer(); + + if (node == fs->Root()) return B_OK; + if (node != NULL && node->IsStale()) { + // in the case of a stale node, VnodeToInode::Unlink was never called by the client, + // so the Inode hasn't been deleted yet + vti->Clear(); + } + ASSERT(vti->GetPointer() == NULL); delete vti;