Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible

On 10/30/2018 02:46 PM, Michal Privoznik wrote: > On 10/30/2018 01:55 PM, Daniel P. Berrangé wrote: >> On Tue, Oct 30, 2018 at 10:32:08AM +0000, Daniel P. Berrangé wrote: >>> On Tue, Oct 30, 2018 at 11:08:45AM +0100, Michal Privoznik wrote: >>>> On 10/30/2018 10:35 AM, Daniel P. Berrangé wrote: >>>>> On Tue, Oct 30, 2018 at 09:13:50AM +0100, Michal Privoznik wrote: >>>>>> On 10/29/2018 06: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(a)redhat.com&gt; >>>>>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; >>>>>>> --- >>>>>>> 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) { >>>>>>> + virReportSystemError(errno, >>>>>>> + _("Failed to stat %s"), kvm_device); >>>>>>> + return false; >>>>>>> + } >>>>>>> + kvm_ctime = sb.st_ctime; >>>>>> >>>>>> This doesn't feel right. /dev/kvm ctime is changed every time qemu is >>>>>> started or powered off (try running stat over it before and after a >>>>>> domain is started/shut off). So effectively we will fork more often than >>>>>> we would think. Should we cache inode number instead? Because for all >>>>>> that we care is simply if the file is there. >>>>> >>>>> Urgh, that is a bit strange and not the usual semantics for timestamps :-( >>>> >>>> Indeed. >>>> >>>>> >>>>> We can't stat the inode - the code was explicitly trying to cope with the >>>>> way /dev/kvm can change permissions when udev rules get applied. We would >>>>> have to compare the user, group, permissions mask and even ACL, or a hash >>>>> of those. >>>> >>>> Well, we can use ctime as suggested and post a patch for kernel to fix >>>> ctime behaviour. Until the patch is merged our behaviour would be >>>> suboptimal, but still better than it is now. >>> >>> I guess lets talk to KVM team for their input on this and then decide >>> what todo. >> >> Hmm, I wonder if it is not actually a kernel problem, but rather something >> in userspace genuinely touching the device in a way that caues these >> timestamps to be updated. >> > > It is kernel problem. In my testing, the moment I call: > > ioctl(kvm, KVM_CREATE_VM, 0); Okay, I have to retract this claim. 'udevadm monitor' shows some events: KERNEL[3631.129645] change /devices/virtual/misc/kvm (misc) UDEV [3631.130816] change /devices/virtual/misc/kvm (misc) and stopping udevd leaves all three times untouched. So it is udev after all. I just don't know how to find the rule that is causing the issue. Anyway, as for this patch, I think we can merge it in the end, can't we?