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

Nikolay Shirokovskiy posted 3 patches 3 years, 6 months ago
Failed in applying to current master (apply log)
docs/formatdomain.html.in                          |  7 ++++
docs/schemas/domaincommon.rng                      | 10 +++++
src/conf/domain_conf.c                             | 17 ++++++++
src/conf/domain_conf.h                             |  9 ++++
src/qemu/qemu_block.c                              |  5 ++-
src/qemu/qemu_capabilities.c                       |  3 ++
src/qemu/qemu_capabilities.h                       |  1 +
src/qemu/qemu_command.c                            | 26 ++++++++++++
src/qemu/qemu_domain.c                             |  2 +
src/util/virstoragefile.c                          |  1 +
src/util/virstoragefile.h                          |  1 +
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  1 +
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 +
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml  |  1 +
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1 +
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  1 +
tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml    |  1 +
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml    |  1 +
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml    |  1 +
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 +
tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml    |  1 +
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml    |  1 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml    |  1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml  |  1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml  |  1 +
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml    |  1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |  1 +
.../qemuxml2argvdata/disk-metadata_cache_size.args | 34 +++++++++++++++
.../qemuxml2argvdata/disk-metadata_cache_size.xml  | 42 +++++++++++++++++++
tests/qemuxml2argvtest.c                           |  2 +
.../disk-metadata_cache_size.xml                   | 48 ++++++++++++++++++++++
tests/qemuxml2xmltest.c                            |  2 +
42 files changed, 235 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.args
create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.xml
create mode 100644 tests/qemuxml2xmloutdata/disk-metadata_cache_size.xml
[libvirt] [PATCH 0/3] add disk driver metadata_cache_size option
Posted by Nikolay Shirokovskiy 3 years, 6 months ago
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. The reason for behaviour in 2
I do not understand honestly. And 3 will be naturally addessed after
blockjobs starts working with blockdev configuration I guess.

  LINKS

[1] [PATCH] qemu: Added support L2 table cache for qcow2 disk
https://www.redhat.com/archives/libvir-list/2018-July/msg00166.html

[2] [PATCH v6 0/2] Add support for qcow2 cache
 https://www.redhat.com/archives/libvir-list/2017-September/msg00553.html

Nikolay Shirokovskiy (3):
  qemu: caps: add drive.qcow2.l2-cache-size
  xml: add disk driver metadata_cache_size option
  qemu: support metadata-cache-size for blockdev

 docs/formatdomain.html.in                          |  7 ++++
 docs/schemas/domaincommon.rng                      | 10 +++++
 src/conf/domain_conf.c                             | 17 ++++++++
 src/conf/domain_conf.h                             |  9 ++++
 src/qemu/qemu_block.c                              |  5 ++-
 src/qemu/qemu_capabilities.c                       |  3 ++
 src/qemu/qemu_capabilities.h                       |  1 +
 src/qemu/qemu_command.c                            | 26 ++++++++++++
 src/qemu/qemu_domain.c                             |  2 +
 src/util/virstoragefile.c                          |  1 +
 src/util/virstoragefile.h                          |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  1 +
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml    |  1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml    |  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml    |  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml    |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml    |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml    |  1 +
 tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml  |  1 +
 tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml    |  1 +
 tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |  1 +
 .../qemuxml2argvdata/disk-metadata_cache_size.args | 34 +++++++++++++++
 .../qemuxml2argvdata/disk-metadata_cache_size.xml  | 42 +++++++++++++++++++
 tests/qemuxml2argvtest.c                           |  2 +
 .../disk-metadata_cache_size.xml                   | 48 ++++++++++++++++++++++
 tests/qemuxml2xmltest.c                            |  2 +
 42 files changed, 235 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.args
 create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.xml
 create mode 100644 tests/qemuxml2xmloutdata/disk-metadata_cache_size.xml

-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] add disk driver metadata_cache_size option
Posted by Kevin Wolf 3 years, 6 months ago
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.

> 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.

Can you confirm this in practice?

> And 3 will be naturally addessed after blockjobs starts working with
> blockdev configuration I guess.

Hopefully (Peter?), but depending on 2. it might not even be necessary
if libvirt doesn't explicitly store json: pseudo-filenames.

Kevin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] add disk driver metadata_cache_size option
Posted by Nikolay Shirokovskiy 3 years, 6 months ago

On 02.11.2018 13:11, 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.
> 
>> 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.
> 
> Can you confirm this in practice?

Yes, there is no problem when using -blockdev. Cache size will be set
explicitly for every qcow2 format node. The issue is only with -drive,
when you for example start VM with 'maximum' policy, make a snaphot,
destoy VM, change policy to default and then start VM again. It is
supposed that as cache size is not specified then it is set to qemu's
default value but it is not for backing file because cache size is
recorded in filename.

So if we due to inheritance issue we won't use this policy until
libvirt switch to blockdev then this point does not matter.

Nikolay

> 
>> And 3 will be naturally addessed after blockjobs starts working with
>> blockdev configuration I guess.
> 
> Hopefully (Peter?), but depending on 2. it might not even be necessary
> if libvirt doesn't explicitly store json: pseudo-filenames.
> 
> Kevin
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list