server/vfs: use Id_space to manage open nodes

Replacing the node lookup table with an Id_space removes the
limit on open handles per session and allows mutal associativity
between File_system handles and local VFS handles.

Fix #2221

repos/os/include/file_system_session/file_system_session.h

@@ -79,12 +79,12 @@ namespace File_system {
 
 struct File_system::Node_handle
 {
-	int value;
+	unsigned long value;
 
-	Node_handle() : value(-1) { }
+	Node_handle() : value(~0UL) { }
 	Node_handle(int v) : value(v) { }
 
-	bool valid() const { return value != -1; }
+	bool valid() const { return value != ~0UL; }
 
 	bool operator == (Node_handle const &other) const { return other.value == value; }
 	bool operator != (Node_handle const &other) const { return other.value != value; }
@@ -95,21 +95,21 @@ struct File_system::Node_handle
 struct File_system::File_handle : Node_handle
 {
 	File_handle() { }
-	File_handle(int v) : Node_handle(v) { }
+	File_handle(unsigned long v) : Node_handle(v) { }
 };
 
 
 struct File_system::Dir_handle : Node_handle
 {
 	Dir_handle() { }
-	Dir_handle(int v) : Node_handle(v) { }
+	Dir_handle(unsigned long v) : Node_handle(v) { }
 };
 
 
 struct File_system::Symlink_handle : Node_handle
 {
 	Symlink_handle() { }
-	Symlink_handle(int v) : Node_handle(v) { }
+	Symlink_handle(unsigned long v) : Node_handle(v) { }
 };
 
 

repos/os/src/server/vfs/main.cc

@@ -19,7 +19,6 @@
 #include <vfs/dir_file_system.h>
 #include <os/session_policy.h>
 #include <vfs/file_system_factory.h>
-#include <os/config.h>
 #include <base/sleep.h>
 #include <base/component.h>
 
@@ -52,27 +51,23 @@ class Vfs_server::Session_component :
 {
 	private:
 
-		/* maximum number of open nodes per session */
+		Node_space _node_space;
-		enum { MAX_NODE_HANDLES = 128U };
-
-		Node *_nodes[MAX_NODE_HANDLES];
-
-		/**
-		 * Each open node handle can act as a listener to be informed about
-		 * node changes.
-		 */
-		Listener _listeners[MAX_NODE_HANDLES];
 
 		Genode::String<160>     _label;
 
-		Genode::Ram_connection  _ram = { _label.string() };
+		Genode::Ram_connection  _ram;
-		Genode::Heap            _alloc =
+		Genode::Heap            _alloc;
-			{ &_ram, Genode::env()->rm_session() };
+
+		Genode::Signal_handler<Session_component> _process_packet_handler;
 
-		Genode::Signal_handler<Session_component>
-		                        _process_packet_dispatcher;
 		Vfs::Dir_file_system   &_vfs;
-		Directory               _root;
+
+		/*
+		 * The root node needs be allocated with the session struct
+		 * but removeable from the id space at session destruction.
+		 */
+		Genode::Constructible<Directory> _root;
+
 		bool                    _writable;
 
 
@@ -80,50 +75,37 @@ class Vfs_server::Session_component :
 		 ** Handle to node mapping **
 		 ****************************/
 
-		bool _in_range(int handle) const {
-			return ((handle >= 0) && (handle < MAX_NODE_HANDLES));
-		}
-
-		int _next_slot()
-		{
-			for (int i = 1; i < MAX_NODE_HANDLES; ++i)
-				if (_nodes[i] == nullptr)
-					return i;
-
-			throw Out_of_metadata();
-		}
-
 		/**
-		 * Lookup node using its handle as key
+		 * Apply functor to node
-		 */
-		Node *_lookup_node(Node_handle handle) {
-			return _in_range(handle.value) ? _nodes[handle.value] : 0; }
-
-		/**
-		 * Lookup typed node using its handle as key
 		 *
 		 * \throw Invalid_handle
 		 */
-		template <typename HANDLE_TYPE>
+		template <typename FUNC>
-		typename Node_type<HANDLE_TYPE>::Type &_lookup(HANDLE_TYPE handle)
+		void _apply(Node_handle handle, FUNC const &fn)
 		{
-			if (!_in_range(handle.value))
+			Node_space::Id id { handle.value };
-				throw Invalid_handle();
 
-			typedef typename Node_type<HANDLE_TYPE>::Type Node;
+			try { _node_space.apply<Node>(id, fn); }
-			Node *node = dynamic_cast<Node *>(_nodes[handle.value]);
+			catch (Node_space::Unknown_id) { throw Invalid_handle(); }
-			if (!node)
-				throw Invalid_handle();
-
-			return *node;
 		}
 
-		bool _refer_to_same_node(Node_handle h1, Node_handle h2) const
+		/**
+		 * Apply functor to typed node
+		 *
+		 * \throw Invalid_handle
+		 */
+		template <typename HANDLE_TYPE, typename FUNC>
+		void _apply(HANDLE_TYPE handle, FUNC const &fn)
 		{
-			if (!(_in_range(h1.value) && _in_range(h2.value)))
+			Node_space::Id id { handle.value };
-				throw Invalid_handle();
 
-			return _nodes[h1.value] == _nodes[h2.value];
+			try { _node_space.apply<Node>(id, [&] (Node &node) {
+				typedef typename Node_type<HANDLE_TYPE>::Type Typed_node;
+				Typed_node *n = dynamic_cast<Typed_node *>(&node);
+				if (!n)
+					throw Invalid_handle();
+				fn(*n);
+			}); } catch (Node_space::Unknown_id) { throw Invalid_handle(); }
 		}
 
 
@@ -152,24 +134,20 @@ class Vfs_server::Session_component :
 
 			switch (packet.operation()) {
 
-			case Packet_descriptor::READ: {
+			case Packet_descriptor::READ: try {
-				Node *node = _lookup_node(packet.handle());
+				_apply(packet.handle(), [&] (Node &node) {
-				if (!(node && (node->mode&READ_ONLY)))
+					if (node.mode&READ_ONLY)
-					return;
+						res_length = node.read(_vfs, (char *)content, length, seek);
-
+				}); } catch (...) { }
-				res_length = node->read(_vfs, (char *)content, length, seek);
 				break;
-			}
 
-			case Packet_descriptor::WRITE: {
+			case Packet_descriptor::WRITE: try {
-				Node *node = _lookup_node(packet.handle());
+				_apply(packet.handle(), [&] (Node &node) {
-				if (!(node && (node->mode&WRITE_ONLY)))
+					if (node.mode&WRITE_ONLY)
-					return;
+						res_length = node.write(_vfs, (char const *)content, length, seek);
-
+				}); } catch (...) { }
-				res_length = node->write(_vfs, (char const *)content, length, seek);
 				break;
 			}
-			}
 
 			packet.length(res_length);
 			packet.succeeded(!!res_length);
@@ -179,12 +157,12 @@ class Vfs_server::Session_component :
 		{
 			Packet_descriptor packet = tx_sink()->get_packet();
 
-			_process_packet_op(packet);
 
 			/*
 			 * The 'acknowledge_packet' function cannot block because we
 			 * checked for 'ready_to_ack' in '_process_packets'.
 			 */
+			_process_packet_op(packet);
 			tx_sink()->acknowledge_packet(packet);
 		}
 
@@ -232,6 +210,18 @@ class Vfs_server::Session_component :
 					throw Invalid_name();
 		}
 
+		void _close(Node &node)
+		{
+			if (File *file = dynamic_cast<File*>(&node))
+				destroy(_alloc, file);
+			else if (Directory *dir = dynamic_cast<Directory*>(&node))
+				destroy(_alloc, dir);
+			else if (Symlink *link = dynamic_cast<Symlink*>(&node))
+				destroy(_alloc, link);
+			else
+				destroy(_alloc, &node);
+		}
+
 	public:
 
 		/**
@@ -252,29 +242,23 @@ class Vfs_server::Session_component :
 		                  bool                  writable)
 		:
 			Session_rpc_object(env.ram().alloc(tx_buf_size), env.rm(), env.ep().rpc_ep()),
-			_label(label),
+			_label(label), _ram(env), _alloc(_ram, env.rm()),
-			_process_packet_dispatcher(env.ep(), *this, &Session_component::_process_packets),
+			_process_packet_handler(env.ep(), *this, &Session_component::_process_packets),
 			_vfs(vfs),
-			_root(vfs, root_path, false),
+			_root(),
 			_writable(writable)
 		{
 			/*
 			 * Register '_process_packets' dispatch function as signal
 			 * handler for packet-avail and ready-to-ack signals.
 			 */
-			_tx.sigh_packet_avail(_process_packet_dispatcher);
+			_tx.sigh_packet_avail(_process_packet_handler);
-			_tx.sigh_ready_to_ack(_process_packet_dispatcher);
+			_tx.sigh_ready_to_ack(_process_packet_handler);
 
-			/*
+			_ram.ref_account(env.ram_session_cap());
-			 * the '/' node is not dynamically allocated, so it is
+			env.ram().transfer_quota(_ram.cap(), ram_quota);
-			 * permanently bound to Dir_handle(0);
-			 */
-			_nodes[0] = &_root;
-			for (unsigned i = 1; i < MAX_NODE_HANDLES; ++i)
-				_nodes[i] = nullptr;
 
-			_ram.ref_account(Genode::env()->ram_session_cap());
+			_root.construct(_node_space, vfs, root_path, false);
-			Genode::env()->ram_session()->transfer_quota(_ram.cap(), ram_quota);
 		}
 
 		/**
@@ -282,8 +266,11 @@ class Vfs_server::Session_component :
 		 */
 		~Session_component()
 		{
-			Dataspace_capability ds = tx_sink()->dataspace();
+			/* remove the root from _node_space via destructor */
-			env()->ram_session()->free(static_cap_cast<Genode::Ram_dataspace>(ds));
+			_root.destruct();
+
+			while (_node_space.apply_any<Node>([&] (Node &node) {
+				_close(node); })) { }
 		}
 
 		void upgrade(char const *args)
@@ -311,22 +298,18 @@ class Vfs_server::Session_component :
 			}
 
 			_assert_valid_path(path_str);
-			Vfs_server::Path fullpath(_root.path());
+			Vfs_server::Path fullpath(_root->path());
 			fullpath.append(path_str);
 			path_str = fullpath.base();
 
-			/* make sure a handle is free before allocating */
-			auto slot = _next_slot();
-
 			if (!create && !_vfs.directory(path_str))
 				throw Lookup_failed();
 
 			Directory *dir;
-			try { dir = new (_alloc) Directory(_vfs, path_str, create); }
+			try { dir = new (_alloc) Directory(_node_space, _vfs, path_str, create); }
 			catch (Out_of_memory) { throw Out_of_metadata(); }
 
-			_nodes[slot] = dir;
+			return Dir_handle(dir->id().value);
-			return Dir_handle(slot);
 		}
 
 		File_handle file(Dir_handle dir_handle, Name const &name,
@@ -335,37 +318,33 @@ class Vfs_server::Session_component :
 			if ((create || (fs_mode & WRITE_ONLY)) && (!_writable))
 				throw Permission_denied();
 
-			Directory &dir = _lookup(dir_handle);
+			File_handle new_handle;
 
-			char const *name_str = name.string();
+			_apply(dir_handle, [&] (Directory &dir) {
-			_assert_valid_name(name_str);
+				char const *name_str = name.string();
+				_assert_valid_name(name_str);
 
-			/* make sure a handle is free before allocating */
+				new_handle = dir.file(
-			auto slot = _next_slot();
+					_node_space, _vfs, _alloc, name_str, fs_mode, create).value;
-
+			});
-			File *file = dir.file(_vfs, _alloc, name_str, fs_mode, create);
+			return new_handle;
-
-			_nodes[slot] = file;
-			return File_handle(slot);
 		}
 
 		Symlink_handle symlink(Dir_handle dir_handle, Name const &name, bool create) override
 		{
 			if (create && !_writable) throw Permission_denied();
 
-			Directory &dir = _lookup(dir_handle);
+			Symlink_handle new_handle;
 
-			char const *name_str = name.string();
+			_apply(dir_handle, [&] (Directory &dir) {
-			_assert_valid_name(name_str);
+				char const *name_str = name.string();
+				_assert_valid_name(name_str);
 
-			/* make sure a handle is free before allocating */
+				new_handle = dir.symlink(
-			auto slot = _next_slot();
+					_node_space, _vfs, _alloc, name_str,
-
+					_writable ? READ_WRITE : READ_ONLY, create).value;
-			Symlink *link = dir.symlink(_vfs, _alloc, name_str,
+			});
-				_writable ? READ_WRITE : READ_ONLY, create);
+			return new_handle;
-
-			_nodes[slot] = link;
-			return Symlink_handle(slot);
 		}
 
 		Node_handle node(File_system::Path const &path) override
@@ -378,90 +357,61 @@ class Vfs_server::Session_component :
 			_assert_valid_path(path_str);
 
 			/* re-root the path */
-			Path sub_path(path_str+1, _root.path());
+			Path sub_path(path_str+1, _root->path());
 			path_str = sub_path.base();
 			if (!_vfs.leaf_path(path_str))
 				throw Lookup_failed();
 
-			auto slot = _next_slot();
 			Node *node;
 
-			try { node  = new (_alloc) Node(path_str, STAT_ONLY); }
+			try { node  = new (_alloc) Node(_node_space, path_str, STAT_ONLY); }
 			catch (Out_of_memory) { throw Out_of_metadata(); }
 
-			_nodes[slot] = node;
+			return Node_handle(node->id().value);
-			return Node_handle(slot);
 		}
 
 		void close(Node_handle handle) override
 		{
-			/* handle '0' cannot be freed */
+			_apply(handle, [&] (Node &node) {
-			if (!handle.value) {
+				/* root directory should not be freed */
-				_root.notify_listeners();
+				if (!(node.id() == _root->id()))
-				return;
+					_close(node);
-			}
+			});
-
-			if (!_in_range(handle.value))
-				return;
-
-			Node *node = _nodes[handle.value];
-			if (!node) { return; }
-
-			node->notify_listeners();
-
-			/*
-			 * De-allocate handle
-			 */
-			Listener &listener = _listeners[handle.value];
-
-			if (listener.valid())
-				node->remove_listener(&listener);
-
-			if (File *file = dynamic_cast<File*>(node))
-				destroy(_alloc, file);
-			else if (Directory *dir = dynamic_cast<Directory*>(node))
-				destroy(_alloc, dir);
-			else if (Symlink *link = dynamic_cast<Symlink*>(node))
-				destroy(_alloc, link);
-			else
-				destroy(_alloc, node);
-
-			_nodes[handle.value] = 0;
-			listener = Listener();
 		}
 
 		Status status(Node_handle node_handle) override
 		{
-			Directory_service::Stat vfs_stat;
 			File_system::Status      fs_stat;
 
-			Node &node = _lookup(node_handle);
+			_apply(node_handle, [&] (Node &node) {
+				Directory_service::Stat vfs_stat;
 
-			if (_vfs.stat(node.path(), vfs_stat) != Directory_service::STAT_OK)
+				if (_vfs.stat(node.path(), vfs_stat) != Directory_service::STAT_OK)
-				return fs_stat;
+					return;
 
-			fs_stat.inode = vfs_stat.inode;
+				fs_stat.inode = vfs_stat.inode;
 
-			switch (vfs_stat.mode & (
+				switch (vfs_stat.mode & (
-				Directory_service::STAT_MODE_DIRECTORY |
+					Directory_service::STAT_MODE_DIRECTORY |
-				Directory_service::STAT_MODE_SYMLINK |
+					Directory_service::STAT_MODE_SYMLINK |
-				File_system::Status::MODE_FILE)) {
+					File_system::Status::MODE_FILE)) {
 
-			case Directory_service::STAT_MODE_DIRECTORY:
+				case Directory_service::STAT_MODE_DIRECTORY:
-				fs_stat.mode = File_system::Status::MODE_DIRECTORY;
+					fs_stat.mode = File_system::Status::MODE_DIRECTORY;
-				fs_stat.size = _vfs.num_dirent(node.path()) * sizeof(Directory_entry);
+					fs_stat.size = _vfs.num_dirent(node.path()) * sizeof(Directory_entry);
-				return fs_stat;
+					return;
 
-			case Directory_service::STAT_MODE_SYMLINK:
+				case Directory_service::STAT_MODE_SYMLINK:
-				fs_stat.mode = File_system::Status::MODE_SYMLINK;
+					fs_stat.mode = File_system::Status::MODE_SYMLINK;
-				break;
+					break;
 
-			default: /* Directory_service::STAT_MODE_FILE */
+				default: /* Directory_service::STAT_MODE_FILE */
-				fs_stat.mode = File_system::Status::MODE_FILE;
+					fs_stat.mode = File_system::Status::MODE_FILE;
-				break;
+					break;
-			}
+				}
 
-			fs_stat.size = vfs_stat.size;
+				fs_stat.size = vfs_stat.size;
+			});
 			return fs_stat;
 		}
 
