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;

+ + 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)

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 :-(

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.

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.

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);

On Tue, Oct 30, 2018 at 03:07:59PM +0000, Daniel P. Berrangé wrote: >On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote: >> 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? > >Not really. Udev is in use everywhere, so this behaviour makes the >patch useless in practice, even though it is technically right in >theory :-( > Does it? With this behaviour we still do the "expensive" work after any machine has started. But for one machine starting it still has the effect of running it only once. And we *need* to run it for each machine unless we also cache the result per (at least) user:group of that machine as every machine can run under different user:group. I'll go through the patch again (just skimmed it the first time), but I think this actually still makes it better (although not perfect). I wonder why the udev rule does not fire only on change (why doesn't it say ACTION=="change").

>Regards, >Daniel >-- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| >|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >-- >libvir-list mailing list &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Oct 30, 2018 at 03:07:59PM +0000, Daniel P. Berrangé wrote: >On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote: >> 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? > >Not really. Udev is in use everywhere, so this behaviour makes the >patch useless in practice, even though it is technically right in >theory :-( > Does it? With this behaviour we still do the "expensive" work after any machine has started. But for one machine starting it still has the effect of running it only once. And we *need* to run it for each machine unless we also cache the result per (at least) user:group of that machine as every machine can run under different user:group.

I'll go through the patch again (just skimmed it the first time), but I think this actually still makes it better (although not perfect). I wonder why the udev rule does not fire only on change (why doesn't it say ACTION=="change"). >Regards, >Daniel >-- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| >|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >-- >libvir-list mailing list &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list

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. > > eg I vaguely recall a udev rule that resets permissions on device nodes > whenever an FD is closed, which might cause this kind of behaviour After trying hard to find the rule that mangles the ctime I've found the following bug in udev: https://github.com/zippy2/systemd/commit/51915e4e6a64c581da321c25448d8062... I've send that as a pull request to systemd. So after all, ctime might be usable again.

Michal

On Tue, Nov 20, 2018 at 11:00 AM +0100, Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; wrote: > On Thu, Nov 01, 2018 at 04:37 PM +0100, Michal Privoznik <mprivozn(a)redhat.com&gt; 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. >>> >>> eg I vaguely recall a udev rule that resets permissions on device nodes >>> whenever an FD is closed, which might cause this kind of behaviour >> >> After trying hard to find the rule that mangles the ctime I've found the >> following bug in udev: >> >> https://github.com/zippy2/systemd/commit/51915e4e6a64c581da321c25448d8062... >> >> I've send that as a pull request to systemd. So after all, ctime might >> be usable again. > > Are there any other objections against this patch? Because even with the > ctime bug in udev it’s performs _much_ better than before.

Polite ping :) […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294