[libvirt] [PATCH 0/4] Introduce support for FITRIM within guest OS
https://bugzilla.redhat.com/show_bug.cgi?id=831159 Michal Privoznik (4): Introduce virDomainFSTrim() public API remote: Implement virDomainFSTrim qemu: Implement virDomainFSTrim virsh: Expose new virDomainFSTrim API include/libvirt/libvirt.h.in | 4 ++ src/driver.h | 6 +++ src/libvirt.c | 50 +++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/qemu/qemu_agent.c | 25 +++++++++++++ src/qemu/qemu_agent.h | 2 + src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 10 +++++- src/remote_protocol-structs | 7 ++++ src/rpc/gendispatch.pl | 1 + tools/virsh-domain.c | 40 ++++++++++++++++++++ tools/virsh.pod | 12 ++++++ 13 files changed, 245 insertions(+), 1 deletions(-) -- 1.7.8.6
This will call FITRIM within guest. The API has 4 arguments, however, only 2 will be used for now (@dom and @minumum). The rest two are there if in future qemu guest agent learns them. --- include/libvirt/libvirt.h.in | 4 +++ src/driver.h | 6 +++++ src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 65 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 49a361a..b161bce 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4438,6 +4438,10 @@ int virDomainOpenGraphics(virDomainPtr dom, int virDomainInjectNMI(virDomainPtr domain, unsigned int flags); +int virDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags); /** * virSchedParameterType: diff --git a/src/driver.h b/src/driver.h index 7ba66ad..5163840 100644 --- a/src/driver.h +++ b/src/driver.h @@ -903,6 +903,11 @@ typedef int unsigned char **cpumap, unsigned int *online, unsigned int flags); +typedef int + (*virDrvDomainFSTrim)(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags); /** * _virDriver: @@ -1094,6 +1099,7 @@ struct _virDriver { virDrvNodeGetMemoryParameters nodeGetMemoryParameters; virDrvNodeSetMemoryParameters nodeSetMemoryParameters; virDrvNodeGetCPUMap nodeGetCPUMap; + virDrvDomainFSTrim domainFSTrim; }; typedef int diff --git a/src/libvirt.c b/src/libvirt.c index bdb1dc6..1c6863f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20164,3 +20164,53 @@ error: virDispatchError(conn); return -1; } + +/** + * virDomainFSTrim: + * @dom: a domain object + * @mountPoint: which mount point trim + * @minimum: Minimum contiguous free range to discard in bytes + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * Calls FITRIM within the guest (hence guest agent may be + * required depending on hypervisor used). Either call it on each + * mounted filesystem (@mountPoint is NULL) or just on specified + * @mountPoint. @minimum tell that free ranges smaller than this + * may be ignored (this is a hint and the guest may not respect + * it). By increasing this value, the fstrim operation will + * complete more quickly for filesystems with badly fragmented + * free space, although not all blocks will be discarded. + * + * Returns 0 on success, -1 otherwise. + */ +int +virDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags) +{ + VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x", + mountPoint, minimum, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (dom->conn->driver->domainFSTrim) { + int ret = dom->conn->driver->domainFSTrim(dom, mountPoint, + minimum, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index f494821..f699e93 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -574,4 +574,9 @@ LIBVIRT_1.0.0 { virNodeGetCPUMap; } LIBVIRT_0.10.2; +LIBVIRT_1.0.1 { + global: + virDomainFSTrim; +} LIBVIRT_1.0.0; + # .... define new API here using predicted next version number .... -- 1.7.8.6
On Tue, Nov 20, 2012 at 07:47:35PM +0100, Michal Privoznik wrote:
This will call FITRIM within guest. The API has 4 arguments, however, only 2 will be used for now (@dom and @minumum). The rest two are there if in future qemu guest agent learns them. --- include/libvirt/libvirt.h.in | 4 +++ src/driver.h | 6 +++++ src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 65 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 49a361a..b161bce 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4438,6 +4438,10 @@ int virDomainOpenGraphics(virDomainPtr dom,
int virDomainInjectNMI(virDomainPtr domain, unsigned int flags);
+int virDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags);
/** * virSchedParameterType: diff --git a/src/driver.h b/src/driver.h index 7ba66ad..5163840 100644 --- a/src/driver.h +++ b/src/driver.h @@ -903,6 +903,11 @@ typedef int unsigned char **cpumap, unsigned int *online, unsigned int flags); +typedef int + (*virDrvDomainFSTrim)(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags);
/** * _virDriver: @@ -1094,6 +1099,7 @@ struct _virDriver { virDrvNodeGetMemoryParameters nodeGetMemoryParameters; virDrvNodeSetMemoryParameters nodeSetMemoryParameters; virDrvNodeGetCPUMap nodeGetCPUMap; + virDrvDomainFSTrim domainFSTrim; };
typedef int diff --git a/src/libvirt.c b/src/libvirt.c index bdb1dc6..1c6863f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -20164,3 +20164,53 @@ error: virDispatchError(conn); return -1; } + +/** + * virDomainFSTrim: + * @dom: a domain object + * @mountPoint: which mount point trim + * @minimum: Minimum contiguous free range to discard in bytes + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * Calls FITRIM within the guest (hence guest agent may be + * required depending on hypervisor used). Either call it on each + * mounted filesystem (@mountPoint is NULL) or just on specified + * @mountPoint. @minimum tell that free ranges smaller than this + * may be ignored (this is a hint and the guest may not respect + * it). By increasing this value, the fstrim operation will + * complete more quickly for filesystems with badly fragmented + * free space, although not all blocks will be discarded. + * + * Returns 0 on success, -1 otherwise. + */ +int +virDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags) +{ + VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x", + mountPoint, minimum, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + }
You're missing the check for read-only connections
+ + if (dom->conn->driver->domainFSTrim) { + int ret = dom->conn->driver->domainFSTrim(dom, mountPoint, + minimum, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +}
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 11/20/12 19:47, Michal Privoznik wrote:
This will call FITRIM within guest. The API has 4 arguments, however, only 2 will be used for now (@dom and @minumum). The rest two are there if in future qemu guest agent learns them. --- include/libvirt/libvirt.h.in | 4 +++ src/driver.h | 6 +++++ src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 65 insertions(+), 0 deletions(-)
...
+ +/** + * virDomainFSTrim: + * @dom: a domain object + * @mountPoint: which mount point trim + * @minimum: Minimum contiguous free range to discard in bytes + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * Calls FITRIM within the guest (hence guest agent may be + * required depending on hypervisor used). Either call it on each + * mounted filesystem (@mountPoint is NULL) or just on specified + * @mountPoint. @minimum tell that free ranges smaller than this + * may be ignored (this is a hint and the guest may not respect + * it). By increasing this value, the fstrim operation will + * complete more quickly for filesystems with badly fragmented + * free space, although not all blocks will be discarded. + * + * Returns 0 on success, -1 otherwise. + */ +int +virDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags) +{ + VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x", + mountPoint, minimum, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN(dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + }
as Dan pointed out. The check for read-only connections is missing.
+ + if (dom->conn->driver->domainFSTrim) { + int ret = dom->conn->driver->domainFSTrim(dom, mountPoint, + minimum, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1;
otherwise ACK. Peter
A new rule to fixup_name() in gendispatch.pl needs to be added, otherwise we are left with remoteDomainFstrim which is not wanted. --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 10 +++++++++- src/remote_protocol-structs | 7 +++++++ src/rpc/gendispatch.pl | 1 + 4 files changed, 18 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62b7729..a8c6993 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6143,6 +6143,7 @@ static virDriver remote_driver = { .nodeSetMemoryParameters = remoteNodeSetMemoryParameters, /* 0.10.2 */ .nodeGetMemoryParameters = remoteNodeGetMemoryParameters, /* 0.10.2 */ .nodeGetCPUMap = remoteNodeGetCPUMap, /* 1.0.0 */ + .domainFSTrim = remoteDomainFSTrim, /* 1.0.1 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d6ac3c1..31567e2 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2682,6 +2682,13 @@ struct remote_node_get_cpu_map_ret { int ret; }; +struct remote_domain_fstrim_args { + remote_nonnull_domain dom; + remote_string mountPoint; + unsigned hyper minimum; + unsigned int flags; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -3026,7 +3033,8 @@ enum remote_procedure { REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, /* autogen autogen */ - REMOTE_PROC_NODE_GET_CPU_MAP = 293 /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_CPU_MAP = 293, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_FSTRIM = 294 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 6fe7213..d0d4f53 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2139,6 +2139,12 @@ struct remote_node_get_cpu_map_ret { u_int online; int ret; }; +struct remote_domain_fstrim_args { + remote_nonnull_domain dom; + remote_string mountPoint; + uint64_t minimum; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2433,4 +2439,5 @@ enum remote_procedure { REMOTE_PROC_NETWORK_UPDATE = 291, REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, REMOTE_PROC_NODE_GET_CPU_MAP = 293, + REMOTE_PROC_DOMAIN_FSTRIM = 294, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ae7ecba..899f4bc 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -44,6 +44,7 @@ sub fixup_name { $name =~ s/Os$/OS/; $name =~ s/Nmi$/NMI/; $name =~ s/Pm/PM/; + $name =~ s/Fstrim$/FSTrim/; return $name; } -- 1.7.8.6
On 11/20/12 19:47, Michal Privoznik wrote:
A new rule to fixup_name() in gendispatch.pl needs to be added, otherwise we are left with remoteDomainFstrim which is not wanted. --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 10 +++++++++- src/remote_protocol-structs | 7 +++++++ src/rpc/gendispatch.pl | 1 + 4 files changed, 18 insertions(+), 1 deletions(-)
Straightforward. ACK Peter
using qemu guest agent. As said in previous patch, @mountPoint must be NULL and @flags zero because qemu guest agent doesn't support these arguments yet. If qemu learns them, we can start supporting them as well. --- src/qemu/qemu_agent.c | 25 ++++++++++++++ src/qemu/qemu_agent.h | 2 + src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index ab6dc22..6f517c9 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1448,3 +1448,28 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + +int +qemuAgentFSTrim(qemuAgentPtr mon, + unsigned long long minimum) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuAgentMakeCommand("guest-fstrim", + "U:minimum", minimum, + NULL); + if (!cmd) + return ret; + + ret = qemuAgentCommand(mon, cmd, &reply, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); + + if (reply && ret == 0) + ret = qemuAgentCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index c62c438..dad068b 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -83,4 +83,6 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon, const char *cmd, char **result, int timeout); +int qemuAgentFSTrim(qemuAgentPtr mon, + unsigned long long minimum); #endif /* __QEMU_AGENT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 595c452..3dc5bdd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14733,6 +14733,88 @@ cleanup: return result; } +static int +qemuDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + qemuDomainObjPrivatePtr priv; + + virCheckFlags(0, -1); + + if (mountPoint) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Specifying mount point " + "is not supported for now")); + return -1; + } + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (priv->agentError) { + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", + _("QEMU guest agent is not " + "available due to an error")); + goto cleanup; + } + + if (!priv->agent) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + qemuDomainObjEnterAgent(driver, vm); + ret = qemuAgentFSTrim(priv->agent, minimum); + qemuDomainObjExitAgent(driver, vm); + if (ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to execute agent command")); + goto endjob; + } + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) { + vm = NULL; + } + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -14906,6 +14988,7 @@ static virDriver qemuDriver = { .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */ }; -- 1.7.8.6
On 11/20/12 19:47, Michal Privoznik wrote:
using qemu guest agent. As said in previous patch, @mountPoint must be NULL and @flags zero because qemu guest agent doesn't support these arguments yet. If qemu learns them, we can start supporting them as well. --- src/qemu/qemu_agent.c | 25 ++++++++++++++ src/qemu/qemu_agent.h | 2 + src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index ab6dc22..6f517c9 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1448,3 +1448,28 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + +int +qemuAgentFSTrim(qemuAgentPtr mon, + unsigned long long minimum) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuAgentMakeCommand("guest-fstrim", + "U:minimum", minimum, + NULL); + if (!cmd) + return ret; + + ret = qemuAgentCommand(mon, cmd, &reply, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
I didn't check the full call graph of qemuAgentCommand, but most of the paths already contain error reporting ... (relevance of this command will be revealed later :))
+ + if (reply && ret == 0) + ret = qemuAgentCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index c62c438..dad068b 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -83,4 +83,6 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon, const char *cmd, char **result, int timeout); +int qemuAgentFSTrim(qemuAgentPtr mon, + unsigned long long minimum); #endif /* __QEMU_AGENT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 595c452..3dc5bdd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14733,6 +14733,88 @@ cleanup: return result; }
+static int +qemuDomainFSTrim(virDomainPtr dom, + const char *mountPoint, + unsigned long long minimum, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + qemuDomainObjPrivatePtr priv; + + virCheckFlags(0, -1); + + if (mountPoint) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Specifying mount point " + "is not supported for now")); + return -1; + } + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + }
You can avoid the two blocks of code above using the qemuDomObjFromDomain() helper that does that for you.
+ + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (priv->agentError) { + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", + _("QEMU guest agent is not " + "available due to an error")); + goto cleanup; + } + + if (!priv->agent) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + goto cleanup; + }
I'd swap the two blocks above, it makes more sense to check if there's an agent error after you check for the agent.
+ + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + qemuDomainObjEnterAgent(driver, vm); + ret = qemuAgentFSTrim(priv->agent, minimum); + qemuDomainObjExitAgent(driver, vm); + if (ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to execute agent command")); + goto endjob;
(the relevant part from the comment up) ... this masks the error from the underlying helper function. The goto statement is redundant here.
+ } + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) {
block quotes are not needed here
+ vm = NULL; + } + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -14906,6 +14988,7 @@ static virDriver qemuDriver = { .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */ + .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */ };
I don't like masking errors from underlying layers. I'd rather have it changed so that more specific errors are shown but it's not a hard requirement if you think it's not needed.
under domfstrim command. Since API doesn't support all parameters (some must be NULL or zero), don't expose these in virsh neither. --- tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 ++++++++++++ 2 files changed, 52 insertions(+), 0 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..4ea08f4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8300,6 +8300,45 @@ cleanup: return ret; } +static const vshCmdInfo info_domfstrim[] = { + {"help", N_("Invoke fstrim on domain's mounted filesystems.")}, + {"desc", N_("Invoke fstrim on domain's mounted filesystems.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_domfstrim[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous " + "free ranges smaller than this (Bytes)")}, + {NULL, 0, 0, NULL} +}; +static bool +cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + bool ret = false; + unsigned long long minimum = 0; + unsigned int flags = 0; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { + vshError(ctl, _("Unable to parse integer parameter")); + goto cleanup; + } + + if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) { + vshError(ctl, _("Unable to invoke fstrim")); + goto cleanup; + } + + ret = true; + +cleanup: + return ret; +} + const vshCmdDef domManagementCmds[] = { {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device, 0}, @@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = { {"detach-interface", cmdDetachInterface, opts_detach_interface, info_detach_interface, 0}, {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0}, + {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0}, {"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0}, {"domid", cmdDomid, opts_domid, info_domid, 0}, {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink, info_domif_setlink, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 29be39e..c3fdcb7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display of the domain via VNC, SPICE or RDP. If I<--include-password> is specified, the SPICE channel password will be included in the URI. +=item B<domfstrim> I<domain> [I<--minimum> B<bytes>] + +Issue a fstrim command on all mounted filesystems within a running +domain. It discards blocks which are not in use by the filesystem. +If I<--minimum> B<bytes> is specified, it tells guest kernel length +of contiguous free range. Smaller than this may be ignored (this is +a hint and the guest may not respect it). By increasing this value, +the fstrim operation will complete more quickly for filesystems +with badly fragmented free space, although not all blocks will +be discarded. The default value is zero, meaning "discard +every free block". + =item B<domhostname> I<domain> Returns the hostname of a domain, if the hypervisor makes it available. -- 1.7.8.6
On 11/20/12 19:47, Michal Privoznik wrote:
under domfstrim command. Since API doesn't support all parameters (some must be NULL or zero), don't expose these in virsh neither. --- tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 ++++++++++++ 2 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..4ea08f4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8300,6 +8300,45 @@ cleanup: return ret; }
+static const vshCmdInfo info_domfstrim[] = { + {"help", N_("Invoke fstrim on domain's mounted filesystems.")}, + {"desc", N_("Invoke fstrim on domain's mounted filesystems.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_domfstrim[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous " + "free ranges smaller than this (Bytes)")},
Wouldn't it make sense to expose the mount point argument right now? I know it's not implemented yet, but when somebody does implement that we will need to change this later.
+ {NULL, 0, 0, NULL} +}; +static bool +cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + bool ret = false; + unsigned long long minimum = 0; + unsigned int flags = 0; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { + vshError(ctl, _("Unable to parse integer parameter"));
As you know what's the name of the parameter print it in the error message so the user is not left guessing where the error is.
+ goto cleanup; + } + + if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) { + vshError(ctl, _("Unable to invoke fstrim")); + goto cleanup; + } + + ret = true; + +cleanup: + return ret; +} + const vshCmdDef domManagementCmds[] = { {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device, 0}, @@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = { {"detach-interface", cmdDetachInterface, opts_detach_interface, info_detach_interface, 0}, {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0}, + {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0}, {"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0}, {"domid", cmdDomid, opts_domid, info_domid, 0}, {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink, info_domif_setlink, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 29be39e..c3fdcb7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display of the domain via VNC, SPICE or RDP. If I<--include-password> is specified, the SPICE channel password will be included in the URI.
+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>] + +Issue a fstrim command on all mounted filesystems within a running +domain. It discards blocks which are not in use by the filesystem. +If I<--minimum> B<bytes> is specified, it tells guest kernel length +of contiguous free range. Smaller than this may be ignored (this is +a hint and the guest may not respect it). By increasing this value, +the fstrim operation will complete more quickly for filesystems +with badly fragmented free space, although not all blocks will +be discarded. The default value is zero, meaning "discard +every free block". + =item B<domhostname> I<domain>
Returns the hostname of a domain, if the hypervisor makes it available.
I'd rather see this with the ability to specify mountpoint to the api even if it still isn't supported. Peter
On Wed, Nov 28, 2012 at 11:56:23AM +0100, Peter Krempa wrote:
On 11/20/12 19:47, Michal Privoznik wrote:
under domfstrim command. Since API doesn't support all parameters (some must be NULL or zero), don't expose these in virsh neither. --- tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 ++++++++++++ 2 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..4ea08f4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8300,6 +8300,45 @@ cleanup: return ret; }
+static const vshCmdInfo info_domfstrim[] = { + {"help", N_("Invoke fstrim on domain's mounted filesystems.")}, + {"desc", N_("Invoke fstrim on domain's mounted filesystems.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_domfstrim[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous " + "free ranges smaller than this (Bytes)")},
Wouldn't it make sense to expose the mount point argument right now? I know it's not implemented yet, but when somebody does implement that we will need to change this later.
+ {NULL, 0, 0, NULL} +}; +static bool +cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + bool ret = false; + unsigned long long minimum = 0; + unsigned int flags = 0; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { + vshError(ctl, _("Unable to parse integer parameter"));
As you know what's the name of the parameter print it in the error message so the user is not left guessing where the error is.
+ goto cleanup; + } + + if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) { + vshError(ctl, _("Unable to invoke fstrim")); + goto cleanup; + } + + ret = true; + +cleanup: + return ret; +} + const vshCmdDef domManagementCmds[] = { {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device, 0}, @@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = { {"detach-interface", cmdDetachInterface, opts_detach_interface, info_detach_interface, 0}, {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0}, + {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0}, {"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0}, {"domid", cmdDomid, opts_domid, info_domid, 0}, {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink, info_domif_setlink, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 29be39e..c3fdcb7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display of the domain via VNC, SPICE or RDP. If I<--include-password> is specified, the SPICE channel password will be included in the URI.
+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>] + +Issue a fstrim command on all mounted filesystems within a running +domain. It discards blocks which are not in use by the filesystem. +If I<--minimum> B<bytes> is specified, it tells guest kernel length +of contiguous free range. Smaller than this may be ignored (this is +a hint and the guest may not respect it). By increasing this value, +the fstrim operation will complete more quickly for filesystems +with badly fragmented free space, although not all blocks will +be discarded. The default value is zero, meaning "discard +every free block". + =item B<domhostname> I<domain>
Returns the hostname of a domain, if the hypervisor makes it available.
I'd rather see this with the ability to specify mountpoint to the api even if it still isn't supported.
Agreed, we should not cripple virsh to suit a particular libvirtd feature set, because you have to bear in mind that we have an RPC system that allows an old virsh to talk to a new libvirtd. 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 :|
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa