[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/3] xml: add disk driver metadata_cache_size option



On Thu, Nov 01, 2018 at 14:32:23 +0300, Nikolay Shirokovskiy wrote:
> The only possible value is 'maximum' which makes l2_cache_size large
> enough to keep all metadata in memory. This will unleash disks
> performace for some database workloads and IO benchmarks with random
> access to whole disk.
> 
> Note that imlementation sets l2-cache-size and not cache-size.
> Both *cache-size's is upper limit on cache size value thus instead of
> setting precise limit for disk which involves knowing disk size
> and disk's cluster size we can just set INT64_MAX. Unfortunately
> both old and new versions of qemu fail on setting cache-size to INT64_MAX.
> Fortunately both old and new versions works well on such setting for
> l2-cache-size. As guest performance depends only l2 cache size
> and not refcount cache size (which is documented in recent qemu)
> we can set l2 directly.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy virtuozzo com>
> ---
>  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_command.c                            | 26 ++++++++++++
>  src/qemu/qemu_domain.c                             |  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 +
>  11 files changed, 198 insertions(+)
>  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
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8189959..93e0009 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3556,6 +3556,13 @@
>              virt queues for virtio-blk. (<span class="since">Since 3.9.0</span>)
>            </li>
>            <li>
> +            The optional <code>metadata_cache_size</code> attribute specifies
> +            metadata cache size policy. The only possible value is "maximum" to
> +            keep all metadata in cache, this will help if workload needs access
> +            to whole disk all the time.  (<span class="since">Since
> +            4.9.0</span>)

I wanted to complain that we prefer camelCase to underscores generally,
but given that the <driver> element has at least 4 attributes using
underscores that point would be moot.

What's missing though is the description of the default value when the
attribute is not present. Also I think that we should allow to pass
"default" as a valid argument.

> +          </li>
> +          <li>
>            For virtio disks,
>            <a href="#elementsVirtio">Virtio-specific options</a> can also be
>            set. (<span class="since">Since 3.5.0</span>)
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 099a949..18efa3a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1990,6 +1990,9 @@
>          <ref name="detect_zeroes"/>
>        </optional>
>        <optional>
> +        <ref name="metadata_cache_size"/>
> +      </optional>
> +      <optional>
>          <attribute name='queues'>
>            <ref name="positiveInteger"/>
>          </attribute>
> @@ -2090,6 +2093,13 @@
>        </choice>
>      </attribute>
>    </define>
> +  <define name="metadata_cache_size">
> +    <attribute name='metadata_cache_size'>
> +      <choice>
> +        <value>maximum</value>

Here default should be allowed as well.

> +      </choice>
> +    </attribute>
> +  </define>
>    <define name="controller">
>      <element name="controller">
>        <optional>

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1ff593c..b33e6a5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1330,6 +1330,21 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
>          return -1;
>      }
>  
> +    if (disk->metadata_cache_size &&
> +        (disk->src->type != VIR_STORAGE_TYPE_FILE ||

Why don't do this for network-based qcow2s?

> +         disk->src->format != VIR_STORAGE_FILE_QCOW2)) {

Note that a QCOW2 can also be a part of the backing chain where the top
image format is not qcow2. In such case it would not work. Without
-blockdev support I don't see a possibility to expose that to qemu
though since I expect the -drive command being rejected if the top image
is not a qcow2.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("metadata_cache_size can only be set for qcow2 disks"));
> +        return -1;
> +    }
> +
> +    if (disk->metadata_cache_size &&

This part is common to both of the above conditions.

> +        disk->metadata_cache_size != VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("metadata_cache_size can only be set to 'maximum'"));
> +        return -1;
> +    }
> +
>      if (qemuCaps) {
>          if (disk->serial &&
>              disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> @@ -1353,6 +1368,14 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
>                             _("detect_zeroes is not supported by this QEMU binary"));
>              return -1;
>          }
> +
> +        if (disk->metadata_cache_size &&
> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("setting metadata_cache_size is not supported by "
> +                             "this QEMU binary"));
> +            return -1;
> +        }
>      }
>  
>      if (disk->serial &&
> @@ -1776,6 +1799,9 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>                            virDomainDiskIoTypeToString(disk->iomode));

Additions to XML should be in a separate patch from actual implementation.

>      }
>  
> +    if (disk->metadata_cache_size == VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM)
> +        virBufferAsprintf(&opt, ",l2-cache-size=%ld", INT64_MAX);
> +
>      qemuBuildDiskThrottling(disk, &opt);
>  
>      if (virBufferCheckError(&opt) < 0)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff6..896adf3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9074,6 +9074,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>      /* "snapshot" is a libvirt internal field and thus can be changed */
>      /* startupPolicy is allowed to be updated. Therefore not checked here. */
>      CHECK_EQ(transient, "transient", true);
> +    CHECK_EQ(metadata_cache_size, "metadata_cache_size", true);
>  
>      /* Note: For some address types the address auto generation for
>       * @disk has still not happened at this point (e.g. driver

[...]

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 39a7f1f..a0a2ff3 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3044,6 +3044,8 @@ mymain(void)
>      DO_TEST_CAPS_ARCH_LATEST("x86_64-pc-headless", "x86_64");
>      DO_TEST_CAPS_ARCH_LATEST("x86_64-q35-headless", "x86_64");
>  
> +    DO_TEST("disk-metadata_cache_size", QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE);

Please use DO_TEST_LATEST for this rather than hardcoding caps.

> +
>      if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
>          virFileDeleteTree(fakerootdir);
>  

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]