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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list