@@ -469,19 +419,20 @@ class Vfs_server::Session_component :
 		{
 			if (!_writable) throw Permission_denied();
 
-			Directory &dir = _lookup(dir_handle);
+			_apply(dir_handle, [&] (Directory &dir) {
+				char const *name_str = name.string();
+				_assert_valid_name(name_str);
 
-			char const *name_str = name.string();
+				Path path(name_str, dir.path());
-			_assert_valid_name(name_str);
 
-			Path path(name_str, dir.path());
+				assert_unlink(_vfs.unlink(path.base()));
-
+				dir.mark_as_updated();
-			assert_unlink(_vfs.unlink(path.base()));
+			});
-			dir.mark_as_updated();
 		}
 
 		void truncate(File_handle file_handle, file_size_t size) override {
-			_lookup(file_handle).truncate(size); }
+			_apply(file_handle, [&] (File &file) {
+				file.truncate(size); }); }
 
 		void move(Dir_handle from_dir_handle, Name const &from_name,
 		          Dir_handle to_dir_handle,   Name const &to_name) override
@@ -495,53 +446,29 @@ class Vfs_server::Session_component :
 			_assert_valid_name(from_str);
 			_assert_valid_name(  to_str);
 
-			Directory &from_dir = _lookup(from_dir_handle);
+			_apply(from_dir_handle, [&] (Directory &from_dir) {
-			Directory   &to_dir = _lookup(  to_dir_handle);
+				_apply(to_dir_handle, [&] (Directory &to_dir) {
+					Path from_path(from_str, from_dir.path());
+					Path   to_path(  to_str,   to_dir.path());
 
-			Path from_path(from_str, from_dir.path());
+					assert_rename(_vfs.rename(from_path.base(), to_path.base()));
-			Path   to_path(  to_str,   to_dir.path());
 
-			assert_rename(_vfs.rename(from_path.base(), to_path.base()));
+					from_dir.mark_as_updated();
-
+					to_dir.mark_as_updated();
-			from_dir.mark_as_updated();
+				});
-			to_dir.mark_as_updated();
+			});
 		}
 
