[libvirt] [PATCH 0/3] add disk driver metadata_cache_size option

[Nikolay Shirokovskiy]

On 07.11.2018 17:42, Peter Krempa wrote:
> On Fri, Nov 02, 2018 at 11:11:50 +0100, Kevin Wolf wrote:
>> Am 01.11.2018 um 12:32 hat Nikolay Shirokovskiy geschrieben:
>>> Hi, all.
>>>
>>> This is a patch series after offlist agreement on introducing
>>> metadata-cache-size option for disks. The options itself is described in 2nd
>>> patch of the series.
>>>
>>> There is a plenty of attempts to add option to set qcow2 metadata cache sizes in
>>> past, see [1] [2] to name recent that has comments I tried to address or has
>>> work I reused to some extent.
>>>
>>> I would like to address Peter's comment in [1] that this option is image's
>>> option rather then disk's here. While this makes sense if we set specific cache
>>> value that covers disk only partially, here when we have maximum policy that
>>> covers whole disk it makes sense to set same value for all images. The setted
>>> value is only upper limit on cache size and thus cache for every image will
>>> grow proportionally to image data size "visible from top" eventually I guess.
>>> And these sizes are changing during guest lifetime - thus we need set whole
>>> disk limit for every image for 'maxium' effect.
>>>
>>> On the other hand if we add policies different from 'maximum' it may require
>>> per image option. Well I can't imagine name for element for backing chain that
>>> will have cache size attribute better then 'driver'). And driver is already
>>> used for top image so I leave it this way.
>>>
>>>   KNOWN ISSUES
>>>
>>> 1. when using -driver to configure disks in command line cache size does not
>>>    get propagated thru backing chain
>>>
>>> 2. when making external disk snapshot cache size sneak into backing file
>>>    filename and thus cache size for backing image became remembered there
>>>
>>> 3. blockcommit after such snapshot will not work because libvirt is not ready
>>>    for backing file name is reported as json sometimes
>>>
>>> Looks like 1 can be addressed in qemu.
>>
>> I don't think we want to add inheritance of driver-specific options.
>> Option inheritance is already a bit messy with options that every driver
>> understands.
>>
>> And -drive is the obsolete interface anyway, when you use -blockdev you
>> specify all nodes explicitly.
> 
> So this would mean we get different behaviour depending on whether
> -blockdev is supported by libvirt and qemu or not.
> 
> This means that there are the following possibilities:
> 
> 1) allow this feature only with -blockdev
> 
> 2) limit this only to the top layer image
> 
> 3) make it configurable per backing chain entry.
> 
> Given our discussion on the KVM forum, I don't think it makes sense to
> do 3, 2 would make it incomplete, so 1 is the only reasonable option if
> qemu will not do the inheritance.
> 
>>> The reason for behaviour in 2 I do not understand honestly.
>>
>> QEMU doesn't recognise that the cache size option doesn't affect which
>> data you're accessing. Max had a series that should probably fix it.
>> Maybe we should finally get it merged.
>>
>> Anyway, I suppose if you use libvirt in -blockdev mode, it doesn't make
>> a difference anyway, because libvirt should then manually call
>> blockdev-create for the overlay and therefore specify the backing file
>> string explicitly.
> 
> Yes, in that case we'll create it manually with the correct backing
> store path. Currently we'd ignore such an entry in the backing store
> path when detecting the chain from the disk so this should not affect
> anything.
> 
>> Can you confirm this in practice?
>>
>>> And 3 will be naturally addessed after blockjobs starts working with
>>> blockdev configuration I guess.
> 
> Did you test this? We do support backing files with 'json:' pseudo
> protocol.

A kind of :)

I can not even create snapshot when using -blockdev
(setting/unsetting metadata_cache_size makes no different)

# cat snap.xml 
<domainsnapshot>
  <disks>
    <disk name="sda" snapshot="external"/>
  </disks>
</domainsnapshot>

# virsh snapshot-create VM snap.xml --disk-only --no-metadata
error: internal error: unable to execute QEMU command 'transaction': Cannot find device=drive-scsi0-0-0-0 nor node_name=

But if I create snapshot with qemu using -drive, then destroy domain,
change qemu binary to the one that supports -blockdev, start domain and blockcommit
I get:

# virsh blockcommit VM sda --pivot
error: internal error: unable to find backing name for device drive-scsi0-0-0-0

Yeah, looks like this does not relate to json pseudo protocol in backing file, sorry.
It is just due to blockjobs are not yet supported for -blockdev as mentioned in [1]

Ahhh, blockcommit failed with message: 

error: internal error: qemu block name 'json:{"driver": "qcow2", "l2-cache-size": "9223372036854775807", "file": {"driver": "file", "filename": "/somepath/harddisk.hdd"}}' doesn't match expected '/somepath/harddisk.hdd'

for versions of libvirt before blockdev patches (libvirt-3.9.0). Sorry again.


So looks like we can merge the series after addressing your comments
and leave only support for -blockdev configurations.

[1] https://www.redhat.com/archives/libvir-list/2018-August/msg00755.html

Nikolay

> 
>>
>> Hopefully (Peter?), but depending on 2. it might not even be necessary
>> if libvirt doesn't explicitly store json: pseudo-filenames.
> 
> Well, we will store them eventually in json pseudo-protocol format since
> in some cases (e.g. multi-host gluster) we don't have any other option.
>