These patches contain the beginnings of discard (a.k.a. trim or unmap) support. This will allow us to change virt-sparsify to work on disk images in-place (instead of using slow & inefficient copying). The approach used is to add an optional 'discard' parameter to add-drive. It has 3 possible settings: - 'disable' : the default, no discard is done - 'besteffort' : try to enable it, but don't fail if it's not supported - 'enable' : enable it, and fail if it is not supported I'm not sure if we should wait for post-1.26 before adding discard support. It depends if the interface above is suitable or if we'll have to change it later. However I discussed how to support this at DevConf and this approach was generally agreed. Rich.
Richard W.M. Jones
2014-Mar-10 17:28 UTC
[Libguestfs] [PATCH 1/3] launch: libvirt: Move the is_kvm flag (derived from libvirt capabilities) to backend data struct.
This is just rearranging the data between structs. There should be no
functional change.
---
src/launch-libvirt.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index fd6e6d2..d86e2d2 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -106,6 +106,7 @@ struct backend_libvirt_data {
char *selinux_imagelabel;
bool selinux_norelabel_disks;
char name[DOMAIN_NAME_LEN]; /* random name */
+ bool is_kvm; /* false = qemu, true = kvm (from
capabilities)*/
};
/* Parameters passed to construct_libvirt_xml and subfunctions. We
@@ -122,12 +123,11 @@ struct libvirt_xml_params {
char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */
char console_path[UNIX_PATH_MAX];
bool enable_svirt; /* false if we decided to disable sVirt */
- bool is_kvm; /* false = qemu, true = kvm */
bool current_proc_is_root; /* true = euid is root */
};
-static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct libvirt_xml_params *params);
-static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params);
+static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct backend_libvirt_data *data);
+static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params);
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)));
@@ -299,7 +299,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
if (g->verbose)
guestfs___print_timestamped_message (g, "parsing capabilities
XML");
- if (parse_capabilities (g, capabilities_xml, ¶ms) == -1)
+ if (parse_capabilities (g, capabilities_xml, data) == -1)
goto cleanup;
/* Locate and/or build the appliance. */
@@ -434,7 +434,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
guestfs___drive_name (params.appliance_index, ¶ms.appliance_dev[7]);
params.enable_svirt = ! is_custom_hv (g);
- xml = construct_libvirt_xml (g, ¶ms);
+ xml = construct_libvirt_xml (g, data, ¶ms);
if (!xml)
goto cleanup;
@@ -575,7 +575,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
static int
parse_capabilities (guestfs_h *g, const char *capabilities_xml,
- struct libvirt_xml_params *params)
+ struct backend_libvirt_data *data)
{
CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
@@ -654,9 +654,9 @@ parse_capabilities (guestfs_h *g, const char
*capabilities_xml,
force_tcg = guestfs___get_backend_setting_bool (g, "force_tcg");
if (!force_tcg)
- params->is_kvm = seen_kvm;
+ data->is_kvm = seen_kvm;
else
- params->is_kvm = 0;
+ data->is_kvm = 0;
return 0;
}
@@ -779,10 +779,10 @@ debug_socket_permissions (guestfs_h *g)
}
}
-static int construct_libvirt_xml_domain (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
+static int construct_libvirt_xml_domain (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
static int construct_libvirt_xml_name (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
-static int construct_libvirt_xml_cpu (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
-static int construct_libvirt_xml_boot (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
+static int construct_libvirt_xml_cpu (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
+static int construct_libvirt_xml_boot (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
static int construct_libvirt_xml_seclabel (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
static int construct_libvirt_xml_lifecycle (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
static int construct_libvirt_xml_devices (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
@@ -872,7 +872,9 @@ static int construct_libvirt_xml_appliance (guestfs_h *g,
const struct libvirt_x
__FILE__, __LINE__, (fn));
static xmlChar *
-construct_libvirt_xml (guestfs_h *g, const struct libvirt_xml_params *params)
+construct_libvirt_xml (guestfs_h *g,
+ const struct backend_libvirt_data *data,
+ const struct libvirt_xml_params *params)
{
xmlChar *ret = NULL;
CLEANUP_XMLBUFFERFREE xmlBufferPtr xb = NULL;
@@ -905,7 +907,7 @@ construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params)
return NULL;
}
- if (construct_libvirt_xml_domain (g, params, xo) == -1)
+ if (construct_libvirt_xml_domain (g, data, params, xo) == -1)
return NULL;
if (xmlTextWriterEndDocument (xo) == -1) {
@@ -925,19 +927,20 @@ construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params)
static int
construct_libvirt_xml_domain (guestfs_h *g,
+ const struct backend_libvirt_data *data,
const struct libvirt_xml_params *params,
xmlTextWriterPtr xo)
{
start_element ("domain") {
- attribute ("type", params->is_kvm ? "kvm" :
"qemu");
+ attribute ("type", data->is_kvm ? "kvm" :
"qemu");
attribute_ns ("xmlns", "qemu", NULL,
"http://libvirt.org/schemas/domain/qemu/1.0");
if (construct_libvirt_xml_name (g, params, xo) == -1)
return -1;
- if (construct_libvirt_xml_cpu (g, params, xo) == -1)
+ if (construct_libvirt_xml_cpu (g, data, params, xo) == -1)
return -1;
- if (construct_libvirt_xml_boot (g, params, xo) == -1)
+ if (construct_libvirt_xml_boot (g, data, params, xo) == -1)
return -1;
if (construct_libvirt_xml_seclabel (g, params, xo) == -1)
return -1;
@@ -968,6 +971,7 @@ construct_libvirt_xml_name (guestfs_h *g,
/* CPU and memory features. */
static int
construct_libvirt_xml_cpu (guestfs_h *g,
+ const struct backend_libvirt_data *data,
const struct libvirt_xml_params *params,
xmlTextWriterPtr xo)
{
@@ -987,7 +991,7 @@ construct_libvirt_xml_cpu (guestfs_h *g,
* Only do this with KVM. It is broken in subtle ways on TCG, and
* fairly pointless anyway.
*/
- if (params->is_kvm) {
+ if (data->is_kvm) {
start_element ("cpu") {
attribute ("mode", "host-passthrough");
start_element ("model") {
@@ -1031,6 +1035,7 @@ construct_libvirt_xml_cpu (guestfs_h *g,
/* Boot parameters. */
static int
construct_libvirt_xml_boot (guestfs_h *g,
+ const struct backend_libvirt_data *data,
const struct libvirt_xml_params *params,
xmlTextWriterPtr xo)
{
@@ -1039,7 +1044,7 @@ construct_libvirt_xml_boot (guestfs_h *g,
/* Linux kernel command line. */
flags = 0;
- if (!params->is_kvm)
+ if (!data->is_kvm)
flags |= APPLIANCE_COMMAND_LINE_IS_TCG;
cmdline = guestfs___appliance_command_line (g, params->appliance_dev,
flags);
--
1.8.5.3
Richard W.M. Jones
2014-Mar-10 17:28 UTC
[Libguestfs] [PATCH 2/3] 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 | 107 +++++++++++++++++++++++++++++++----------------
src/guestfs-internal.h | 9 ++++
src/launch-direct.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++-
src/launch-libvirt.c | 96 ++++++++++++++++++++++++++++++++++++++----
src/launch-uml.c | 6 +++
6 files changed, 316 insertions(+), 44 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 2c85b52..68e37f7 100644
--- a/src/drives.c
+++ b/src/drives.c
@@ -115,7 +115,8 @@ static struct drive *
create_drive_file (guestfs_h *g, const char *path,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
struct drive *drv = safe_calloc (g, 1, sizeof *drv);
@@ -128,6 +129,7 @@ create_drive_file (guestfs_h *g, const char *path,
drv->name = name ? safe_strdup (g, name) : NULL;
drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
+ drv->discard = discard;
if (readonly) {
if (create_overlay (g, drv) == -1) {
@@ -151,7 +153,8 @@ create_drive_non_file (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
struct drive *drv = safe_calloc (g, 1, sizeof *drv);
@@ -168,6 +171,7 @@ create_drive_non_file (guestfs_h *g,
drv->name = name ? safe_strdup (g, name) : NULL;
drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
+ drv->discard = discard;
if (readonly) {
if (create_overlay (g, drv) == -1) {
@@ -191,7 +195,8 @@ create_drive_curl (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
if (secret != NULL) {
error (g, _("curl: you cannot specify a secret with this
protocol"));
@@ -223,7 +228,7 @@ create_drive_curl (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static struct drive *
@@ -233,7 +238,8 @@ create_drive_gluster (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
if (username != NULL) {
error (g, _("gluster: you cannot specify a username with this
protocol"));
@@ -263,7 +269,7 @@ create_drive_gluster (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static int
@@ -285,7 +291,8 @@ create_drive_nbd (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
if (username != NULL) {
error (g, _("nbd: you cannot specify a username with this
protocol"));
@@ -308,7 +315,7 @@ create_drive_nbd (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static struct drive *
@@ -318,7 +325,8 @@ create_drive_rbd (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
size_t i;
@@ -348,7 +356,7 @@ create_drive_rbd (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static struct drive *
@@ -358,7 +366,8 @@ create_drive_sheepdog (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
size_t i;
@@ -397,7 +406,7 @@ create_drive_sheepdog (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static struct drive *
@@ -407,7 +416,8 @@ create_drive_ssh (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
if (secret != NULL) {
error (g, _("ssh: you cannot specify a secret with this
protocol"));
@@ -444,7 +454,7 @@ create_drive_ssh (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
static struct drive *
@@ -454,7 +464,8 @@ create_drive_iscsi (guestfs_h *g,
const char *username, const char *secret,
bool readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label, const char *cachemode)
+ const char *disk_label, const char *cachemode,
+ enum discard discard)
{
if (username != NULL) {
error (g, _("iscsi: you cannot specify a username with this
protocol"));
@@ -491,7 +502,7 @@ create_drive_iscsi (guestfs_h *g,
servers, nr_servers, exportname,
username, secret,
readonly, format, iface, name, disk_label,
- cachemode);
+ cachemode, discard);
}
/* Traditionally you have been able to use /dev/null as a filename, as
@@ -528,14 +539,15 @@ create_drive_dev_null (guestfs_h *g, bool readonly, const
char *format,
return NULL;
return create_drive_file (g, tmpfile, readonly, format, iface, name,
- disk_label, 0);
+ disk_label, NULL, discard_disable);
}
static struct drive *
create_drive_dummy (guestfs_h *g)
{
/* A special drive struct that is used as a dummy slot for the appliance. */
- return create_drive_file (g, "", 0, NULL, NULL, NULL, NULL, 0);
+ return create_drive_file (g, "", 0, NULL, NULL, NULL, NULL, NULL,
+ discard_disable);
}
static void
@@ -563,8 +575,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";
@@ -590,12 +602,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=" : "",
@@ -603,7 +615,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. */
@@ -824,6 +838,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
const char *username;
const char *secret;
const char *cachemode;
+ enum discard discard;
struct drive *drv;
size_t i, drv_index;
@@ -852,6 +867,28 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
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"))
+ discard = discard_disable;
+ else if (STREQ (optargs->discard, "enable"))
+ discard = discard_enable;
+ else if (STREQ (optargs->discard, "besteffort"))
+ discard = discard_besteffort;
+ else {
+ error (g, _("discard parameter must be 'disable',
'enable' or 'besteffort'"));
+ free_drive_servers (servers, nr_servers);
+ return -1;
+ }
+ }
+ else
+ discard = discard_disable;
+
+ if (readonly && discard == discard_enable) {
+ error (g, _("discard support cannot be enabled on read-only
drives"));
+ free_drive_servers (servers, nr_servers);
+ return -1;
+ }
+
if (format && !valid_format_iface (format)) {
error (g, _("%s parameter is empty or contains disallowed
characters"),
"format");
@@ -904,7 +941,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
}
drv = create_drive_file (g, filename, readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
}
else if (STREQ (protocol, "ftp")) {
@@ -912,71 +949,71 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "ftps")) {
drv = create_drive_curl (g, drive_protocol_ftps,
servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "gluster")) {
drv = create_drive_gluster (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "http")) {
drv = create_drive_curl (g, drive_protocol_http,
servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "https")) {
drv = create_drive_curl (g, drive_protocol_https,
servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "iscsi")) {
drv = create_drive_iscsi (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "nbd")) {
drv = create_drive_nbd (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "rbd")) {
drv = create_drive_rbd (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "sheepdog")) {
drv = create_drive_sheepdog (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "ssh")) {
drv = create_drive_ssh (g, servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else if (STREQ (protocol, "tftp")) {
drv = create_drive_curl (g, drive_protocol_tftp,
servers, nr_servers, filename,
username, secret,
readonly, format, iface, name,
- disk_label, cachemode);
+ disk_label, cachemode, discard);
}
else {
error (g, _("unknown protocol '%s'"), protocol);
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 545146f..df481a7 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 = 1,
+ discard_besteffort = 2,
+};
+
/* 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, bool
qemu15, bool qemu16);
/* utils.c */
extern int guestfs___validate_guid (const char *);
diff --git a/src/launch-direct.c b/src/launch-direct.c
index 1e84061..584c4de 100644
--- a/src/launch-direct.c
+++ b/src/launch-direct.c
@@ -509,6 +509,29 @@ 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;
+
+ 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, major, minor))
+ 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 +540,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 +1394,90 @@ 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".
+ *
+ * qemu15: qemu >= 1.5. This was the first version that supported
+ * the discard option on -drive at all.
+ *
+ * qemu16: qemu >= 1.6. This was the first version that supported
+ * unmap on qcow2 backing files.
+ */
+bool
+guestfs___discard_possible (guestfs_h *g, struct drive *drv,
+ bool qemu15, bool qemu16)
+{
+ 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 d86e2d2..1c46b57 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 backend_libvir
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);
@@ -1238,7 +1253,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, NULL, NULL,
+ xo, "qcow2",
"unsafe",
+ discard_disable)
== -1)
return -1;
}
@@ -1375,8 +1392,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;
}
@@ -1412,14 +1430,43 @@ 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;
+
+ 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 >= 1005000,
+ data->qemu_version >= 1006000))
+ 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;
@@ -1504,7 +1551,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, NULL, NULL, xo,
+ "qcow2",
"unsafe",
+ discard_disable) == -1)
return -1;
if (construct_libvirt_xml_disk_address (g, xo, params->appliance_index)
@@ -1664,6 +1713,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
Richard W.M. Jones
2014-Mar-10 17:28 UTC
[Libguestfs] [PATCH 3/3] tests: Add a test of discard support, using fstrim.
Test that fstrim actually works, end-to-end.
---
Makefile.am | 1 +
configure.ac | 1 +
tests/discard/Makefile.am | 26 ++++++++++
tests/discard/test-discard.pl | 116 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 144 insertions(+)
create mode 100644 tests/discard/Makefile.am
create mode 100755 tests/discard/test-discard.pl
diff --git a/Makefile.am b/Makefile.am
index 020628e..11b12c2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,6 +46,7 @@ SUBDIRS += tests/events
SUBDIRS += tests/parallel
SUBDIRS += tests/create
SUBDIRS += tests/disks
+SUBDIRS += tests/discard
SUBDIRS += tests/mountable
SUBDIRS += tests/network
SUBDIRS += tests/lvm
diff --git a/configure.ac b/configure.ac
index 35460e2..3785ea8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1673,6 +1673,7 @@ AC_CONFIG_FILES([Makefile
tests/charsets/Makefile
tests/create/Makefile
tests/data/Makefile
+ tests/discard/Makefile
tests/disks/Makefile
tests/disks/test-qemu-drive-libvirt.xml
tests/disk-labels/Makefile
diff --git a/tests/discard/Makefile.am b/tests/discard/Makefile.am
new file mode 100644
index 0000000..f159287
--- /dev/null
+++ b/tests/discard/Makefile.am
@@ -0,0 +1,26 @@
+# libguestfs
+# Copyright (C) 2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+include $(top_srcdir)/subdir-rules.mk
+
+TESTS = \
+ test-discard.pl
+
+TESTS_ENVIRONMENT = $(top_builddir)/run --test
+
+EXTRA_DIST = \
+ $(TESTS)
diff --git a/tests/discard/test-discard.pl b/tests/discard/test-discard.pl
new file mode 100755
index 0000000..770049c
--- /dev/null
+++ b/tests/discard/test-discard.pl
@@ -0,0 +1,116 @@
+#!/usr/bin/perl
+# Copyright (C) 2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that discard support works.
+
+use strict;
+use warnings;
+
+use Sys::Guestfs;
+
+# Since we read error messages, we want to ensure they are printed
+# in English, hence:
+$ENV{"LANG"} = "C";
+
+$| = 1;
+
+my $g = Sys::Guestfs->new ();
+
+# Discard is only supported when using qemu.
+if ($g->get_backend () ne "libvirt" &&
+ $g->get_backend () !~ /^libvirt:/ &&
+ $g->get_backend () ne "direct") {
+ print "$0: skipped test because discard is only supported when using
qemu\n";
+ exit 77;
+}
+
+# You can set this to "raw" or "qcow2".
+my $format = "raw";
+
+my $disk;
+my @args;
+if ($format eq "raw") {
+ $disk = "test-discard.img";
+ @args = ( preallocation => "sparse" );
+} elsif ($format eq "qcow2") {
+ $disk = "test-discard.qcow2";
+ @args = ( preallocation => "off", compat => "1.1"
);
+} else {
+ die "$0: invalid disk format: $format\n";
+}
+
+# Create a disk and add it with discard enabled. This is allowed to
+# fail, eg because qemu is too old, but libguestfs must tell us that
+# it failed (since we're using 'enable', not 'besteffort').
+$g->disk_create ($disk, $format, 100*1024*1024, @args);
+END { unlink ($disk); };
+
+eval {
+ $g->add_drive ($disk, format => $format, readonly => 0, discard
=> "enable");
+ $g->launch ();
+};
+if ($@) {
+ if ($@ =~ /discard cannot be enabled on this drive/) {
+ # This is OK. Libguestfs says it's not possible to enable
+ # discard on this drive (eg. because qemu is too old). Print
+ # the reason and skip the test.
+ print "$0: skipped test: $@\n";
+ exit 77;
+ }
+ die # propagate the unexpected error
+}
+
+# Is fstrim available in the appliance?
+unless ($g->feature_available (["fstrim"])) {
+ print "$0: skipped test because fstrim is not available\n";
+ exit 77;
+}
+
+# At this point we've got a disk which claims to support discard.
+# Let's test that theory.
+
+my $orig_size = (stat ($disk))[12];
+print "original size:\t$orig_size (blocks)\n";
+#system "du -sh $disk";
+
+# Write a filesystem onto the disk and fill it with data.
+
+$g->mkfs ("ext4", "/dev/sda");
+$g->mount_options ("discard", "/dev/sda",
"/");
+$g->fill (33, 10000000, "/data");
+$g->sync ();
+
+my $full_size = (stat ($disk))[12];
+print "full size:\t$full_size (blocks)\n";
+#system "du -sh $disk";
+
+die "surprising result: full size <= original size"
+ if $full_size <= $orig_size;
+
+# Remove the file and then try to trim the filesystem.
+
+$g->rm ("/data");
+$g->fstrim ("/");
+$g->sync ();
+$g->close ();
+
+my $trimmed_size = (stat ($disk))[12];
+print "trimmed size:\t$trimmed_size (blocks)\n";
+#system "du -sh $disk";
+
+die "looks like the fstrim operation did not work"
+ if $trimmed_size >= $full_size;
--
1.8.5.3
Pino Toscano
2014-Mar-10 17:50 UTC
Re: [Libguestfs] [PATCH 2/3] New API parameter: Add discard parameter to guestfs_add_drive_opts.
Hi, On Monday 10 March 2014 17:28:53 Richard W.M. Jones wrote:> index 1e84061..584c4de 100644 > --- a/src/launch-direct.c > +++ b/src/launch-direct.c > @@ -509,6 +509,29 @@ 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; > + > + 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, major, minor))There is an argument mismatch in this call to guestfs___discard_possible (int's vs bool's). What about turn guestfs___discard_possible to just get the qemu version, and do the checks in it rather than asking to prefill the two "is qemu >= 1.5" and "is qemu >= 1.6" bool's? -- Pino Toscano
Pino Toscano
2014-Mar-10 17:59 UTC
Re: [Libguestfs] [PATCH 1/3] launch: libvirt: Move the is_kvm flag (derived from libvirt capabilities) to backend data struct.
Hi, On Monday 10 March 2014 17:28:52 Richard W.M. Jones wrote:> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c > index fd6e6d2..d86e2d2 100644 > --- a/src/launch-libvirt.c > +++ b/src/launch-libvirt.c > @@ -106,6 +106,7 @@ struct backend_libvirt_data { > char *selinux_imagelabel; > bool selinux_norelabel_disks; > char name[DOMAIN_NAME_LEN]; /* random name */ > + bool is_kvm; /* false = qemu, true = kvm (from capabilities)*/ }; > > /* Parameters passed to construct_libvirt_xml and subfunctions. We > @@ -122,12 +123,11 @@ struct libvirt_xml_params { > char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */ > char console_path[UNIX_PATH_MAX]; > bool enable_svirt; /* false if we decided to disable sVirt */ > - bool is_kvm; /* false = qemu, true = kvm */ > bool current_proc_is_root; /* true = euid is root */ > }; > > -static int parse_capabilities (guestfs_h *g, const char *capabilities_xml, struct libvirt_xml_params *params); > -static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct libvirt_xml_params *params); > +static int parse_capabilities (guestfs_h *g, const char *capabilities_xml, struct backend_libvirt_data *data); > +static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct backend_libvirt_data *data, const struct libvirt_xml_params *params);Considering the libvirt_xml_params struct has already a 'struct backend_libvirt_data *data' member, I guess these (and the others) functions could just access params->data->is_kvm with no need to pass also backend_libvirt_data around, no? After all, construct_libvirt_xml_name does this already. -- Pino Toscano