-		void sigh(Node_handle handle, Signal_context_capability sigh) override
+		void sigh(Node_handle handle, Signal_context_capability sigh) override { }
-		{
-			if (!_in_range(handle.value))
-				throw Invalid_handle();
-
-			Node *node = dynamic_cast<Node *>(_nodes[handle.value]);
-			if (!node)
-				throw Invalid_handle();
-
-			Listener &listener = _listeners[handle.value];
-
-			/*
-			 * If there was already a handler registered for the node,
-			 * remove the old handler.
-			 */
-			if (listener.valid())
-				node->remove_listener(&listener);
-
-			/*
-			 * Register new handler
-			 */
-			listener = Listener(sigh);
-			node->add_listener(&listener);
-		}
 
 		/**
 		 * Sync the VFS and send any pending signals on the node.
 		 */
 		void sync(Node_handle handle) override
 		{
-			try {
+			_apply(handle, [&] (Node &node) {
-				Node &node = _lookup(handle);
 				_vfs.sync(node.path());
-				node.notify_listeners();
+			});
-			} catch (Invalid_handle) { }
 		}
 
 		void control(Node_handle, Control) override { }

repos/os/src/server/vfs/node.h

@@ -18,6 +18,7 @@
 #include <file_system/node.h>
 #include <vfs/file_system.h>
 #include <os/path.h>
+#include <base/id_space.h>
 
 /* Local includes */
 #include "assert.h"
@@ -32,6 +33,8 @@ namespace Vfs_server {
 	struct File;
 	struct Symlink;
 
+	typedef Genode::Id_space<Node> Node_space;
+
 	/* Vfs::MAX_PATH is shorter than File_system::MAX_PATH */
 	enum { MAX_PATH_LEN = Vfs::MAX_PATH_LEN };
 
@@ -67,13 +70,16 @@ namespace Vfs_server {
 }
 
 
-struct Vfs_server::Node : File_system::Node_base
+struct Vfs_server::Node : File_system::Node_base, Node_space::Element
 {
 	Path const _path;
 	Mode const  mode;
 
-	Node(char const *node_path, Mode node_mode)
+	Node(Node_space &space, char const *node_path, Mode node_mode)
-	: _path(node_path), mode(node_mode) { }
+	:
+		Node_space::Element(*this, space),
+		_path(node_path), mode(node_mode)
+	{ }
 
 	virtual ~Node() { }
 
@@ -86,11 +92,12 @@ struct Vfs_server::Node : File_system::Node_base
 
 struct Vfs_server::Symlink : Node
 {
-	Symlink(Vfs::File_system &vfs,
+	Symlink(Node_space &space,
+	        Vfs::File_system &vfs,
 	        char       const *link_path,
 	        Mode              mode,
 	        bool              create)
-	: Node(link_path, mode)
+	: Node(space, link_path, mode)
 	{
 		if (create)
 			assert_symlink(vfs.symlink("", link_path));
@@ -132,12 +139,13 @@ class Vfs_server::File : public Node
 
 	public:
 
-		File(Vfs::File_system  &vfs,
+		File(Node_space &space,
+		     Vfs::File_system  &vfs,
 		     Genode::Allocator &alloc,
 		     char       const  *file_path,
 		     Mode               fs_mode,
 		     bool               create)
-		: Node(file_path, fs_mode)
+		: Node(space, file_path, fs_mode)
 		{
 			unsigned vfs_mode =
 				(fs_mode-1) | (create ? Vfs::Directory_service::OPEN_MODE_CREATE : 0);
@@ -201,35 +209,37 @@ class Vfs_server::File : public Node
 
 struct Vfs_server::Directory : Node
 {
-	Directory(Vfs::File_system &vfs, char const *dir_path, bool create)
+	Directory(Node_space &space, Vfs::File_system &vfs, char const *dir_path, bool create)
-	: Node(dir_path, READ_ONLY)
+	: Node(space, dir_path, READ_ONLY)
 	{
 		if (create)
 			assert_mkdir(vfs.mkdir(dir_path, 0));
 	}
 
-	File *file(Vfs::File_system  &vfs,
+	Node_space::Id file(Node_space        &space,
-	           Genode::Allocator &alloc,
+	                    Vfs::File_system  &vfs,
-	           char        const *file_path,
+	                    Genode::Allocator &alloc,
-	           Mode               mode,
+	                    char        const *file_path,
-	           bool               create)
+	                    Mode               mode,
+	                    bool               create)
 	{
 		Path subpath(file_path, path());
 		char const *path_str = subpath.base();
 
 		File *file;
-		try { file = new (alloc) File(vfs, alloc, path_str, mode, create); }
+		try { file = new (alloc) File(space, vfs, alloc, path_str, mode, create); }
 		catch (Out_of_memory) { throw Out_of_metadata(); }
 		if (create)
 			mark_as_updated();
-		return file;
+		return file->id();
 	}
 
-	Symlink *symlink(Vfs::File_system  &vfs,
+	Node_space::Id symlink(Node_space        &space,
-	                 Genode::Allocator &alloc,
+	                       Vfs::File_system  &vfs,
-	                 char        const *link_path,
+	                       Genode::Allocator &alloc,
-	                 Mode               mode,
+	                       char        const *link_path,
-	                 bool               create)
+	                       Mode               mode,
+	                       bool               create)
 	{
 		Path subpath(link_path, path());
 		char const *path_str = subpath.base();
@@ -240,11 +250,11 @@ struct Vfs_server::Directory : Node
 		}
 
 		Symlink *link;
-		try { link = new (alloc) Symlink(vfs, path_str, mode, create); }
+		try { link = new (alloc) Symlink(space, vfs, path_str, mode, create); }
 		catch (Out_of_memory) { throw Out_of_metadata(); }
 		if (create)
 			mark_as_updated();
-		return link;
+		return link->id();
 	}
 
 

repos/os/src/server/vfs/target.mk

@@ -1,4 +1,4 @@
 TARGET   = vfs
 SRC_CC   = main.cc
-LIBS     = base config vfs
+LIBS     = base vfs
 INC_DIR += $(PRG_DIR)