[Libguestfs] [nbdkit PATCH 5/5] ext2: Support parallel requests and connections

Instead of serializing connections (which in turn serializes requests) because it is unsafe to open more than one handle into the filesystem at the same time, use the recently-added APIs to open the filesystem during .after_fork, then use the fact that ext2 code then supports parallel access to files within the single system handle. --- filters/ext2/ext2.c | 225 ++++++++++++++++++++++++++------------------ 1 file changed, 131 insertions(+), 94 deletions(-) diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c index cc80458f..5b047246 100644 --- a/filters/ext2/ext2.c +++ b/filters/ext2/ext2.c @@ -36,6 +36,7 @@ #include <stdlib.h> #include <inttypes.h> #include <errno.h> +#include <stdbool.h> /* Inlining is broken in the ext2fs header file. Disable it by * defining the following: @@ -55,6 +56,14 @@ */ static const char *file; +/* Filesystem handle, shared between all client connections. */ +static ext2_filsys fs; + +/* Plugin access shared between all client connections, and doubles as + * the "name" parameter for ext2fs_open. + */ +static nbdkit_next *plugin; + static void ext2_load (void) { @@ -89,7 +98,8 @@ ext2_config_complete (nbdkit_next_config_complete *next, nbdkit_backend *nxdata) if (strcmp (file, "exportname") == 0) file = NULL; else if (file[0] != '/') { - nbdkit_error ("the file parameter must refer to an absolute path"); + nbdkit_error ("the file parameter must be 'exportname' or refer to " + "an absolute path"); return -1; } @@ -100,13 +110,89 @@ ext2_config_complete (nbdkit_next_config_complete *next, nbdkit_backend *nxdata) "ext2file=<FILENAME> (required) Absolute name of file to serve inside the\n" \ " disk image, or 'exportname' for client choice." +/* Opening more than one instance of the filesystem in parallel is a + * recipe for disaster, so instead we open a single instance during + * after_fork to share among all client connections. If the + * underlying plugin supports parallel requests, then we do too since + * ext2 code is re-entrant through our one open handle. + */ +static int +ext2_after_fork (nbdkit_backend *nxdata) +{ + CLEANUP_FREE char *name = NULL; + int fs_flags; + int64_t r; + errcode_t err; + + /* It would be desirable for ‘nbdkit -r’ to behave the same way as + * ‘mount -o ro’. But we don't know the state of the readonly flag + * until ext2_open is called, so for now we can't do that. We could + * add a knob during .config if desired; but until then, we blindly + * request write access to the underlying plugin, for journal + * replay. + * + * Similarly, there is no sane way to pass the client's exportname + * through to the plugin (whether or not .config was set to serve a + * single file or to let the client choose by exportname), so + * blindly ask for "" and rely on the plugin's default. + */ + plugin = nbdkit_next_context_open (nxdata, 0, "", true); + if (plugin == NULL) { + nbdkit_error ("unable to open plugin"); + return -1; + } + if (plugin->prepare (plugin) == -1) + goto fail; + + fs_flags = 0; +#ifdef EXT2_FLAG_64BITS + fs_flags |= EXT2_FLAG_64BITS; +#endif + r = plugin->get_size (plugin); + if (r == -1) + goto fail; + /* XXX See note above about a knob for read-only */ + r = plugin->can_write (plugin); + if (r == -1) + goto fail; + if (r == 1) + fs_flags |= EXT2_FLAG_RW; + + name = nbdkit_io_encode (plugin); + if (!name) { + nbdkit_error ("nbdkit_io_encode: %m"); + goto fail; + } + + err = ext2fs_open (name, fs_flags, 0, 0, nbdkit_io_manager, &fs); + if (err != 0) { + nbdkit_error ("open: %s", error_message (err)); + goto fail; + } + + return 0; + + fail: + plugin->finalize (plugin); + nbdkit_next_context_close (plugin); + return -1; +} + +static void +ext2_cleanup (nbdkit_backend *nxdata) +{ + if (fs) + ext2fs_close (fs); + plugin->finalize (plugin); + nbdkit_next_context_close (plugin); +} + /* The per-connection handle. */ struct handle { const char *exportname; /* Client export name. */ - ext2_filsys fs; /* Filesystem handle. */ ext2_ino_t ino; /* Inode of open file. */ ext2_file_t file; /* File handle. */ - nbdkit_next *next; /* "name" parameter to ext2fs_open. */ + nbdkit_context *context; }; /* Export list. */ @@ -117,18 +203,17 @@ ext2_list_exports (nbdkit_next_list_exports *next, nbdkit_backend *nxdata, /* If we are honoring export names, the default export "" won't * work, and we must not leak export names from the underlying * plugin. Advertising all filenames within the ext2 image could be - * huge, and even if we wanted to, it would require that we could - * open the plugin prior to the client reaching our .open. So leave - * the list empty instead. + * huge, although we could do it if we wanted to since the + * filesystem was already opened in .after_fork. */ if (!file) return 0; /* If we are serving a specific ext2file, we don't care what export - * name the user passes, but the underlying plugin might; there's no - * harm in advertising that list. + * name the user passes, but it's too late to pass that on to the + * underlying plugin, so advertise just "". */ - return next (nxdata, readonly, exports); + return nbdkit_use_default_export (exports); } /* Default export. */ @@ -170,17 +255,7 @@ ext2_open (nbdkit_next_open *next, nbdkit_context *nxdata, return NULL; } - /* If file == NULL (ie. using exportname) then don't - * pass the client exportname to the lower layers. - */ - exportname = file ? exportname : ""; - - /* Request write access to the underlying plugin, for journal replay. */ - if (next (nxdata, 0, exportname) == -1) { - free (h); - return NULL; - } - + h->context = nxdata; return h; } @@ -189,36 +264,12 @@ ext2_prepare (nbdkit_next *next, void *handle, int readonly) { struct handle *h = handle; errcode_t err; - int fs_flags; int file_flags; struct ext2_inode inode; - int64_t r; CLEANUP_FREE char *name = NULL; const char *fname = file ?: h->exportname; CLEANUP_FREE char *absname = NULL; - - fs_flags = 0; -#ifdef EXT2_FLAG_64BITS - fs_flags |= EXT2_FLAG_64BITS; -#endif - r = next->get_size (next); - if (r == -1) - return -1; - r = next->can_write (next); - if (r == -1) - return -1; - if (r == 0) - readonly = 1; - - if (!readonly) - fs_flags |= EXT2_FLAG_RW; - - h->next = next; - name = nbdkit_io_encode (next); - if (!name) { - nbdkit_error ("nbdkit_io_encode: %m"); - return -1; - } + nbdkit_next *old; if (fname[0] != '/') { if (asprintf (&absname, "/%s", fname) < 0) { @@ -228,53 +279,58 @@ ext2_prepare (nbdkit_next *next, void *handle, int readonly) fname = absname; } - err = ext2fs_open (name, fs_flags, 0, 0, nbdkit_io_manager, &h->fs); - if (err != 0) { - nbdkit_error ("open: %s", error_message (err)); - goto err0; - } - if (strcmp (fname, "/") == 0) /* probably gonna fail, but we'll catch it later */ h->ino = EXT2_ROOT_INO; else { - err = ext2fs_namei (h->fs, EXT2_ROOT_INO, EXT2_ROOT_INO, + err = ext2fs_namei (fs, EXT2_ROOT_INO, EXT2_ROOT_INO, &fname[1], &h->ino); if (err != 0) { nbdkit_error ("%s: namei: %s", fname, error_message (err)); - goto err1; + return -1; } } /* Check that fname is a regular file. * XXX This won't follow symlinks, we'd have to do that manually. */ - err = ext2fs_read_inode (h->fs, h->ino, &inode); + err = ext2fs_read_inode (fs, h->ino, &inode); if (err != 0) { nbdkit_error ("%s: inode: %s", fname, error_message (err)); - goto err1; + return -1; } if (!LINUX_S_ISREG (inode.i_mode)) { nbdkit_error ("%s: must be a regular file in the disk image", fname); - goto err1; + return -1; } file_flags = 0; if (!readonly) file_flags |= EXT2_FILE_WRITE; - err = ext2fs_file_open2 (h->fs, h->ino, NULL, file_flags, &h->file); + err = ext2fs_file_open2 (fs, h->ino, NULL, file_flags, &h->file); if (err != 0) { nbdkit_error ("%s: open: %s", fname, error_message (err)); - goto err1; + return -1; } + /* Associate our shared backend with this connection, so we don't + * have to override every single callback function. + */ + old = nbdkit_context_set_next (h->context, plugin); + assert (old == NULL); return 0; +} - err1: - ext2fs_close (h->fs); - h->fs = NULL; - err0: - return -1; +static int +ext2_finalize (nbdkit_next *next, void *handle) +{ + struct handle *h = handle; + nbdkit_next *old; + + /* Ensure our shared plugin handle survives past this connection. */ + old = nbdkit_context_set_next (h->context, NULL); + assert (old == next); + return 0; } /* Free up the per-connection handle. */ @@ -283,10 +339,8 @@ ext2_close (void *handle) { struct handle *h = handle; - if (h->fs) { + if (h->file) ext2fs_file_close (h->file); - ext2fs_close (h->fs); - } free (h); } @@ -306,12 +360,10 @@ ext2_can_cache (nbdkit_next *next, void *handle) static int ext2_can_multi_conn (nbdkit_next *next, void *handle) { - /* Since we do not permit parallel connections, it does not matter - * what we advertise here, and we could just as easily inherit the - * plugin's .can_multi_conn. But realistically, if we adjust - * .thread_model, we cannot advertise support unless .flush is - * consistent, and that would require inspecting the ext2 source - * code, so for now, we hard-code a safe answer. + /* Although we permit parallel client connections multiplexed into + * the single shared filesystem handle, we would have to audit the + * ext2 source to learn if it is cache-consistent. So for now, + * hard-code a safe answer. */ return 0; } @@ -324,7 +376,7 @@ ext2_can_flush (nbdkit_next *next, void *handle) * plugin ability, since ext2 wants to flush the filesystem into * permanent storage when possible. */ - if (next->can_flush (next) == -1) + if (plugin->can_flush (plugin) == -1) return -1; return 1; } @@ -339,7 +391,7 @@ ext2_can_zero (nbdkit_next *next, void *handle) /* For now, tell nbdkit to call .pwrite instead of any optimization. * However, we also want to cache the underlying plugin support. */ - if (next->can_zero (next) == -1) + if (plugin->can_zero (plugin) == -1) return -1; return NBDKIT_ZERO_EMULATE; } @@ -350,28 +402,11 @@ ext2_can_trim (nbdkit_next *next, void *handle) /* For now, tell nbdkit to never call .trim. However, we also want * to cache the underlying plugin support. */ - if (next->can_trim (next) == -1) + if (plugin->can_trim (plugin) == -1) return -1; return 0; } -/* It might be possible to relax this, but it's complicated. - * - * It's desirable for ‘nbdkit -r’ to behave the same way as - * ‘mount -o ro’. But we don't know the state of the readonly flag - * until ext2_open is called (because the NBD client can also request - * a readonly connection). So we could not set the "ro" flag if we - * opened the filesystem any earlier (eg in ext2_config). - * - * So out of necessity we have one ext2_filsys handle per connection, - * but if we allowed parallel work on those handles then we would get - * data corruption, so we need to serialize connections. - */ -static int ext2_thread_model (void) -{ - return NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS; -} - /* Description. */ static const char * ext2_export_description (nbdkit_next *next, void *handle) @@ -379,7 +414,7 @@ ext2_export_description (nbdkit_next *next, void *handle) struct handle *h = handle; const char *fname = file ?: h->exportname; const char *slash = fname[0] == '/' ? "" : "/"; - const char *base = next->export_description (next); + const char *base = plugin->export_description (plugin); if (!base) return NULL; @@ -506,11 +541,13 @@ static struct nbdkit_filter filter = { .config = ext2_config, .config_complete = ext2_config_complete, .config_help = ext2_config_help, - .thread_model = ext2_thread_model, + .after_fork = ext2_after_fork, + .cleanup = ext2_cleanup, .list_exports = ext2_list_exports, .default_export = ext2_default_export, .open = ext2_open, .prepare = ext2_prepare, + .finalize = ext2_finalize, .close = ext2_close, .can_fua = ext2_can_fua, .can_cache = ext2_can_cache, -- 2.31.1