Re: [PATCH 00/23] Add block write threshold event

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

 



On 03/24/2017 09:55 AM, Peter Krempa wrote:
> On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
>> This is another version of the stuff that I've posted here:
>> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
>> which was partially based on the very old discussion at
>> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
>>
>> This version fixes some of the review feedback that I've got and also fixes all
>> the issues pointed out in the original cover letter since I managed to implement
>> the node name detection in a way suitable for this.
>>
>> The event is useful for mgmt apps using thin-provisioned storage so that they
>> don't have to poll for the disk filling all the time.
> 
> I've pushed the updated version after incorporating most of the review
> feedback to:
> 
> git fetch git://pipo.sk/pipo/libvirt.git block-threshold-2
> 

Here's the interdiff; still a couple of things to tweak:

> diff --git w/src/libvirt-domain.c c/src/libvirt-domain.c
> index 815cbb1..ca63138 100644
> --- w/src/libvirt-domain.c
> +++ c/src/libvirt-domain.c
> @@ -11838,8 +11838,12 @@ virDomainSetVcpu(virDomainPtr domain,
>   * Set the threshold level for delivering the
>   * VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD if the device or backing chain element
>   * described by @dev is written beyond the set threshold level. The threshold
> - * level is unset once the event fired. The event may not be delivered at all if
> - * libvirtd was not running at the moment when the threshold was reached.
> + * level is unset once the event fired. The event might not be delivered at all

maybe s/fired/fires/

> + * if libvirtd was not running at the moment when the threshold was reached.
> + *
> + * Hypervisors report the last  written sector of an image in the bulk stats API

extra space

> + * (virConnectGetAllDomainStats/virDomainListGetStats) as
> + * "block.<num>.allocation" in the VIR_DOMAIN_STATS_BLOCK group.

Maybe also mention (thanks to the followup patch) that
block.<num>.threshold can be inspected in bulk stats to learn the
current threshold.

>   *
>   * This event allows to use thin-provisioned storage which needs management
>   * tools to grow it without the need for polling of the data.
> diff --git w/src/qemu/qemu_domain.c c/src/qemu/qemu_domain.c
> index 0469a1f..6985643 100644
> --- w/src/qemu/qemu_domain.c
> +++ c/src/qemu/qemu_domain.c
> @@ -8521,7 +8521,7 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
>   * @src: filled with the specific backing store element if provided
>   * @idx: index of @src in the backing chain, if provided
>   *
> - * Looks up the disk in the domain via @nodename and returns it's definition.
> + * Looks up the disk in the domain via @nodename and returns its definition.
>   * Optionally fills @src and @idx if provided with the specific backing chain
>   * element which corresponds to the node name.
>   */
> diff --git w/src/qemu/qemu_process.c c/src/qemu/qemu_process.c
> index 8477710..53b4c5f 100644
> --- w/src/qemu/qemu_process.c
> +++ c/src/qemu/qemu_process.c
> @@ -1470,6 +1470,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>          if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) {
>              event = virDomainEventBlockThresholdNewFromObj(vm, dev, path,
>                                                             threshold, excess);
> +            VIR_FREE(dev);
>          }

Oh nice - you caught something I missed.

>      }
> 
> diff --git w/src/remote/remote_driver.c c/src/remote/remote_driver.c
> index baa5cba..27bcc9b 100644
> --- w/src/remote/remote_driver.c
> +++ c/src/remote/remote_driver.c
> @@ -8436,7 +8436,7 @@ static virHypervisorDriver hypervisor_driver = {
>      .domainGetGuestVcpus = remoteDomainGetGuestVcpus, /* 2.0.0 */
>      .domainSetGuestVcpus = remoteDomainSetGuestVcpus, /* 2.0.0 */
>      .domainSetVcpu = remoteDomainSetVcpu, /* 3.1.0 */
> -    .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.1.0 */
> +    .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.2.0 */
>  };
> 
>  static virNetworkDriver network_driver = {
> diff --git w/src/util/virbuffer.c c/src/util/virbuffer.c
> index 8f0f49d..80c8e28 100644
> --- w/src/util/virbuffer.c
> +++ c/src/util/virbuffer.c
> @@ -90,7 +90,7 @@ virBufferAdjustIndent(virBufferPtr buf, int indent)
> 
> 
>  /**
> - * virBufferAdjustIndent:
> + * virBufferSetIndent:
>   * @buf: the buffer
>   * @indent: new indentation size.
>   *
> diff --git w/tests/virbuftest.c c/tests/virbuftest.c
> index 34160e6..8ec6ce5 100644
> --- w/tests/virbuftest.c
> +++ c/tests/virbuftest.c
> @@ -405,6 +405,34 @@ testBufEscapeN(const void *opaque)
> 
> 
>  static int
> +testBufSetIndent(const void *opaque ATTRIBUTE_UNUSED)
> +{

I might have just squashed in a test with the existing tests of
AdjustIndent, but what you have is fine.

> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    char *actual;
> +    int ret = -1;
> +
> +    virBufferSetIndent(&buf, 11);
> +    virBufferAddLit(&buf, "test\n");
> +    virBufferSetIndent(&buf, 2);
> +    virBufferAddLit(&buf, "test2\n");
> +
> +    if (!(actual = virBufferContentAndReset(&buf)))
> +        goto cleanup;
> +
> +    if (STRNEQ(actual, "           test\n  test2\n")) {
> +        VIR_TEST_DEBUG("testBufSetIndent: expected indent not set\n");
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(actual);
> +    return ret;
> +}
> +
> +
> +static int
>  mymain(void)
>  {
>      int ret = 0;
> @@ -422,6 +450,7 @@ mymain(void)
>      DO_TEST("Auto-indentation", testBufAutoIndent, 0);
>      DO_TEST("Trim", testBufTrim, 0);
>      DO_TEST("AddBuffer", testBufAddBuffer, 0);
> +    DO_TEST("set indent", testBufSetIndent, 0);
> 
>  #define DO_TEST_ADD_STR(DATA, EXPECT)                                  \
>      do {                                                               \
> diff --git w/tools/virsh-domain.c c/tools/virsh-domain.c
> index 3662952..74d0a0e 100644
> --- w/tools/virsh-domain.c
> +++ c/tools/virsh-domain.c
> @@ -7105,7 +7105,7 @@ static const vshCmdInfo info_domblkthreshold[] = {
>                  "device or it's backing chain element")
>      },
>      {.name = "desc",
> -     .data = N_("set threshold for ")
> +     .data = N_("set threshold for block-threshold even for a block device")

s/even/event/

>      },
>      {.name = NULL}
>  };

Once you make the right tweaks in the corresponding patches, I think
that means you've earned an ACK to the series.  My next email should be
a summary of my testing...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux