On 10/29/18 6:34 PM, Marc Hartmayer wrote:
> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
> group. This reduces the overhead of the QEMU capabilities cache
> lookup. Before this patch there were many fork() calls used for
> checking whether /dev/kvm is accessible. Now we store the result
> whether /dev/kvm is accessible or not and we only need to re-run the
> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
>
> Suggested-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 54 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e228f52ec0bb..85516954149b 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
> virArch hostArch;
> unsigned int microcodeVersion;
> char *kernelVersion;
> +
> + /* cache whether /dev/kvm is usable as runUid:runGuid */
> + virTristateBool kvmUsable;
> + time_t kvmCtime;
> };
> typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
> typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
> @@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
> }
>
>
> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
> +static bool
> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
> +{
> + struct stat sb;
> + static const char *kvm_device = "/dev/kvm";
> + virTristateBool value;
> + virTristateBool cached_value = priv->kvmUsable;
> + time_t kvm_ctime;
> + time_t cached_kvm_ctime = priv->kvmCtime;
> +
> + if (stat(kvm_device, &sb) < 0) {
I'd report error here iff errno != ENOENT.
> + virReportSystemError(errno,
> + _("Failed to stat %s"), kvm_device);
> + return false;
> + }
> + kvm_ctime = sb.st_ctime;
> +
> + if (kvm_ctime != cached_kvm_ctime) {
> + VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
> + (long long)kvm_ctime, (long long)cached_kvm_ctime);
> + cached_value = VIR_TRISTATE_BOOL_ABSENT;
> + }
> +
> + if (cached_value != VIR_TRISTATE_BOOL_ABSENT)
> + return cached_value == VIR_TRISTATE_BOOL_YES;
> +
> + if (virFileAccessibleAs(kvm_device, R_OK | W_OK,
> + priv->runUid, priv->runGid) == 0) {
> + value = VIR_TRISTATE_BOOL_YES;
> + } else {
> + value = VIR_TRISTATE_BOOL_NO;
> + }
> +
> + /* There is a race window between 'stat' and
> + * 'virFileAccessibleAs'. However, since we're only interested in
> + * detecting changes *after* the virFileAccessibleAs check, we can
> + * neglect this here.
> + */
> + priv->kvmCtime = kvm_ctime;
> + priv->kvmUsable = value;
> +
> + return value == VIR_TRISTATE_BOOL_YES;
> +}
> +
> +
> static bool
> virQEMUCapsIsValid(void *data,
> void *privData)
> @@ -3872,8 +3922,7 @@ virQEMUCapsIsValid(void *data,
> return true;
> }
>
> - kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
> - priv->runUid, priv->runGid) == 0;
> + kvmUsable = virQEMUCapsKVMUsable(priv);
>
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> kvmUsable) {
> @@ -4684,6 +4733,7 @@ virQEMUCapsCacheNew(const char *libDir,
> priv->runUid = runUid;
> priv->runGid = runGid;
> priv->microcodeVersion = microcodeVersion;
> + priv->kvmUsable = VIR_TRISTATE_BOOL_ABSENT;
>
> if (uname(&uts) == 0 &&
> virAsprintf(&priv->kernelVersion, "%s %s", uts.release, uts.version) < 0)
>
Otherwise looking good. I agree with Martin that this might not be the
best solution but it's the best we have (also, the problem is not on our
side).
ACK (will wait a day or two for others to chime in before pushing)
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
