[Libguestfs] [PATCH v3 01/10] New API parameter: Add discard parameter to guestfs_add_drive_opts.

This adds a discard parameter to the guestfs_add_drive_opts which approximately maps to the discard=ignore|unmap parameter supported by qemu. If discard is set to "enable" then we force discard=unmap (and try to fail if it is not possible). If discard is set to the more useful "besteffort" option, then we enable discard if possible. The default is "disable". --- generator/actions.ml | 32 +++++++++++++- src/drives.c | 38 +++++++++++++--- src/guestfs-internal.h | 9 ++++ src/launch-direct.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++- src/launch-libvirt.c | 99 ++++++++++++++++++++++++++++++++++++++--- src/launch-uml.c | 6 +++ 6 files changed, 287 insertions(+), 14 deletions(-) diff --git a/generator/actions.ml b/generator/actions.ml index a6ef194..b25194c 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1286,7 +1286,7 @@ not all belong to a single logical operating system { defaults with name = "add_drive"; - style = RErr, [String "filename"], [OBool "readonly"; OString "format"; OString "iface"; OString "name"; OString "label"; OString "protocol"; OStringList "server"; OString "username"; OString "secret"; OString "cachemode"]; + style = RErr, [String "filename"], [OBool "readonly"; OString "format"; OString "iface"; OString "name"; OString "label"; OString "protocol"; OStringList "server"; OString "username"; OString "secret"; OString "cachemode"; OString "discard"]; once_had_no_optargs = true; blocking = false; fish_alias = ["add"]; @@ -1504,6 +1504,36 @@ for scratch or temporary disks. =back +=item C<discard> + +Enable or disable discard (a.k.a. trim or unmap) support on this +drive. If enabled, operations such as C<guestfs_fstrim> will be able +to discard / make thin / punch holes in the underlying host file +or device. + +Possible discard settings are: + +=over 4 + +=item C<discard = \"disable\"> + +Disable discard support. This is the default. + +=item C<discard = \"enable\"> + +Enable discard support. Fail if discard is not possible. + +=item C<discard = \"besteffort\"> + +Enable discard support if possible, but don't fail if it is not +supported. + +Since not all backends and not all underlying systems support +discard, this is a good choice if you want to use discard if +possible, but don't mind if it doesn't work. + +=back + =back" }; { defaults with diff --git a/src/drives.c b/src/drives.c index 2c776a2..57c2471 100644 --- a/src/drives.c +++ b/src/drives.c @@ -61,6 +61,7 @@ struct drive_create_data { const char *name; const char *disk_label; const char *cachemode; + enum discard discard; }; /* Compile all the regular expressions once when the shared library is @@ -144,6 +145,7 @@ create_drive_file (guestfs_h *g, drv->name = data->name ? safe_strdup (g, data->name) : NULL; drv->disk_label = data->disk_label ? safe_strdup (g, data->disk_label) : NULL; drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode) : NULL; + drv->discard = data->discard; if (data->readonly) { if (create_overlay (g, drv) == -1) { @@ -178,6 +180,7 @@ create_drive_non_file (guestfs_h *g, drv->name = data->name ? safe_strdup (g, data->name) : NULL; drv->disk_label = data->disk_label ? safe_strdup (g, data->disk_label) : NULL; drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode) : NULL; + drv->discard = data->discard; if (data->readonly) { if (create_overlay (g, drv) == -1) { @@ -473,6 +476,7 @@ create_drive_dev_null (guestfs_h *g, return NULL; data->exportname = tmpfile; + data->discard = discard_disable; return create_drive_file (g, data); } @@ -511,8 +515,8 @@ free_drive_struct (struct drive *drv) free (drv); } -static const char * -protocol_to_string (enum drive_protocol protocol) +const char * +guestfs___drive_protocol_to_string (enum drive_protocol protocol) { switch (protocol) { case drive_protocol_file: return "file"; @@ -538,12 +542,12 @@ static char * drive_to_string (guestfs_h *g, const struct drive *drv) { return safe_asprintf - (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s", + (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s", drv->src.u.path, drv->readonly ? " readonly" : "", drv->src.format ? " format=" : "", drv->src.format ? : "", - protocol_to_string (drv->src.protocol), + guestfs___drive_protocol_to_string (drv->src.protocol), drv->iface ? " iface=" : "", drv->iface ? : "", drv->name ? " name=" : "", @@ -551,7 +555,9 @@ drive_to_string (guestfs_h *g, const struct drive *drv) drv->disk_label ? " label=" : "", drv->disk_label ? : "", drv->cachemode ? " cache=" : "", - drv->cachemode ? : ""); + drv->cachemode ? : "", + drv->discard == discard_disable ? "" : + drv->discard == discard_enable ? " discard=enable" : " discard=besteffort"); } /* Add struct drive to the g->drives vector at the given index. */ @@ -795,6 +801,28 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename, data.cachemode = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_CACHEMODE_BITMASK ? optargs->cachemode : NULL; + if (optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_DISCARD_BITMASK) { + if (STREQ (optargs->discard, "disable")) + data.discard = discard_disable; + else if (STREQ (optargs->discard, "enable")) + data.discard = discard_enable; + else if (STREQ (optargs->discard, "besteffort")) + data.discard = discard_besteffort; + else { + error (g, _("discard parameter must be 'disable', 'enable' or 'besteffort'")); + free_drive_servers (data.servers, data.nr_servers); + return -1; + } + } + else + data.discard = discard_disable; + + if (data.readonly && data.discard == discard_enable) { + error (g, _("discard support cannot be enabled on read-only drives")); + free_drive_servers (data.servers, data.nr_servers); + return -1; + } + if (data.format && !valid_format_iface (data.format)) { error (g, _("%s parameter is empty or contains disallowed characters"), "format"); diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 545146f..95b09cf 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -231,6 +231,12 @@ struct drive_source { char *secret; }; +enum discard { + discard_disable = 0, + discard_enable, + discard_besteffort, +}; + /* There is one 'struct drive' per drive, including hot-plugged drives. */ struct drive { /* Original source of the drive, eg. file:..., http:... */ @@ -252,6 +258,7 @@ struct drive { char *name; char *disk_label; char *cachemode; + enum discard discard; }; /* Extra hv parameters (from guestfs_config). */ @@ -713,6 +720,7 @@ extern size_t guestfs___checkpoint_drives (guestfs_h *g); extern void guestfs___rollback_drives (guestfs_h *g, size_t); extern void guestfs___add_dummy_appliance_drive (guestfs_h *g); extern void guestfs___free_drives (guestfs_h *g); +extern const char *guestfs___drive_protocol_to_string (enum drive_protocol protocol); /* appliance.c */ extern int guestfs___build_appliance (guestfs_h *g, char **kernel, char **dtb, char **initrd, char **appliance); @@ -832,6 +840,7 @@ extern void guestfs___cleanup_cmd_close (struct command **); /* launch-direct.c */ extern char *guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src); +extern bool guestfs___discard_possible (guestfs_h *g, struct drive *drv, unsigned long qemu_version); /* utils.c */ extern int guestfs___validate_guid (const char *); diff --git a/src/launch-direct.c b/src/launch-direct.c index 1e84061..3d478c7 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -509,6 +509,30 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) CLEANUP_FREE char *file = NULL, *escaped_file = NULL, *param = NULL; if (!drv->overlay) { + const char *discard_mode = ""; + int major = data->qemu_version_major, minor = data->qemu_version_minor; + unsigned long qemu_version = major * 1000000 + minor * 1000; + + switch (drv->discard) { + case discard_disable: + /* Since the default is always discard=ignore, don't specify it + * on the command line. This also avoids unnecessary breakage + * with qemu < 1.5 which didn't have the option at all. + */ + break; + case discard_enable: + if (!guestfs___discard_possible (g, drv, qemu_version)) + goto cleanup0; + /*FALLTHROUGH*/ + case discard_besteffort: + /* I believe from reading the code that this is always safe as + * long as qemu >= 1.5. + */ + if (major > 1 || (major == 1 && minor >= 5)) + discard_mode = ",discard=unmap"; + break; + } + /* Make the file= parameter. */ file = guestfs___drive_source_qemu_param (g, &drv->src); escaped_file = qemu_escape_param (g, file); @@ -517,10 +541,11 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) * the if=... at the end. */ param = safe_asprintf - (g, "file=%s%s,cache=%s%s%s%s%s,id=hd%zu", + (g, "file=%s%s,cache=%s%s%s%s%s%s,id=hd%zu", escaped_file, drv->readonly ? ",snapshot=on" : "", drv->cachemode ? drv->cachemode : "writeback", + discard_mode, drv->src.format ? ",format=" : "", drv->src.format ? drv->src.format : "", drv->disk_label ? ",serial=" : "", @@ -1370,6 +1395,96 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src) abort (); } +/* Test if discard is both supported by qemu AND possible with the + * underlying file or device. This returns 1 if discard is possible. + * It returns 0 if not possible and sets the error to the reason why. + * + * This function is called when the user set discard == "enable". + * + * qemu_version is the version of qemu in the form returned by libvirt: + * major * 1,000,000 + minor * 1,000 + release + */ +bool +guestfs___discard_possible (guestfs_h *g, struct drive *drv, + unsigned long qemu_version) +{ + /* qemu >= 1.5. This was the first version that supported the + * discard option on -drive at all. + */ + bool qemu15 = qemu_version >= 1005000; + /* qemu >= 1.6. This was the first version that supported unmap on + * qcow2 backing files. + */ + bool qemu16 = qemu_version >= 1006000; + + if (!qemu15) { + error (g, _("discard cannot be enabled on this drive: " + "qemu < 1.5")); + return false; + } + + /* If it's an overlay, discard is not possible (on the underlying + * file). This has probably been caught earlier since we already + * checked that the drive is !readonly. Nevertheless ... + */ + if (drv->overlay) { + error (g, _("discard cannot be enabled on this drive: " + "the drive has a read-only overlay")); + return false; + } + + /* Look at the source format. */ + if (drv->src.format == NULL) { + /* We could autodetect the format, but we don't ... yet. XXX */ + error (g, _("discard cannot be enabled on this drive: " + "you have to specify the format of the file")); + return false; + } + else if (STREQ (drv->src.format, "raw")) + /* OK */ ; + else if (STREQ (drv->src.format, "qcow2")) { + if (!qemu16) { + error (g, _("discard cannot be enabled on this drive: " + "qemu < 1.6 cannot do discard on qcow2 files")); + return false; + } + } + else { + /* It's possible in future other formats will support discard, but + * currently (qemu 1.7) none of them do. + */ + error (g, _("discard cannot be enabled on this drive: " + "qemu does not support discard for '%s' format files"), + drv->src.format); + return false; + } + + switch (drv->src.protocol) { + /* Protocols which support discard. */ + case drive_protocol_file: + case drive_protocol_gluster: + case drive_protocol_iscsi: + case drive_protocol_nbd: + case drive_protocol_rbd: + case drive_protocol_sheepdog: /* XXX depends on server version */ + break; + + /* Protocols which don't support discard. */ + case drive_protocol_ftp: + case drive_protocol_ftps: + case drive_protocol_http: + case drive_protocol_https: + case drive_protocol_ssh: + case drive_protocol_tftp: + error (g, _("discard cannot be enabled on this drive: " + "protocol '%s' does not support discard"), + guestfs___drive_protocol_to_string (drv->src.protocol)); + return false; + } + + return true; +} + static int shutdown_direct (guestfs_h *g, void *datav, int check_for_errors) { diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index da1d665..e0049eb 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -107,6 +107,7 @@ struct backend_libvirt_data { bool selinux_norelabel_disks; char name[DOMAIN_NAME_LEN]; /* random name */ bool is_kvm; /* false = qemu, true = kvm (from capabilities)*/ + unsigned long qemu_version; /* qemu version (from libvirt) */ }; /* Parameters passed to construct_libvirt_xml and subfunctions. We @@ -131,6 +132,7 @@ static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct libvirt_xml_pa static void debug_appliance_permissions (guestfs_h *g); static void debug_socket_permissions (guestfs_h *g); static void libvirt_error (guestfs_h *g, const char *fs, ...) __attribute__((format (printf,2,3))); +static void libvirt_debug (guestfs_h *g, const char *fs, ...) __attribute__((format (printf,2,3))); static int is_custom_hv (guestfs_h *g); static int is_blk (const char *path); static void ignore_errors (void *ignore, virErrorPtr ignore2); @@ -283,6 +285,19 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) */ virConnSetErrorFunc (conn, NULL, ignore_errors); + /* Get hypervisor (hopefully qemu) version. */ + if (virConnectGetVersion (conn, &data->qemu_version) == 0) { + debug (g, "qemu version (reported by libvirt) = %lu (%lu.%lu.%lu)", + data->qemu_version, + data->qemu_version / 1000000UL, + data->qemu_version / 1000UL % 1000UL, + data->qemu_version % 1000UL); + } + else { + libvirt_debug (g, "unable to read qemu version from libvirt"); + data->qemu_version = 0; + } + if (g->verbose) guestfs___print_timestamped_message (g, "get libvirt capabilities"); @@ -789,7 +804,7 @@ static int construct_libvirt_xml_devices (guestfs_h *g, const struct libvirt_xml static int construct_libvirt_xml_qemu_cmdline (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo); static int construct_libvirt_xml_disk (guestfs_h *g, const struct backend_libvirt_data *data, xmlTextWriterPtr xo, struct drive *drv, size_t drv_index); static int construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr xo, size_t drv_index); -static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, xmlTextWriterPtr xo, const char *format, const char *cachemode); +static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, const struct backend_libvirt_data *data, struct drive *drv, xmlTextWriterPtr xo, const char *format, const char *cachemode, enum discard discard); static int construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr xo, size_t drv_index); static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g, xmlTextWriterPtr xo, const struct drive_source *src); static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const struct backend_libvirt_data *data, xmlTextWriterPtr xo); @@ -1233,7 +1248,9 @@ construct_libvirt_xml_disk (guestfs_h *g, if (construct_libvirt_xml_disk_target (g, xo, drv_index) == -1) return -1; - if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe") + if (construct_libvirt_xml_disk_driver_qemu (g, data, drv, + xo, "qcow2", "unsafe", + discard_disable) == -1) return -1; } @@ -1370,8 +1387,9 @@ construct_libvirt_xml_disk (guestfs_h *g, return -1; } - if (construct_libvirt_xml_disk_driver_qemu (g, xo, format, - drv->cachemode ? : "writeback") + if (construct_libvirt_xml_disk_driver_qemu (g, data, drv, xo, format, + drv->cachemode ? : "writeback", + drv->discard) == -1) return -1; } @@ -1407,14 +1425,46 @@ construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr xo, } static int -construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, xmlTextWriterPtr xo, +construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, + const struct backend_libvirt_data *data, + struct drive *drv, + xmlTextWriterPtr xo, const char *format, - const char *cachemode) + const char *cachemode, + enum discard discard) { + bool discard_unmap = false; + + /* When adding the appliance disk, we don't have a 'drv' struct. + * However the caller will use discard_disable, so we don't need it. + */ + assert (discard == discard_disable || drv != NULL); + + switch (discard) { + case discard_disable: + /* Since the default is always discard=ignore, don't specify it + * in the XML. + */ + break; + case discard_enable: + if (!guestfs___discard_possible (g, drv, data->qemu_version)) + return -1; + /*FALLTHROUGH*/ + case discard_besteffort: + /* I believe from reading the code that this is always safe as + * long as qemu >= 1.5. + */ + if (data->qemu_version >= 1005000) + discard_unmap = true; + break; + } + start_element ("driver") { attribute ("name", "qemu"); attribute ("type", format); attribute ("cache", cachemode); + if (discard_unmap) + attribute ("discard", "unmap"); } end_element (); return 0; @@ -1499,7 +1549,9 @@ construct_libvirt_xml_appliance (guestfs_h *g, attribute ("bus", "scsi"); } end_element (); - if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe") == -1) + if (construct_libvirt_xml_disk_driver_qemu (g, params->data, NULL, xo, + "qcow2", "unsafe", + discard_disable) == -1) return -1; if (construct_libvirt_xml_disk_address (g, xo, params->appliance_index) @@ -1659,6 +1711,39 @@ libvirt_error (guestfs_h *g, const char *fs, ...) free (msg); } +/* Same as 'libvirt_error' but calls debug instead. */ +static void +libvirt_debug (guestfs_h *g, const char *fs, ...) +{ + va_list args; + char *msg; + int len; + virErrorPtr err; + + if (!g->verbose) + return; + + va_start (args, fs); + len = vasprintf (&msg, fs, args); + va_end (args); + + if (len < 0) + msg = safe_asprintf (g, + _("%s: internal error forming error message"), + __func__); + + /* In all recent libvirt, this retrieves the thread-local error. */ + err = virGetLastError (); + if (err) + debug (g, "%s: %s [code=%d domain=%d]", + msg, err->message, err->code, err->domain); + else + debug (g, "%s", msg); + + /* NB. 'err' must not be freed! */ + free (msg); +} + #if HAVE_LIBSELINUX static void selinux_warning (guestfs_h *g, const char *func, diff --git a/src/launch-uml.c b/src/launch-uml.c index deda366..2a6ddaf 100644 --- a/src/launch-uml.c +++ b/src/launch-uml.c @@ -122,6 +122,12 @@ uml_supported (guestfs_h *g) _("uml backend does not support drives with 'label' parameter")); return false; } + /* Note that discard == "besteffort" is fine. */ + if (drv->discard == discard_enable) { + error (g, + _("uml backend does not support drives with 'discard' parameter set to 'enable'")); + return false; + } } return true; -- 1.8.5.3