nfs4: Improve compliance with VFS policy

* Eliminate the use of acquire_vnode as a probe to see whether a node
  has been constructed on the client side.
* Put the root node in unmount.
* In DirectoryCache::_LoadSnapshot(), don't mark a node removed unless
  certain that no other hard links exist.  The old logic might cause
  problems if, after marking a node removed, the client found another
  link to the file in another server directory.

The reason for removing the asserts from FileSystem::GetInode() is
that, with get_vnode calls replacing acquire_vnode calls, this may be
called earlier in the process of file creation, before
FileSystem::fInoIdMap contains an entry for the new file.

The changed logic in _LoadSnapshot isn't directly related to the new
VFS asserts.  It is meant to correct a problem that I noticed
incidentally when making the other changes.

Change-Id: I075485bc68ea9443be899add948116f0a0bca047
Reviewed-on: https://review.haiku-os.org/c/haiku/+/9624
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: waddlesplash <[email protected]>

src/add-ons/kernel/file_systems/nfs4/DirectoryCache.cpp

@@ -237,10 +237,8 @@ DirectoryCache::_LoadSnapshot(bool trash)
 				}
 				if (!nodeFound) {
 					// The inode-name association that was cached in 'current' is no longer valid.
-					result = fInode->GetFileSystem()->TrashStaleNode(current->fNode);
-					if (result != B_OK)
-						INFORM("_LoadSnapshot: Couldn't free stale node %" B_PRIdINO "\n",
-							current->fNode);
+					fInode->GetFileSystem()->ServerUnlinkCleanup(current->fNode, fInode,
+						current->fName);
 				}
 			}
 			delete current;

src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp

@@ -63,8 +63,6 @@ FileSystem::~FileSystem()
 			free(const_cast<char*>(fPath[i]));
 	}
 	delete[] fPath;
-
-	delete fRoot;
 }
 
 
@@ -267,13 +265,6 @@ FileSystem::GetInode(ino_t id, Inode** _inode)
 
 	FileInfo fi;
 	status_t result = fInoIdMap.GetFileInfo(&fi, id);
-	if (result == B_ENTRY_NOT_FOUND) {
-		bool removed = false;
-		status_t getRemovedResult = get_vnode_removed(fFsVolume, id, &removed);
-		ASSERT(getRemovedResult == B_OK);
-		ASSERT(removed == true);
-	}
-
 	if (result != B_OK)
 		return result;
 
@@ -469,37 +460,36 @@ FileSystem::GetDelegation(const FileHandle& handle)
 	return it.Current();
 }
 
-/*! If a node object with this inode number exists, free it at the FS and VFS levels to
-	ensure that no operation will try to use it.
 
+/*! Mark this node removed and free it to ensure that no operation will try to use it.
+	@pre We hold a VFS ref to this node.
+	@post The ref needs to be released by the caller.
 */
 status_t
-FileSystem::TrashStaleNode(ino_t ino)
+FileSystem::TrashStaleNode(Inode* inode)
 {
-	status_t result = B_OK;
-	if (acquire_vnode(fFsVolume, ino) == B_OK) {
-		// we still have a vnode for the stale file present in memory
+	ino_t ino = inode->ID();
+	INFORM("TrashStaleNode %p %" B_PRIdINO "\n", inode, ino);
 
-		// mark it as stale
-		VnodeToInode* vti;
-		result = get_vnode(fFsVolume, ino, reinterpret_cast<void**>(&vti));
-		ASSERT(result == B_OK);
-		Inode* inode = vti->GetPointer();
-		if (inode != NULL)
-			inode->SetStale();
-		put_vnode(fFsVolume, ino);
+	inode->SetStale();
 
-		// delete it
-		result = remove_vnode(fFsVolume, ino);
-		ASSERT(result == B_OK);
-		put_vnode(fFsVolume, ino);
+	status_t result = remove_vnode(fFsVolume, ino);
+	ASSERT(result == B_OK);
 
-		// verify it is gone
-		if (acquire_vnode(fFsVolume, ino) == B_OK)
-			result = B_ERROR;
-		else
-			result = B_OK;
-	}
+	return result;
+}
+
+
+/*! Check whether the server node still has any hard links (including links that the client may be
+	unaware of).  If none, mark the client node removed.
+*/
+status_t
+FileSystem::TrashIfStale(Inode* inode)
+{
+	struct stat stat;
+	status_t result = inode->Stat(&stat, NULL, true);
+	if (result != B_OK)
+		result = TrashStaleNode(inode);
 
 	return result;
 }
@@ -515,16 +505,46 @@ FileSystem::TrashStaleNode(ino_t ino)
 	present, and will be replaced when the caller calls fInoIdMap->AddName.
 */
 void
-FileSystem::EnsureNoCollision(ino_t newID, const FileHandle& handle)
+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 = TrashStaleNode(newID);
-		ASSERT(result == B_OK);
+	VnodeToInode* existingVti = NULL;
+	status_t result = get_vnode(fFsVolume, newId, reinterpret_cast<void**>(&existingVti));
+	if (result == B_OK) {
+		// We haven't finished creating the new node yet, so whatever get_vnode returned must be
+		// stale since the server just re-issued its inode number.
+		ASSERT(handle != existingVti->GetPointer()->fInfo.fHandle);
+		result = TrashStaleNode(existingVti->GetPointer());
+		if (result != B_OK)
+			INFORM("EnsureNoCollision: Couldn't trash stale node %" B_PRIdINO "\n", newId);
+		put_vnode(fFsVolume, newId);
+	}
+
+	return;
+}
+
+
+/*!	Delete a name from a client-side node after someone else has unlinked that name on the server
+	side.  If that was the last name known to the client, call TrashIfStale.
+	@param missingName A previously cached file name that is no longer valid.
+*/
+void
+FileSystem::ServerUnlinkCleanup(ino_t id, Inode* parent, const char* missingName)
+{
+	FileInfo fileInfo;
+	status_t result = fInoIdMap.GetFileInfo(&fileInfo, id);
+
+	VnodeToInode* vti = NULL;
+	result = get_vnode(fFsVolume, id, reinterpret_cast<void**>(&vti));
+	if (result == B_OK) {
+		bool noRemainingNames = false;
+		noRemainingNames = fileInfo.fNames->RemoveName(parent->fInfo.fNames, missingName);
+		if (noRemainingNames) {
+			// This client knows of no hard links to this node.
+			result = TrashIfStale(vti->GetPointer());
+			if (result != B_OK)
+				INFORM("ServerUnlinkCleanup: Couldn't trash stale node %" B_PRIdINO "\n", id);
+		}
+		put_vnode(fFsVolume, id);
 	}
 
 	return;

src/add-ons/kernel/file_systems/nfs4/FileSystem.h

@@ -84,8 +84,11 @@ public:
 
 	inline	mutex&				CreateFileLock();
 
-			status_t			TrashStaleNode(ino_t ino);
-			void				EnsureNoCollision(ino_t newID, const FileHandle& handle);
+			status_t			TrashStaleNode(Inode* inode);
+			status_t			TrashIfStale(Inode* inode);
+			void				EnsureNoCollision(ino_t newId, const FileHandle& handle);
+			void				ServerUnlinkCleanup(ino_t id, Inode* parent,
+									const char* missingName);
 
 			void				Dump(void (*xprintf)(const char*, ...) = dprintf);
 

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

@@ -533,7 +533,7 @@ Inode::Access(int mode)
 
 
 status_t
-Inode::Stat(struct stat* st, OpenAttrCookie* attr)
+Inode::Stat(struct stat* st, OpenAttrCookie* attr, bool revalidate)
 {
 	ASSERT(st != NULL);
 
@@ -541,7 +541,7 @@ Inode::Stat(struct stat* st, OpenAttrCookie* attr)
 		return GetStat(st, attr);
 
 	bool cache = fFileSystem->GetConfiguration().fCacheMetadata;
-	if (!cache)
+	if (!cache || revalidate)
 		return GetStat(st, NULL);
 
 	status_t result = fMetaCache.GetStat(st);

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

@@ -66,7 +66,7 @@ public:
 									ino_t* oldID = NULL);
 
 					status_t	Stat(struct stat* st,
-									OpenAttrCookie* attr = NULL);
+									OpenAttrCookie* attr = NULL, bool revalidate = false);
 					status_t	WriteStat(const struct stat* st, uint32 mask,
 									OpenAttrCookie* attr = NULL);
 

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

