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

[Michal Privoznik]

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 at redhat.com>
>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.ibm.com>
>>> ---
>>>  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.

Michal