[libvirt] [PATCH] libxl: add discard support to libxl_device_disk
Translate libvirt discard settings into libxl-4.5 discard settings. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- default means leave decision to libxl. Not sure if that is what "default" in libvirt terms really means. src/libxl/libxl_conf.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b7fed7f..4cb062e 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -713,6 +713,33 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config) return -1; } +static void +libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard) +{ + if (!x_disk->readwrite) + return; +#if defined(LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE) + switch (discard) { + case VIR_DOMAIN_DISK_DISCARD_DEFAULT: + break; + case VIR_DOMAIN_DISK_DISCARD_UNMAP: + libxl_defbool_set(&x_disk->discard_enable, true); + break; + default: + case VIR_DOMAIN_DISK_DISCARD_IGNORE: + libxl_defbool_set(&x_disk->discard_enable, false); + break; + } +#else + if (discard == VIR_DOMAIN_DISK_DISCARD_DEFAULT) + return; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("This version of libxenlight does not support " + "discard= option passing")); +#endif +} + + int libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) { @@ -827,6 +854,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) x_disk->removable = 1; x_disk->readwrite = !l_disk->readonly; x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0; + libxlDiskSetDiscard(x_disk, l_disk->discard); /* An empty CDROM must have the empty format, otherwise libxl fails. */ if (x_disk->is_cdrom && !x_disk->pdev_path) x_disk->format = LIBXL_DISK_FORMAT_EMPTY;
Olaf Hering wrote:
Translate libvirt discard settings into libxl-4.5 discard settings.
Signed-off-by: Olaf Hering <olaf@aepfle.de> ---
default means leave decision to libxl. Not sure if that is what "default" in libvirt terms really means.
It generally means hypervisor default, so deferring to libxl is the right thing to do.
src/libxl/libxl_conf.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b7fed7f..4cb062e 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -713,6 +713,33 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config) return -1; }
+static void +libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard) +{ + if (!x_disk->readwrite) + return; +#if defined(LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE) + switch (discard) { + case VIR_DOMAIN_DISK_DISCARD_DEFAULT:
case VIR_DOMAIN_DISK_DISCARD_LAST:
+ break; + case VIR_DOMAIN_DISK_DISCARD_UNMAP: + libxl_defbool_set(&x_disk->discard_enable, true); + break; + default:
Then you can remove 'default'.
+ case VIR_DOMAIN_DISK_DISCARD_IGNORE: + libxl_defbool_set(&x_disk->discard_enable, false); + break; + } +#else + if (discard == VIR_DOMAIN_DISK_DISCARD_DEFAULT) + return; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("This version of libxenlight does not support " + "discard= option passing"));
An error would be reported here, but the overall libxlMakeDisk operation would succeed right? Shouldn't it fail if the user requests discard but it is not supported? Regards, Jim
+#endif +} + + int libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) { @@ -827,6 +854,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) x_disk->removable = 1; x_disk->readwrite = !l_disk->readonly; x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0; + libxlDiskSetDiscard(x_disk, l_disk->discard); /* An empty CDROM must have the empty format, otherwise libxl fails. */ if (x_disk->is_cdrom && !x_disk->pdev_path) x_disk->format = LIBXL_DISK_FORMAT_EMPTY;
On 05/30/2014 03:07 PM, Jim Fehlig wrote:
Olaf Hering wrote:
Translate libvirt discard settings into libxl-4.5 discard settings.
Signed-off-by: Olaf Hering <olaf@aepfle.de> ---
+static void +libxlDiskSetDiscard(libxl_device_disk *x_disk, int discard) +{ + if (!x_disk->readwrite) + return; +#if defined(LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE) + switch (discard) {
This line needs a cast to the enum type, if you want the compiler to enforce that you covered all enum values.
+ case VIR_DOMAIN_DISK_DISCARD_DEFAULT:
case VIR_DOMAIN_DISK_DISCARD_LAST:
+ break; + case VIR_DOMAIN_DISK_DISCARD_UNMAP: + libxl_defbool_set(&x_disk->discard_enable, true); + break; + default:
Then you can remove 'default'.
Removing 'default' is also essential for the compiler-enforced full enum coverage trick in a switch statement. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 30, Jim Fehlig wrote:
Olaf Hering wrote:
+ if (discard == VIR_DOMAIN_DISK_DISCARD_DEFAULT) + return; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("This version of libxenlight does not support " + "discard= option passing")); An error would be reported here, but the overall libxlMakeDisk operation would succeed right? Shouldn't it fail if the user requests discard but it is not supported?
How would libvirt know if the underlying backing store supports discard? It would need to maintain a list of supported targets. If it remains just some sort of warning a given .xml file can be shared among different libvirt versions. After all, discard is not operation critical. Olaf
On Mon, Jul 07, 2014 at 04:00:44PM +0200, Olaf Hering wrote:
On Fri, May 30, Jim Fehlig wrote:
Olaf Hering wrote:
+ if (discard == VIR_DOMAIN_DISK_DISCARD_DEFAULT) + return; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("This version of libxenlight does not support " + "discard= option passing")); An error would be reported here, but the overall libxlMakeDisk operation would succeed right? Shouldn't it fail if the user requests discard but it is not supported?
How would libvirt know if the underlying backing store supports discard? It would need to maintain a list of supported targets. If it remains just some sort of warning a given .xml file can be shared among different libvirt versions. After all, discard is not operation critical.
IIUC, this is not about the underlying storage, but about whether the hypervisor API supports the discard setting. The libvirt policy is to always raise VIR_ERR_CONFIG_UNSUPPORTED and return failure to the app if they request a setting that the hypervisor does not support. Whether discard is critical or not really depends on your POV, so it is better not to make such assumptions on behalf of the mgmt app. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Jul 07, Daniel P. Berrange wrote:
IIUC, this is not about the underlying storage, but about whether the hypervisor API supports the discard setting. The libvirt policy is to always raise VIR_ERR_CONFIG_UNSUPPORTED and return failure to the app if they request a setting that the hypervisor does not support. Whether discard is critical or not really depends on your POV, so it is better not to make such assumptions on behalf of the mgmt app.
So how should the code path error out in this case? Olaf
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jim Fehlig -
Olaf Hering