[Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

 Hi,

 This series is based on a previous series submitted by Uri Lublin:

http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html

 Details on the patches, except for this question: does it make sense to have
a 'low' watermark for block devices?

 I think it doesn't, then the event (and the monitor accompanying command)
should be called BLOCK_HIGH_WATERMARK. But this makes the event very
unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
high/low watermark type.

 It's a machine protocol, so I don't think the additional parameter
matters much.

[Qemu-devel] [PATCH 1/3] block-qcow2: keep highest allocated offset

[Luiz Capitulino]

From: Uri Lublin <uril@redhat.com>

We want to know the highest allocated offset for qcow2 images.

It can be useful for allocating more diskspace for an image
(e.g. an lvm logical volume) before we run out-of-disk-space.

In this version image refcount table is not scanned.
Also highest-alloc is not kept when the process exits.
Thus it only keeps the highest offset of the current run.

Signed-off-by: Uri Lublin <uril@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block/qcow2-refcount.c |    3 +++
 block/qcow2.c          |    2 ++
 block/qcow2.h          |    3 +++
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 917fc88..9cb38c8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -550,6 +550,9 @@ retry:
             size,
             (s->free_cluster_index - nb_clusters) << s->cluster_bits);
 #endif
+    if (s->highest_alloc < s->free_cluster_index) {
+        s->highest_alloc = s->free_cluster_index;
+    }
     return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 5b6dad9..d9af90b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -230,6 +230,8 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     if (qcow_read_extensions(bs, sizeof(header), ext_end))
         goto fail;
 
+    s->highest_alloc = 0;
+
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
diff --git a/block/qcow2.h b/block/qcow2.h
index de9397a..6cc50e6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -112,6 +112,9 @@ typedef struct BDRVQcowState {
     uint32_t crypt_method_header;
     AES_KEY aes_encrypt_key;
     AES_KEY aes_decrypt_key;
+
+    int64_t highest_alloc; /* highest cluester allocated (in clusters) */
+
     uint64_t snapshots_offset;
     int snapshots_size;
     int nb_snapshots;
-- 
1.7.0.1

[Qemu-devel] [PATCH 2/3] monitor: Introduce block_watermark command

[Luiz Capitulino]

This command sets a watermark value for a device. The next commit
will add the BLOCK_WATERMARK event, which is emitted when this
value is reached.

Please, note that currently only the 'high' watermark value is
supported.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c         |    5 +++++
 block.h         |    2 ++
 block_int.h     |    3 +++
 monitor.c       |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx |   16 ++++++++++++++++
 5 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 31d1ba4..260502a 100644
--- a/block.c
+++ b/block.c
@@ -1209,6 +1209,11 @@ void bdrv_flush_all(void)
             bdrv_flush(bs);
 }
 
+void bdrv_set_high_watermark(BlockDriverState *bs, uint64_t offset)
+{
+    bs->high_watermark = offset;
+}
+
 /*
  * Returns true iff the specified sector is present in the disk image. Drivers
  * not implementing the functionality are assumed to not support backing files,
diff --git a/block.h b/block.h
index edf5704..af32436 100644
--- a/block.h
+++ b/block.h
@@ -130,6 +130,8 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
 void bdrv_flush(BlockDriverState *bs);
 void bdrv_flush_all(void);
 
+void bdrv_set_high_watermark(BlockDriverState *bs, uint64_t offset);
+
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 	int *pnum);
 
diff --git a/block_int.h b/block_int.h
index 50e1a0b..53e087d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -156,6 +156,9 @@ struct BlockDriverState {
 
     void *sync_aiocb;
 
+    /* high watermark for monitor event */
+    uint64_t high_watermark;
+
     /* I/O stats (display with "info blockstats"). */
     uint64_t rd_bytes;
     uint64_t wr_bytes;
diff --git a/monitor.c b/monitor.c
index 672ae47..595bd62 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1041,6 +1041,52 @@ static int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return eject_device(mon, bs, force);
 }
 
+static int do_block_set_watermark(Monitor *mon, const QDict *qdict,
+                                  QObject **ret_data)
+{
+    int64_t offset;
+    char format[32];
+    BlockDriverState *bs;
+
+    bs = bdrv_find(qdict_get_str(qdict, "device"));
+    if (!bs) {
+        qemu_error_new(QERR_DEVICE_NOT_FOUND, qdict_get_str(qdict, "device"));
+        return -1;
+    }
+
+    bdrv_get_format(bs, format, sizeof(format));
+
+    if (format[0] == '\0') {
+        qemu_error_new(QERR_DEVICE_NOT_ACTIVE, qdict_get_str(qdict, "device"));
+        return -1;
+    }
+
+    /* we only support qcow2 currently */
+    if (strcmp(format, "qcow2")) {
+        qemu_error_new(QERR_INVALID_BLOCK_FORMAT, format);
+        return -1;
+    }
+
+    if (strcmp(qdict_get_str(qdict, "type"), "high")) {
+        qemu_error_new(QERR_INVALID_PARAMETER, "type");
+        return -1;
+    }
+
+    offset = qdict_get_int(qdict, "offset");
+    if (offset < 0) {
+        qemu_error_new(QERR_INVALID_PARAMETER, "offset");
+        return -1;
+    }
+
+    /* 
+     * XXX: 'offset' is int64_t because the monitor code doesn't
+     * work with uint64_t. Does it matter? If it does, the
+     * monitor has to be fixed.
+     */
+    bdrv_set_high_watermark(bs, offset);
+    return 0;
+}
+
 static int do_block_set_passwd(Monitor *mon, const QDict *qdict,
                                 QObject **ret_data)
 {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 7f9d261..c4b48db 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1127,6 +1127,22 @@ Set the encrypted device @var{device} password to @var{password}
 ETEXI
 
     {
+        .name       = "block_watermark",
+        .args_type  = "device:B,type:s,offset:l",
+        .params     = "device type offset",
+        .help       = "set block device watermark",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set_watermark,
+    },
+
+STEXI
+@item block_watermark @var{device} @var{type} @var{offset}
+@findex block_watermark
+Set the block device @var{device} watermark @var{type} to @var{offset}
+ETEXI
+
+
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
-- 
1.7.0.1

[Qemu-devel] [PATCH 3/3] QMP: Introduce BLOCK_WATERMARK event

[Luiz Capitulino]

Emitted whenever the watermark value set by the Monitor
'block_watermark' command is reached.

This value is not permanently stored and it's automatically
set to zero when the event is emitted.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt     |   26 ++++++++++++++++++++++++++
 block/qcow2-refcount.c |   20 ++++++++++++++++++++
 monitor.c              |    3 +++
 monitor.h              |    1 +
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index a94e9b4..f1fe3aa 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -26,6 +26,32 @@ Example:
 Note: If action is "stop", a STOP event will eventually follow the
 BLOCK_IO_ERROR event.
 
+BLOCK_WATERMARK
+---------------
+
+Emitted when the watermark value for a device is reached.
+
+Data:
+
+- "device": device name (json-string)
+- "type": watermark type (json-string, only "high" currently)
+- "mark": current device's high watermark value (json-int)
+- "allocation": current allocation (json-int)
+
+Example:
+
+{ "event": "BLOCK_WATERMARK",
+    "data": { "device": "ide0-hd0",
+              "ide0-hd0",
+              "mark": 1048576,
+              "allocation": 1376256 },
+    "timestamp": { "seconds": 1268170711, "microseconds": 698676 } }
+
+Note: (1) The watermark value is set by the 'block_watermark' Monitor's command
+      (2) The device's watermark value is set to zero when the event is
+          emitted. Thus, this event is emitted only once.
+      (3) Currently only the high watermark value is supported
+
 RESET
 -----
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9cb38c8..684e0ff 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -23,6 +23,8 @@
  */
 
 #include "qemu-common.h"
+#include "qjson.h"
+#include "monitor.h"
 #include "block_int.h"
 #include "block/qcow2.h"
 
@@ -531,6 +533,17 @@ static int update_cluster_refcount(BlockDriverState *bs,
 /* cluster allocation functions */
 
 
+static void watermark_mon_event(const BlockDriverState *bs, int64_t ha)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'device': %s, 'type': 'high', "
+                              "'allocation': %" PRId64 ", 'mark': %" PRId64 "}",
+                              bs->device_name, ha, bs->high_watermark);
+
+    monitor_protocol_event(QEVENT_BLOCK_WATERMARK, data);
+    qobject_decref(data);
+}
 
 /* return < 0 if error */
 static int64_t alloc_clusters_noref(BlockDriverState *bs, int64_t size)
@@ -552,6 +565,13 @@ retry:
 #endif
     if (s->highest_alloc < s->free_cluster_index) {
         s->highest_alloc = s->free_cluster_index;
+        if (bs->high_watermark) {
+            uint64_t ha = (s->highest_alloc << s->cluster_bits);
+            if (ha >= bs->high_watermark) {
+                watermark_mon_event(bs, ha);
+                bs->high_watermark = 0;
+            }
+        }
     }
     return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
 }
diff --git a/monitor.c b/monitor.c
index 595bd62..3f74994 100644
--- a/monitor.c
+++ b/monitor.c
@@ -423,6 +423,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_BLOCK_IO_ERROR:
             event_name = "BLOCK_IO_ERROR";
             break;
+        case QEVENT_BLOCK_WATERMARK:
+            event_name = "BLOCK_WATERMARK";
+            break;
         case QEVENT_RTC_CHANGE:
             event_name = "RTC_CHANGE";
             break;
diff --git a/monitor.h b/monitor.h
index e5f2d2b..e767990 100644
--- a/monitor.h
+++ b/monitor.h
@@ -23,6 +23,7 @@ typedef enum MonitorEvent {
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
     QEVENT_BLOCK_IO_ERROR,
+    QEVENT_BLOCK_WATERMARK,
     QEVENT_RTC_CHANGE,
     QEVENT_WATCHDOG,
     QEVENT_MAX,
-- 
1.7.0.1

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>   Hi,
>
>   This series is based on a previous series submitted by Uri Lublin:
>
> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>
>   Details on the patches, except for this question: does it make sense to have
> a 'low' watermark for block devices?
>    

Does it make sense to have a high watermark?

Regards,

Anthony Liguori

>   I think it doesn't, then the event (and the monitor accompanying command)
> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> high/low watermark type.
>
>   It's a machine protocol, so I don't think the additional parameter
> matters much.
>
>
>    

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>   Hi,
>
>   This series is based on a previous series submitted by Uri Lublin:
>
> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>
>   Details on the patches, except for this question: does it make sense to have
> a 'low' watermark for block devices?
>
>   I think it doesn't, then the event (and the monitor accompanying command)
> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> high/low watermark type.
>    

The alternative way to implement this is for a management tool to just 
poll the allocated disk size periodically.

It's no more/less safe than generating an event on a "watermark" because 
the event is still racy with respect to a guest that's writing very 
quickly to the disk.

Regards,

Anthony Liguori

>   It's a machine protocol, so I don't think the additional parameter
> matters much.
>
>
>    

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Tue, 09 Mar 2010 17:03:14 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
> >   Hi,
> >
> >   This series is based on a previous series submitted by Uri Lublin:
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
> >
> >   Details on the patches, except for this question: does it make sense to have
> > a 'low' watermark for block devices?
> >    
> 
> Does it make sense to have a high watermark?

 Yes, as it avoids polling :)

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Tue, 09 Mar 2010 17:08:33 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
> >   Hi,
> >
> >   This series is based on a previous series submitted by Uri Lublin:
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
> >
> >   Details on the patches, except for this question: does it make sense to have
> > a 'low' watermark for block devices?
> >
> >   I think it doesn't, then the event (and the monitor accompanying command)
> > should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> > unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> > high/low watermark type.
> >    
> 
> The alternative way to implement this is for a management tool to just 
> poll the allocated disk size periodically.
> 
> It's no more/less safe than generating an event on a "watermark" because 
> the event is still racy with respect to a guest that's writing very 
> quickly to the disk.

 The argument against polling is not only about possible races, but
it's something very inefficient to do as the condition you're polling
for may never happen but you're wasting cpu resources for it.

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Tue, 9 Mar 2010 20:22:03 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 09 Mar 2010 17:08:33 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> 
> > On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
> > >   Hi,
> > >
> > >   This series is based on a previous series submitted by Uri Lublin:
> > >
> > > http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
> > >
> > >   Details on the patches, except for this question: does it make sense to have
> > > a 'low' watermark for block devices?
> > >
> > >   I think it doesn't, then the event (and the monitor accompanying command)
> > > should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> > > unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> > > high/low watermark type.
> > >    
> > 
> > The alternative way to implement this is for a management tool to just 
> > poll the allocated disk size periodically.
> > 
> > It's no more/less safe than generating an event on a "watermark" because 
> > the event is still racy with respect to a guest that's writing very 
> > quickly to the disk.
> 
>  The argument against polling is not only about possible races, but
> it's something very inefficient to do as the condition you're polling
> for may never happen but you're wasting cpu resources for it.

 Also note that this applies for other events and if this becomes the
standard mode of operation, the end result is that we're delegating
event generation for the management tools.

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/09/2010 05:22 PM, Luiz Capitulino wrote:
> On Tue, 09 Mar 2010 17:08:33 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>>      
>>>    Hi,
>>>
>>>    This series is based on a previous series submitted by Uri Lublin:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>>>
>>>    Details on the patches, except for this question: does it make sense to have
>>> a 'low' watermark for block devices?
>>>
>>>    I think it doesn't, then the event (and the monitor accompanying command)
>>> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
>>> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
>>> high/low watermark type.
>>>
>>>        
>> The alternative way to implement this is for a management tool to just
>> poll the allocated disk size periodically.
>>
>> It's no more/less safe than generating an event on a "watermark" because
>> the event is still racy with respect to a guest that's writing very
>> quickly to the disk.
>>      
>   The argument against polling is not only about possible races, but
> it's something very inefficient to do as the condition you're polling
> for may never happen but you're wasting cpu resources for it.
>    

No, you just move the polling into qemu in the block I/O path.  It's now 
"polling" for whether a watermark has been hit.

The problem with this functionality is that it's extremely tied to a 
very specific management policy.  It's not at all generic.

Polling is an entirely legitimate thing for a management tool to do and 
we shouldn't introduce events that don't make sense just to avoid polling.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/09/2010 05:25 PM, Luiz Capitulino wrote:
>   Also note that this applies for other events and if this becomes the
> standard mode of operation, the end result is that we're delegating
> event generation for the management tools.
>    

We will end up with hundreds of events if we implement this sort of 
thing for every management tool.

There's a significant difference between a VNC connect event and a "high 
watermark" event.

A client connects and disconnects relatively infrequently.  They are 
absolute events and are likely to trigger some action.  A management 
tool could poll but generally speaking, generating a notification has 
wide valid.

A "high watermark" event is an event that occurs because one statistic 
that we track hit a very simple condition: stat > THRESHOLD.  But as the 
commit message alludes to, why is stat > THRESHOLD interesting and stat 
< THRESHOLD not interesting?

Moreover, why isn't SUM(stat[0..n]) > THRESHOLD not the truly 
interesting event?  It's because in this instance, a very specific 
management tool happens to implement a heuristic based on stat > 
THRESHOLD.  However, a different management tool is likely to implement 
a significantly different heuristic.  This isn't limited to the block 
device.

A good example is the recent balloon statistics information.  We're 
implementing a daemon that periodically polls the balloon driver using a 
rather sophisticated set of conditions to take actions.  You could make 
the same argument that we should push those heuristics into QEMU which 
would mean that daemon wouldn't need to exist.

Management tools are going to have to poll statistics from QEMU to 
implement heuristics.  It's not something we can hide in events.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Kevin Wolf]

Am 09.03.2010 23:53, schrieb Luiz Capitulino:
>  Hi,
> 
>  This series is based on a previous series submitted by Uri Lublin:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
> 
>  Details on the patches, except for this question: does it make sense to have
> a 'low' watermark for block devices?
> 
>  I think it doesn't, then the event (and the monitor accompanying command)
> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> high/low watermark type.

When should a low watermark trigger? Images only ever grow, so testing
if the highest allocated block goes below some watermark would never
work (except for some unlikely special cases like deleting a snapshot
after which no new clusters have been allocated).

>  It's a machine protocol, so I don't think the additional parameter
> matters much.

I don't think anyone is ever going to use it, but it probably wouldn't
hurt either.

Kevin

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Kevin Wolf]

Am 10.03.2010 00:08, schrieb Anthony Liguori:
> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>>   Hi,
>>
>>   This series is based on a previous series submitted by Uri Lublin:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>>
>>   Details on the patches, except for this question: does it make sense to have
>> a 'low' watermark for block devices?
>>
>>   I think it doesn't, then the event (and the monitor accompanying command)
>> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
>> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
>> high/low watermark type.
>>    
> 
> The alternative way to implement this is for a management tool to just 
> poll the allocated disk size periodically.

Then we need to provide that information using the monitor. As far as I
know, we don't do that yet. Not doing that would mean that the
management tool would have to open an image which is already in use by
qemu (which is already something I feel uncomfortable about) and search
for the highest allocated cluster (which makes it completely inefficient
and therefore basically forbids the use case).

Really, we have no choice but to implement the high watermark tracking
in the qemu block layer. The only question is if we have a monitor
command to ask for the current value or if we signal an event if it goes
above a given threshold.

I don't think I'm really decided on that question.

> It's no more/less safe than generating an event on a "watermark" because 
> the event is still racy with respect to a guest that's writing very 
> quickly to the disk.

Being racy isn't a problem, a management tool doing this kind of things
needs to use werror=ENOSPC (at least) anyway. The watermark thing, as I
understand it, is only a mechanism to make it less likely that the VM
has to be stopped.

Kevin

Re: [Qemu-devel] Re: [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Christoph Hellwig]

On Wed, Mar 10, 2010 at 09:33:31AM +0100, Kevin Wolf wrote:
> When should a low watermark trigger? Images only ever grow, so testing
> if the highest allocated block goes below some watermark would never
> work (except for some unlikely special cases like deleting a snapshot
> after which no new clusters have been allocated).

Wither proper guest support images can shrink.  I'm about to land
support for that in qemu, although for now just for every limited
setups.

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Tue, 09 Mar 2010 17:55:41 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/09/2010 05:25 PM, Luiz Capitulino wrote:
> >   Also note that this applies for other events and if this becomes the
> > standard mode of operation, the end result is that we're delegating
> > event generation for the management tools.
> >    
> 
> We will end up with hundreds of events if we implement this sort of 
> thing for every management tool.
> 
> There's a significant difference between a VNC connect event and a "high 
> watermark" event.
> 
> A client connects and disconnects relatively infrequently.  They are 
> absolute events and are likely to trigger some action.  A management 
> tool could poll but generally speaking, generating a notification has 
> wide valid.
> 
> A "high watermark" event is an event that occurs because one statistic 
> that we track hit a very simple condition: stat > THRESHOLD.  But as the 
> commit message alludes to, why is stat > THRESHOLD interesting and stat 
> < THRESHOLD not interesting?

 Well, maybe we need to define a criteria for what should be an
event and what shouldn't then? Or what should be done by QEMU and
what should be done by the management tool?

 Also, would be good to hear from management tool people (that's
usually libvirt people) what they think about doing this kind of
thing by polling.

 That said, we can't do that for this specific case as explained by
Kevin.

> Moreover, why isn't SUM(stat[0..n]) > THRESHOLD not the truly 
> interesting event?  It's because in this instance, a very specific 
> management tool happens to implement a heuristic based on stat > 
> THRESHOLD.  However, a different management tool is likely to implement 
> a significantly different heuristic.  This isn't limited to the block 
> device.
> 
> A good example is the recent balloon statistics information.  We're 
> implementing a daemon that periodically polls the balloon driver using a 
> rather sophisticated set of conditions to take actions.  You could make 
> the same argument that we should push those heuristics into QEMU which 
> would mean that daemon wouldn't need to exist.
> 
> Management tools are going to have to poll statistics from QEMU to 
> implement heuristics.  It's not something we can hide in events.
> 
> Regards,
> 
> Anthony Liguori

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Wed, 10 Mar 2010 09:40:42 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 10.03.2010 00:08, schrieb Anthony Liguori:
> > On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
> >>   Hi,
> >>
> >>   This series is based on a previous series submitted by Uri Lublin:
> >>
> >> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
> >>
> >>   Details on the patches, except for this question: does it make sense to have
> >> a 'low' watermark for block devices?
> >>
> >>   I think it doesn't, then the event (and the monitor accompanying command)
> >> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
> >> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
> >> high/low watermark type.
> >>    
> > 
> > The alternative way to implement this is for a management tool to just 
> > poll the allocated disk size periodically.
> 
> Then we need to provide that information using the monitor. As far as I
> know, we don't do that yet.

 No, we don't. We have a 'info blockstats' command though and it could
provide image related stats info if needed.

> Not doing that would mean that the
> management tool would have to open an image which is already in use by
> qemu (which is already something I feel uncomfortable about) and search
> for the highest allocated cluster (which makes it completely inefficient
> and therefore basically forbids the use case).
> 
> Really, we have no choice but to implement the high watermark tracking
> in the qemu block layer. The only question is if we have a monitor
> command to ask for the current value or if we signal an event if it goes
> above a given threshold.
> 
> I don't think I'm really decided on that question.

 I'm not either and that's why I'd like to hear from the management
tool people.

> > It's no more/less safe than generating an event on a "watermark" because 
> > the event is still racy with respect to a guest that's writing very 
> > quickly to the disk.
> 
> Being racy isn't a problem, a management tool doing this kind of things
> needs to use werror=ENOSPC (at least) anyway. The watermark thing, as I
> understand it, is only a mechanism to make it less likely that the VM
> has to be stopped.
> 
> Kevin

Re: [Qemu-devel] Re: [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Luiz Capitulino]

On Wed, 10 Mar 2010 10:28:43 +0100
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Mar 10, 2010 at 09:33:31AM +0100, Kevin Wolf wrote:
> > When should a low watermark trigger? Images only ever grow, so testing
> > if the highest allocated block goes below some watermark would never
> > work (except for some unlikely special cases like deleting a snapshot
> > after which no new clusters have been allocated).
> 
> Wither proper guest support images can shrink.  I'm about to land
> support for that in qemu, although for now just for every limited
> setups.

 It were this kind of future functionality I was concerned about.

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/10/2010 02:40 AM, Kevin Wolf wrote:
> Am 10.03.2010 00:08, schrieb Anthony Liguori:
>    
>> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>>      
>>>    Hi,
>>>
>>>    This series is based on a previous series submitted by Uri Lublin:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>>>
>>>    Details on the patches, except for this question: does it make sense to have
>>> a 'low' watermark for block devices?
>>>
>>>    I think it doesn't, then the event (and the monitor accompanying command)
>>> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
>>> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
>>> high/low watermark type.
>>>
>>>        
>> The alternative way to implement this is for a management tool to just
>> poll the allocated disk size periodically.
>>      
> Then we need to provide that information using the monitor. As far as I
> know, we don't do that yet.

Okay, but that's certainly a reasonable thing to add though, no?


>> It's no more/less safe than generating an event on a "watermark" because
>> the event is still racy with respect to a guest that's writing very
>> quickly to the disk.
>>      
> Being racy isn't a problem, a management tool doing this kind of things
> needs to use werror=ENOSPC (at least) anyway. The watermark thing, as I
> understand it, is only a mechanism to make it less likely that the VM
> has to be stopped.
>    

Correct.  A management tool could poll every 5 minutes to make the same 
determination.

This approach seems superior to me because it's considerably more 
flexible.  In this particular model, you have one disk on an LVM volume 
and you need to grow that single disk image when you hit a high water mark.

However, an alternative and equally valid model would be an LVM volume 
containing a file system with multiple disk images for a single guest.  
In this case, there is no high water mark for an individual disk, but 
rather, there's a high water mark for the combination of all the disks.

You can do both with polling.  Ultimately, it comes down to a question 
of mechanism vs. policy.  You're generating an event based on a policy 
(stat > THRESHOLD) verses providing an event that's a mechanism to 
implement a policy (VNC client connected).

Basically, whenever there's one of more valid choices, it's probably a 
policy and is better done in a management tool.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

> Kevin
>    

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Kevin Wolf]

Am 10.03.2010 22:20, schrieb Anthony Liguori:
> On 03/10/2010 02:40 AM, Kevin Wolf wrote:
>> Am 10.03.2010 00:08, schrieb Anthony Liguori:
>>    
>>> On 03/09/2010 04:53 PM, Luiz Capitulino wrote:
>>>      
>>>>    Hi,
>>>>
>>>>    This series is based on a previous series submitted by Uri Lublin:
>>>>
>>>> http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00864.html
>>>>
>>>>    Details on the patches, except for this question: does it make sense to have
>>>> a 'low' watermark for block devices?
>>>>
>>>>    I think it doesn't, then the event (and the monitor accompanying command)
>>>> should be called BLOCK_HIGH_WATERMARK. But this makes the event very
>>>> unflexible, so I have called it BLOCK_WATERMARK and added parameters for the
>>>> high/low watermark type.
>>>>
>>>>        
>>> The alternative way to implement this is for a management tool to just
>>> poll the allocated disk size periodically.
>>>      
>> Then we need to provide that information using the monitor. As far as I
>> know, we don't do that yet.
> 
> Okay, but that's certainly a reasonable thing to add though, no?

Well, if you're aware of the semantics of this value, it might be. It's
not exactly intuitive, but this is currently hidden inside qemu.

What the high watermark says (in this implementation) is the highest
offset into the image file of an cluster that was allocated during this
qemu run. If you restart qemu, it starts at 0 again.

I think there once was a version that tried to calculate the absolute
highest value when the image was opened, but it was reverted because it
just took too long. For the same reason I think a low watermark is
unrealistic, even if we get shrinking images some time. It's just not
doable efficiently, at least not in an easy way.

I'm not sure if this semantics makes it a good public interface. Other
than that, I'm not overly concerned with doing it like you suggest.

>>> It's no more/less safe than generating an event on a "watermark" because
>>> the event is still racy with respect to a guest that's writing very
>>> quickly to the disk.
>>>      
>> Being racy isn't a problem, a management tool doing this kind of things
>> needs to use werror=ENOSPC (at least) anyway. The watermark thing, as I
>> understand it, is only a mechanism to make it less likely that the VM
>> has to be stopped.
>>    
> 
> Correct.  A management tool could poll every 5 minutes to make the same 
> determination.

Yes. At the cost of doing it too little during installation and too
often during regular operation. Of course, you could add heuristics to
make the interval dynamic...

But honestly, while I do understand your point, this feels like a hack
to work around shortcomings of an interface. So what we need to decide
is which criterion outweighs the other in practice.

> This approach seems superior to me because it's considerably more 
> flexible.  In this particular model, you have one disk on an LVM volume 
> and you need to grow that single disk image when you hit a high water mark.
> 
> However, an alternative and equally valid model would be an LVM volume 
> containing a file system with multiple disk images for a single guest.  
> In this case, there is no high water mark for an individual disk, but 
> rather, there's a high water mark for the combination of all the disks.

If your file system supports sparse files, it's the wrong number. You
need the allocated space then, not the highest offset. I really can't
see much use of this watermark outside the original model.

Kevin

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/11/2010 02:34 AM, Kevin Wolf wrote:
>
> Well, if you're aware of the semantics of this value, it might be. It's
> not exactly intuitive, but this is currently hidden inside qemu.
>
> What the high watermark says (in this implementation) is the highest
> offset into the image file of an cluster that was allocated during this
> qemu run. If you restart qemu, it starts at 0 again.
>
> I think there once was a version that tried to calculate the absolute
> highest value when the image was opened, but it was reverted because it
> just took too long. For the same reason I think a low watermark is
> unrealistic, even if we get shrinking images some time. It's just not
> doable efficiently, at least not in an easy way.
>
> I'm not sure if this semantics makes it a good public interface. Other
> than that, I'm not overly concerned with doing it like you suggest.
>    

Making it an event certainly exposes it as part of the public interface 
though, no?
Correct. A management tool could poll every 5 minutes to make the same

>> determination.
>>      
> Yes. At the cost of doing it too little during installation and too
> often during regular operation. Of course, you could add heuristics to
> make the interval dynamic...
>    

Depends on how much room you leave in your watermark.

> But honestly, while I do understand your point, this feels like a hack
> to work around shortcomings of an interface. So what we need to decide
> is which criterion outweighs the other in practice.
>    

If I understand the use case correctly, what this really boils down to 
is that you want to create a growable image on top of a non-growable 
device.  The management tool then deals with growth using this interface.

I'm somewhat inclined to suggest that the proper way to support this is 
to teach qemu how to grow the LVM volume like it would grow any normal file.

>> This approach seems superior to me because it's considerably more
>> flexible.  In this particular model, you have one disk on an LVM volume
>> and you need to grow that single disk image when you hit a high water mark.
>>
>> However, an alternative and equally valid model would be an LVM volume
>> containing a file system with multiple disk images for a single guest.
>> In this case, there is no high water mark for an individual disk, but
>> rather, there's a high water mark for the combination of all the disks.
>>      
> If your file system supports sparse files, it's the wrong number. You
> need the allocated space then, not the highest offset. I really can't
> see much use of this watermark outside the original model.
>    

Good point.

Regards,

Anthony Liguori

> Kevin
>    

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Avi Kivity]

On 03/11/2010 04:19 PM, Anthony Liguori wrote:
>
>> But honestly, while I do understand your point, this feels like a hack
>> to work around shortcomings of an interface. So what we need to decide
>> is which criterion outweighs the other in practice.
>
> If I understand the use case correctly, what this really boils down to 
> is that you want to create a growable image on top of a non-growable 
> device.  The management tool then deals with growth using this interface.
>
> I'm somewhat inclined to suggest that the proper way to support this 
> is to teach qemu how to grow the LVM volume like it would grow any 
> normal file.

Interesting.  Could use a helper or helper.so, or a new block format 
driver (block/lvm.c).  Still, if management is actually interested in 
the change, not just to grow the volume (perhaps to handle out-of-space, 
or to check against a global disk low condition), the helper/driver then 
needs to communicate with the management system, which is awkward.

I guess it's workable though.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Anthony Liguori]

On 03/11/2010 08:58 AM, Avi Kivity wrote:
> On 03/11/2010 04:19 PM, Anthony Liguori wrote:
>>
>>> But honestly, while I do understand your point, this feels like a hack
>>> to work around shortcomings of an interface. So what we need to decide
>>> is which criterion outweighs the other in practice.
>>
>> If I understand the use case correctly, what this really boils down 
>> to is that you want to create a growable image on top of a 
>> non-growable device.  The management tool then deals with growth 
>> using this interface.
>>
>> I'm somewhat inclined to suggest that the proper way to support this 
>> is to teach qemu how to grow the LVM volume like it would grow any 
>> normal file.
>
> Interesting.  Could use a helper or helper.so, or a new block format 
> driver (block/lvm.c).

I've suggested plugins for the block drivers in the past.  I don't think 
it was well received.

>   Still, if management is actually interested in the change, not just 
> to grow the volume (perhaps to handle out-of-space, or to check 
> against a global disk low condition), the helper/driver then needs to 
> communicate with the management system, which is awkward.

The management system can poll information from the block driver.  
Polling is not always evil :-)

Regards,

Anthony Liguori

>
> I guess it's workable though.
>

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Avi Kivity]

On 03/11/2010 05:07 PM, Anthony Liguori wrote:
>>   Still, if management is actually interested in the change, not just 
>> to grow the volume (perhaps to handle out-of-space, or to check 
>> against a global disk low condition), the helper/driver then needs to 
>> communicate with the management system, which is awkward.
>
> The management system can poll information from the block driver.  
> Polling is not always evil :-)

In this particular case, it can even poll lvm directly (and I think it 
does), as it needs global information.

So a variant of the raw driver might work here.  Is there any way to 
pass options to nested format drivers?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/3]: BLOCK_WATERMARK QMP event

[Kevin Wolf]

Am 11.03.2010 15:19, schrieb Anthony Liguori:
> On 03/11/2010 02:34 AM, Kevin Wolf wrote:
>>
>> Well, if you're aware of the semantics of this value, it might be. It's
>> not exactly intuitive, but this is currently hidden inside qemu.
>>
>> What the high watermark says (in this implementation) is the highest
>> offset into the image file of an cluster that was allocated during this
>> qemu run. If you restart qemu, it starts at 0 again.
>>
>> I think there once was a version that tried to calculate the absolute
>> highest value when the image was opened, but it was reverted because it
>> just took too long. For the same reason I think a low watermark is
>> unrealistic, even if we get shrinking images some time. It's just not
>> doable efficiently, at least not in an easy way.
>>
>> I'm not sure if this semantics makes it a good public interface. Other
>> than that, I'm not overly concerned with doing it like you suggest.
> 
> Making it an event certainly exposes it as part of the public interface 
> though, no?

Not sure if we're talking about the same "it". If we generate an event
when the absolute value goes over a given threshold we hide the somewhat
non-intuitive part of the mechanism, which is the absolute number itself.

>> But honestly, while I do understand your point, this feels like a hack
>> to work around shortcomings of an interface. So what we need to decide
>> is which criterion outweighs the other in practice.
> 
> If I understand the use case correctly, what this really boils down to 
> is that you want to create a growable image on top of a non-growable 
> device.  The management tool then deals with growth using this interface.
> 
> I'm somewhat inclined to suggest that the proper way to support this is 
> to teach qemu how to grow the LVM volume like it would grow any normal file.

Hm... Never thought about that. I'm somewhat inclined to suggest that
you have a point there. :-)

This probably means that we finally need to get the image formats (raw,
qcow2, ...) and the protocols (file, host_cdrom, lvm, ...) properly
separated. Which is a good thing because we should do it anyway.

Kevin