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

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.