@@ -259,9 +259,12 @@ static status_t
 nfs4_unmount(fs_volume* volume)
 {
 	TRACE("volume = %p\n", volume);
+
 	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
 	RPC::Server* server = fs->Server();
 
+	put_vnode(volume, fs->Root()->ID());
+
 	delete fs;
 	gRPCServerManager->Release(server);
 
@@ -599,24 +602,24 @@ nfs4_unlink(fs_volume* volume, fs_vnode* dir, const char* name)
 		return B_ENTRY_NOT_FOUND;
 
 	ino_t id;
-	status_t result = inode->Remove(name, NF4REG, &id);
-	if (result != B_OK)
-		return result;
-	locker.Unlock();
-
-	result = acquire_vnode(volume, id);
+	inode->LookUp(name, &id);
+	VnodeToInode* childVti = NULL;
+	status_t result = get_vnode(volume, id, reinterpret_cast<void**>(&childVti));
 	if (result == B_OK) {
-		result = get_vnode(volume, id, reinterpret_cast<void**>(&vti));
-		ASSERT(result == B_OK);
-		
+		ino_t removedId;
+		status_t result = inode->Remove(name, NF4REG, &removedId);
+		if (result != B_OK)
+			return result;
+		ASSERT(removedId == id);
+		locker.Unlock();
+
 		if (vti->Unlink(inode->fInfo.fNames, name))
 			remove_vnode(volume, id);
 
 		put_vnode(volume, id);
-		put_vnode(volume, id);
 	}
 
-	return B_OK;
+	return result;
 }
 
 
@@ -656,14 +659,10 @@ nfs4_rename(fs_volume* volume, fs_vnode* fromDir, const char* fromName,
 
 	if (oldID != 0) {
 		// we have overriden an inode
-		result = acquire_vnode(volume, oldID);
+		result = get_vnode(volume, oldID, reinterpret_cast<void**>(&vti));
 		if (result == B_OK) {
-			result = get_vnode(volume, oldID, reinterpret_cast<void**>(&vti));
-			ASSERT(result == B_OK);
 			if (vti->Unlink(toInode->fInfo.fNames, toName))
 				remove_vnode(volume, oldID);
-
-			put_vnode(volume, oldID);
 			put_vnode(volume, oldID);
 		}
 	}
@@ -741,39 +740,6 @@ nfs4_write_stat(fs_volume* volume, fs_vnode* vnode, const struct stat* stat,
 }
 
 
-static status_t
-get_new_vnode(fs_volume* volume, ino_t id, VnodeToInode** _vti)
-{
-	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
-	Inode* inode;
-	VnodeToInode* vti;
-
-	status_t result = acquire_vnode(volume, id);
-	if (result == B_OK) {
-		ASSERT(get_vnode(volume, id, reinterpret_cast<void**>(_vti)) == B_OK);
-		unremove_vnode(volume, id);
-
-		// Release after acquire
-		put_vnode(volume, id);
-
-		vti = *_vti;
-
-		if (vti->Get() == NULL) {
-			result = fs->GetInode(id, &inode);
-			if (result != B_OK) {
-				put_vnode(volume, id);
-				return result;
-			}
-
-			vti->Replace(inode);
-		}
-		return B_OK;
-	}
-
-	return get_vnode(volume, id, reinterpret_cast<void**>(_vti));
-}
-
-
 static status_t
 nfs4_create(fs_volume* volume, fs_vnode* dir, const char* name, int openMode,
 	int perms, void** _cookie, ino_t* _newVnodeID)
@@ -807,7 +773,7 @@ nfs4_create(fs_volume* volume, fs_vnode* dir, const char* name, int openMode,
 		return result;
 	}
 
-	result = get_new_vnode(volume, *_newVnodeID, &vti);
+	result = get_vnode(volume, *_newVnodeID, reinterpret_cast<void**>(&vti));
 	if (result != B_OK) {
 		delete cookie;
 		return result;
@@ -1031,15 +997,10 @@ nfs4_remove_dir(fs_volume* volume, fs_vnode* parent, const char* name)
 	if (result != B_OK)
 		return result;
 
-	result = acquire_vnode(volume, id);
+	result = get_vnode(volume, id, reinterpret_cast<void**>(&vti));
 	if (result == B_OK) {
-		result = get_vnode(volume, id, reinterpret_cast<void**>(&vti));
-		ASSERT(result == B_OK);
-
 		if (vti->Unlink(inode->fInfo.fNames, name))
 			remove_vnode(volume, id);
-
-		put_vnode(volume, id);
 		put_vnode(volume, id);
 	}