[Qemu-devel] [PATCH 0/2] block: wr_highest_sector blockstat

[Qemu-devel] [PATCH 0/2] block: wr_highest_sector blockstat

[Kevin Wolf]

This is the minimal high watermark implementation that is needed to allow
clients to poll the value. I hope everyone can live with this solution now.

The second patch of this series is optional. It breaks clients that consider
the user monitor a stable API. They should be using QMP, but I'm not sure if we
want to break compatibility on the user monitor today.

Kevin Wolf (2):
  block: Add wr_highest_sector blockstat
  block: Extend info blockstats monitor command

 block.c     |  103 ++++++++++++++++++++++++++++++++++++++++++++--------------
 block_int.h |    1 +
 qdict.c     |    2 +-
 3 files changed, 80 insertions(+), 26 deletions(-)

[Qemu-devel] [PATCH 1/2] block: Add wr_highest_sector blockstat

[Kevin Wolf]

This adds the wr_highest_sector blockstat which implements what is generally
known as the high watermark. It is the highest offset of a sector written to
the respective BlockDriverState since it has been opened.

The query-blockstat QMP command is extended to add this value to the result,
and also to add the statistics of the underlying protocol in a new "parent"
field. Note that to get the "high watermark" of a qcow2 image, you need to look
into the wr_highest_sector field of the parent (which can be a file, a
host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
is the highest offset on the _virtual_ disk that the guest has written to.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 block_int.h |    1 +
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 91fecab..b75cef2 100644
--- a/block.c
+++ b/block.c
@@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
         set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
     }
 
+    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
+        bs->wr_highest_sector = sector_num + nb_sectors - 1;
+    }
+
     return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
 }
 
@@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
                         qdict_get_int(qdict, "wr_bytes"),
                         qdict_get_int(qdict, "rd_operations"),
                         qdict_get_int(qdict, "wr_operations"));
+    if (qdict_haskey(qdict, "parent")) {
+        QObject *parent = qdict_get(qdict, "parent");
+        bdrv_stats_iter(parent, mon);
+    }
 }
 
 void bdrv_stats_print(Monitor *mon, const QObject *data)
@@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
     qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon);
 }
 
+static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
+{
+    QObject *res;
+    QObject *parent = NULL;
+
+    if (bs->file) {
+        parent = bdrv_info_stats_bs(bs->file);
+    }
+
+    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
+                             "'rd_bytes': %" PRId64 ","
+                             "'wr_bytes': %" PRId64 ","
+                             "'rd_operations': %" PRId64 ","
+                             "'wr_operations': %" PRId64 ","
+                             "'wr_highest_offset': %" PRId64
+                             "} }",
+                             bs->device_name,
+                             bs->rd_bytes, bs->wr_bytes,
+                             bs->rd_ops, bs->wr_ops,
+                             bs->wr_highest_sector * 512);
+    if (parent) {
+        QDict *dict = qobject_to_qdict(res);
+        qdict_put_obj(dict, "parent", parent);
+    }
+
+    return res;
+}
+
 /**
  * bdrv_info_stats(): show block device statistics
  *
@@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
  *     - "wr_bytes": bytes written
  *     - "rd_operations": read operations
  *     - "wr_operations": write operations
- * 
+ *     - "wr_highest_offset": Highest offset of a sector written since the
+ *       BlockDriverState has been opened
+ *     - "parent": Contains recursively the statistics of the underlying
+ *       protocol (e.g. the host file for a qcow2 image). If there is no
+ *       underlying protocol, this field is omitted.
+ *
  * Example:
  *
  * [ { "device": "ide0-hd0",
  *               "stats": { "rd_bytes": 512,
  *                          "wr_bytes": 0,
  *                          "rd_operations": 1,
- *                          "wr_operations": 0 } },
+ *                          "wr_operations": 0,
+ *                          "wr_highest_offset": 0,
+ *                          "parent": {
+ *                              "stats": { "rd_bytes": 1024,
+ *                                         "wr_bytes": 0,
+ *                                         "rd_operations": 2,
+ *                                         "wr_operations": 0,
+ *                                         "wr_highest_offset": 0,
+ *                              }
+ *                          } } },
  *   { "device": "ide1-cd0",
  *               "stats": { "rd_bytes": 0,
  *                          "wr_bytes": 0,
  *                          "rd_operations": 0,
- *                          "wr_operations": 0 } } ]
+ *                          "wr_operations": 0,
+ *                          "wr_highest_offset": 0 } },
  */
 void bdrv_info_stats(Monitor *mon, QObject **ret_data)
 {
@@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
     devices = qlist_new();
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
-        obj = qobject_from_jsonf("{ 'device': %s, 'stats': {"
-                                 "'rd_bytes': %" PRId64 ","
-                                 "'wr_bytes': %" PRId64 ","
-                                 "'rd_operations': %" PRId64 ","
-                                 "'wr_operations': %" PRId64
-                                 "} }",
-                                 bs->device_name,
-                                 bs->rd_bytes, bs->wr_bytes,
-                                 bs->rd_ops, bs->wr_ops);
+        obj = bdrv_info_stats_bs(bs);
         qlist_append_obj(devices, obj);
     }
 
@@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
                                cb, opaque);
 
     if (ret) {
-	/* Update stats even though technically transfer has not happened. */
-	bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
-	bs->wr_ops ++;
+        /* Update stats even though technically transfer has not happened. */
+        bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
+        bs->wr_ops ++;
+        if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
+            bs->wr_highest_sector = sector_num + nb_sectors - 1;
+        }
     }
 
     return ret;
diff --git a/block_int.h b/block_int.h
index a3afe63..1a7240c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -167,6 +167,7 @@ struct BlockDriverState {
     uint64_t wr_bytes;
     uint64_t rd_ops;
     uint64_t wr_ops;
+    uint64_t wr_highest_sector;
 
     /* Whether the disk can expand beyond total_sectors */
     int growable;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Anthony Liguori]

On 04/28/2010 10:56 AM, Kevin Wolf wrote:
> This adds the wr_highest_sector blockstat which implements what is generally
> known as the high watermark. It is the highest offset of a sector written to
> the respective BlockDriverState since it has been opened.
>
> The query-blockstat QMP command is extended to add this value to the result,
> and also to add the statistics of the underlying protocol in a new "parent"
> field. Note that to get the "high watermark" of a qcow2 image, you need to look
> into the wr_highest_sector field of the parent (which can be a file, a
> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
> is the highest offset on the _virtual_ disk that the guest has written to.
>    

Is that the desirable behavior?

When dealing with layered formats, would it be useful to print out stats 
for each of the layers?

Regards,

Anthony Liguori

> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   block.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>   block_int.h |    1 +
>   2 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 91fecab..b75cef2 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>           set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>       }
>
> +    if (bs->wr_highest_sector<  sector_num + nb_sectors - 1) {
> +        bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +    }
> +
>       return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
>   }
>
> @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
>                           qdict_get_int(qdict, "wr_bytes"),
>                           qdict_get_int(qdict, "rd_operations"),
>                           qdict_get_int(qdict, "wr_operations"));
> +    if (qdict_haskey(qdict, "parent")) {
> +        QObject *parent = qdict_get(qdict, "parent");
> +        bdrv_stats_iter(parent, mon);
> +    }
>   }
>
>   void bdrv_stats_print(Monitor *mon, const QObject *data)
> @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>       qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon);
>   }
>
> +static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
> +{
> +    QObject *res;
> +    QObject *parent = NULL;
> +
> +    if (bs->file) {
> +        parent = bdrv_info_stats_bs(bs->file);
> +    }
> +
> +    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> +                             "'rd_bytes': %" PRId64 ","
> +                             "'wr_bytes': %" PRId64 ","
> +                             "'rd_operations': %" PRId64 ","
> +                             "'wr_operations': %" PRId64 ","
> +                             "'wr_highest_offset': %" PRId64
> +                             "} }",
> +                             bs->device_name,
> +                             bs->rd_bytes, bs->wr_bytes,
> +                             bs->rd_ops, bs->wr_ops,
> +                             bs->wr_highest_sector * 512);
> +    if (parent) {
> +        QDict *dict = qobject_to_qdict(res);
> +        qdict_put_obj(dict, "parent", parent);
> +    }
> +
> +    return res;
> +}
> +
>   /**
>    * bdrv_info_stats(): show block device statistics
>    *
> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>    *     - "wr_bytes": bytes written
>    *     - "rd_operations": read operations
>    *     - "wr_operations": write operations
> - *
> + *     - "wr_highest_offset": Highest offset of a sector written since the
> + *       BlockDriverState has been opened
> + *     - "parent": Contains recursively the statistics of the underlying
> + *       protocol (e.g. the host file for a qcow2 image). If there is no
> + *       underlying protocol, this field is omitted.
> + *
>    * Example:
>    *
>    * [ { "device": "ide0-hd0",
>    *               "stats": { "rd_bytes": 512,
>    *                          "wr_bytes": 0,
>    *                          "rd_operations": 1,
> - *                          "wr_operations": 0 } },
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0,
> + *                          "parent": {
> + *                              "stats": { "rd_bytes": 1024,
> + *                                         "wr_bytes": 0,
> + *                                         "rd_operations": 2,
> + *                                         "wr_operations": 0,
> + *                                         "wr_highest_offset": 0,
> + *                              }
> + *                          } } },
>    *   { "device": "ide1-cd0",
>    *               "stats": { "rd_bytes": 0,
>    *                          "wr_bytes": 0,
>    *                          "rd_operations": 0,
> - *                          "wr_operations": 0 } } ]
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0 } },
>    */
>   void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>   {
> @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>       devices = qlist_new();
>
>       QTAILQ_FOREACH(bs,&bdrv_states, list) {
> -        obj = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> -                                 "'rd_bytes': %" PRId64 ","
> -                                 "'wr_bytes': %" PRId64 ","
> -                                 "'rd_operations': %" PRId64 ","
> -                                 "'wr_operations': %" PRId64
> -                                 "} }",
> -                                 bs->device_name,
> -                                 bs->rd_bytes, bs->wr_bytes,
> -                                 bs->rd_ops, bs->wr_ops);
> +        obj = bdrv_info_stats_bs(bs);
>           qlist_append_obj(devices, obj);
>       }
>
> @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                  cb, opaque);
>
>       if (ret) {
> -	/* Update stats even though technically transfer has not happened. */
> -	bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> -	bs->wr_ops ++;
> +        /* Update stats even though technically transfer has not happened. */
> +        bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> +        bs->wr_ops ++;
> +        if (bs->wr_highest_sector<  sector_num + nb_sectors - 1) {
> +            bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +        }
>       }
>
>       return ret;
> diff --git a/block_int.h b/block_int.h
> index a3afe63..1a7240c 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -167,6 +167,7 @@ struct BlockDriverState {
>       uint64_t wr_bytes;
>       uint64_t rd_ops;
>       uint64_t wr_ops;
> +    uint64_t wr_highest_sector;
>
>       /* Whether the disk can expand beyond total_sectors */
>       int growable;
>    

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Anthony Liguori]

On 04/28/2010 10:56 AM, Kevin Wolf wrote:
> This adds the wr_highest_sector blockstat which implements what is generally
> known as the high watermark. It is the highest offset of a sector written to
> the respective BlockDriverState since it has been opened.
>
> The query-blockstat QMP command is extended to add this value to the result,
> and also to add the statistics of the underlying protocol in a new "parent"
> field. Note that to get the "high watermark" of a qcow2 image, you need to look
> into the wr_highest_sector field of the parent (which can be a file, a
> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
> is the highest offset on the _virtual_ disk that the guest has written to.
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>    

I see, you did print out stats for each layer.

I don't think we should take 2/2.  I don't mind QMP having more features 
than the user monitor.

Regards,

Anthony Liguori

> ---
>   block.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>   block_int.h |    1 +
>   2 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 91fecab..b75cef2 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>           set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>       }
>
> +    if (bs->wr_highest_sector<  sector_num + nb_sectors - 1) {
> +        bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +    }
> +
>       return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
>   }
>
> @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
>                           qdict_get_int(qdict, "wr_bytes"),
>                           qdict_get_int(qdict, "rd_operations"),
>                           qdict_get_int(qdict, "wr_operations"));
> +    if (qdict_haskey(qdict, "parent")) {
> +        QObject *parent = qdict_get(qdict, "parent");
> +        bdrv_stats_iter(parent, mon);
> +    }
>   }
>
>   void bdrv_stats_print(Monitor *mon, const QObject *data)
> @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>       qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon);
>   }
>
> +static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
> +{
> +    QObject *res;
> +    QObject *parent = NULL;
> +
> +    if (bs->file) {
> +        parent = bdrv_info_stats_bs(bs->file);
> +    }
> +
> +    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> +                             "'rd_bytes': %" PRId64 ","
> +                             "'wr_bytes': %" PRId64 ","
> +                             "'rd_operations': %" PRId64 ","
> +                             "'wr_operations': %" PRId64 ","
> +                             "'wr_highest_offset': %" PRId64
> +                             "} }",
> +                             bs->device_name,
> +                             bs->rd_bytes, bs->wr_bytes,
> +                             bs->rd_ops, bs->wr_ops,
> +                             bs->wr_highest_sector * 512);
> +    if (parent) {
> +        QDict *dict = qobject_to_qdict(res);
> +        qdict_put_obj(dict, "parent", parent);
> +    }
> +
> +    return res;
> +}
> +
>   /**
>    * bdrv_info_stats(): show block device statistics
>    *
> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>    *     - "wr_bytes": bytes written
>    *     - "rd_operations": read operations
>    *     - "wr_operations": write operations
> - *
> + *     - "wr_highest_offset": Highest offset of a sector written since the
> + *       BlockDriverState has been opened
> + *     - "parent": Contains recursively the statistics of the underlying
> + *       protocol (e.g. the host file for a qcow2 image). If there is no
> + *       underlying protocol, this field is omitted.
> + *
>    * Example:
>    *
>    * [ { "device": "ide0-hd0",
>    *               "stats": { "rd_bytes": 512,
>    *                          "wr_bytes": 0,
>    *                          "rd_operations": 1,
> - *                          "wr_operations": 0 } },
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0,
> + *                          "parent": {
> + *                              "stats": { "rd_bytes": 1024,
> + *                                         "wr_bytes": 0,
> + *                                         "rd_operations": 2,
> + *                                         "wr_operations": 0,
> + *                                         "wr_highest_offset": 0,
> + *                              }
> + *                          } } },
>    *   { "device": "ide1-cd0",
>    *               "stats": { "rd_bytes": 0,
>    *                          "wr_bytes": 0,
>    *                          "rd_operations": 0,
> - *                          "wr_operations": 0 } } ]
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0 } },
>    */
>   void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>   {
> @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>       devices = qlist_new();
>
>       QTAILQ_FOREACH(bs,&bdrv_states, list) {
> -        obj = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> -                                 "'rd_bytes': %" PRId64 ","
> -                                 "'wr_bytes': %" PRId64 ","
> -                                 "'rd_operations': %" PRId64 ","
> -                                 "'wr_operations': %" PRId64
> -                                 "} }",
> -                                 bs->device_name,
> -                                 bs->rd_bytes, bs->wr_bytes,
> -                                 bs->rd_ops, bs->wr_ops);
> +        obj = bdrv_info_stats_bs(bs);
>           qlist_append_obj(devices, obj);
>       }
>
> @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                  cb, opaque);
>
>       if (ret) {
> -	/* Update stats even though technically transfer has not happened. */
> -	bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> -	bs->wr_ops ++;
> +        /* Update stats even though technically transfer has not happened. */
> +        bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> +        bs->wr_ops ++;
> +        if (bs->wr_highest_sector<  sector_num + nb_sectors - 1) {
> +            bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +        }
>       }
>
>       return ret;
> diff --git a/block_int.h b/block_int.h
> index a3afe63..1a7240c 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -167,6 +167,7 @@ struct BlockDriverState {
>       uint64_t wr_bytes;
>       uint64_t rd_ops;
>       uint64_t wr_ops;
> +    uint64_t wr_highest_sector;
>
>       /* Whether the disk can expand beyond total_sectors */
>       int growable;
>    

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Luiz Capitulino]

On Wed, 28 Apr 2010 11:01:12 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 04/28/2010 10:56 AM, Kevin Wolf wrote:
> > This adds the wr_highest_sector blockstat which implements what is generally
> > known as the high watermark. It is the highest offset of a sector written to
> > the respective BlockDriverState since it has been opened.
> >
> > The query-blockstat QMP command is extended to add this value to the result,
> > and also to add the statistics of the underlying protocol in a new "parent"
> > field. Note that to get the "high watermark" of a qcow2 image, you need to look
> > into the wr_highest_sector field of the parent (which can be a file, a
> > host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
> > is the highest offset on the _virtual_ disk that the guest has written to.
> >
> > Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> >    
> 
> I see, you did print out stats for each layer.
> 
> I don't think we should take 2/2.  I don't mind QMP having more features 
> than the user monitor.

 I don't either, but Kevin has said to me that this information is also good
for the user Monitor.

 The real question here is whether or not we're going to stop supporting
stability for the user Monitor and if so, when we'll break it.

 An arguable reasonable policy would be to try to maintain stability for
existing commands. In this specific case, 'info blockstats' is used by
libvirt afaik. So breaking it would mean that older libvirt versions won't
be able to talk to newer qemu (taking libvirt just as real known example).

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Anthony Liguori]

On 04/28/2010 12:04 PM, Luiz Capitulino wrote:
> On Wed, 28 Apr 2010 11:01:12 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 04/28/2010 10:56 AM, Kevin Wolf wrote:
>>      
>>> This adds the wr_highest_sector blockstat which implements what is generally
>>> known as the high watermark. It is the highest offset of a sector written to
>>> the respective BlockDriverState since it has been opened.
>>>
>>> The query-blockstat QMP command is extended to add this value to the result,
>>> and also to add the statistics of the underlying protocol in a new "parent"
>>> field. Note that to get the "high watermark" of a qcow2 image, you need to look
>>> into the wr_highest_sector field of the parent (which can be a file, a
>>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
>>> is the highest offset on the _virtual_ disk that the guest has written to.
>>>
>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>>
>>>        
>> I see, you did print out stats for each layer.
>>
>> I don't think we should take 2/2.  I don't mind QMP having more features
>> than the user monitor.
>>      
>   I don't either, but Kevin has said to me that this information is also good
> for the user Monitor.
>
>   The real question here is whether or not we're going to stop supporting
> stability for the user Monitor and if so, when we'll break it.
>
>   An arguable reasonable policy would be to try to maintain stability for
> existing commands. In this specific case, 'info blockstats' is used by
> libvirt afaik. So breaking it would mean that older libvirt versions won't
> be able to talk to newer qemu (taking libvirt just as real known example).
>    

I think we should try our best to maintain compatibility.  In this case, 
this change would break any non-QMP version of libvirt so it would be 
pretty painful for users.  That's why I'm inclined to not take.

It would be reasonable to add a new info command for the user monitor if 
the functionality is desirable.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Luiz Capitulino]

On Wed, 28 Apr 2010 12:47:49 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 04/28/2010 12:04 PM, Luiz Capitulino wrote:
> > On Wed, 28 Apr 2010 11:01:12 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 04/28/2010 10:56 AM, Kevin Wolf wrote:
> >>      
> >>> This adds the wr_highest_sector blockstat which implements what is generally
> >>> known as the high watermark. It is the highest offset of a sector written to
> >>> the respective BlockDriverState since it has been opened.
> >>>
> >>> The query-blockstat QMP command is extended to add this value to the result,
> >>> and also to add the statistics of the underlying protocol in a new "parent"
> >>> field. Note that to get the "high watermark" of a qcow2 image, you need to look
> >>> into the wr_highest_sector field of the parent (which can be a file, a
> >>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
> >>> is the highest offset on the _virtual_ disk that the guest has written to.
> >>>
> >>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> >>>
> >>>        
> >> I see, you did print out stats for each layer.
> >>
> >> I don't think we should take 2/2.  I don't mind QMP having more features
> >> than the user monitor.
> >>      
> >   I don't either, but Kevin has said to me that this information is also good
> > for the user Monitor.
> >
> >   The real question here is whether or not we're going to stop supporting
> > stability for the user Monitor and if so, when we'll break it.
> >
> >   An arguable reasonable policy would be to try to maintain stability for
> > existing commands. In this specific case, 'info blockstats' is used by
> > libvirt afaik. So breaking it would mean that older libvirt versions won't
> > be able to talk to newer qemu (taking libvirt just as real known example).
> >    
> 
> I think we should try our best to maintain compatibility.  In this case, 
> this change would break any non-QMP version of libvirt so it would be 
> pretty painful for users.  That's why I'm inclined to not take.
> 
> It would be reasonable to add a new info command for the user monitor if 
> the functionality is desirable.

 Seems a good solution to me too.

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Daniel P. Berrange]

On Wed, Apr 28, 2010 at 12:47:49PM -0500, Anthony Liguori wrote:
> On 04/28/2010 12:04 PM, Luiz Capitulino wrote:
> >On Wed, 28 Apr 2010 11:01:12 -0500
> >Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >   
> >>On 04/28/2010 10:56 AM, Kevin Wolf wrote:
> >>     
> >>>This adds the wr_highest_sector blockstat which implements what is 
> >>>generally
> >>>known as the high watermark. It is the highest offset of a sector 
> >>>written to
> >>>the respective BlockDriverState since it has been opened.
> >>>
> >>>The query-blockstat QMP command is extended to add this value to the 
> >>>result,
> >>>and also to add the statistics of the underlying protocol in a new 
> >>>"parent"
> >>>field. Note that to get the "high watermark" of a qcow2 image, you need 
> >>>to look
> >>>into the wr_highest_sector field of the parent (which can be a file, a
> >>>host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState 
> >>>itself
> >>>is the highest offset on the _virtual_ disk that the guest has written 
> >>>to.
> >>>
> >>>Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> >>>
> >>>       
> >>I see, you did print out stats for each layer.
> >>
> >>I don't think we should take 2/2.  I don't mind QMP having more features
> >>than the user monitor.
> >>     
> >  I don't either, but Kevin has said to me that this information is also 
> >  good
> >for the user Monitor.
> >
> >  The real question here is whether or not we're going to stop supporting
> >stability for the user Monitor and if so, when we'll break it.
> >
> >  An arguable reasonable policy would be to try to maintain stability for
> >existing commands. In this specific case, 'info blockstats' is used by
> >libvirt afaik. So breaking it would mean that older libvirt versions won't
> >be able to talk to newer qemu (taking libvirt just as real known example).
> >   
> 
> I think we should try our best to maintain compatibility.  In this case, 
> this change would break any non-QMP version of libvirt so it would be 
> pretty painful for users.  That's why I'm inclined to not take.

I agree. There are alot of existing deployed libvirt's out there which all
still use the user monitor. libvirt will enable JSON by default once QEMU
brings out 0.13, but there'll still be people with older libvirt. We need
to avoid breaking the user monitor for at least one or two releaes to allow
time for apps to catch up with the switch to JSON. Mark the user monitor
deprecated in 0.13 by all means, but keep compatability until either the 
0.15 or 0.16 release if at all practical.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Kevin Wolf]

Am 28.04.2010 18:01, schrieb Anthony Liguori:
> On 04/28/2010 10:56 AM, Kevin Wolf wrote:
>> This adds the wr_highest_sector blockstat which implements what is generally
>> known as the high watermark. It is the highest offset of a sector written to
>> the respective BlockDriverState since it has been opened.
>>
>> The query-blockstat QMP command is extended to add this value to the result,
>> and also to add the statistics of the underlying protocol in a new "parent"
>> field. Note that to get the "high watermark" of a qcow2 image, you need to look
>> into the wr_highest_sector field of the parent (which can be a file, a
>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
>> is the highest offset on the _virtual_ disk that the guest has written to.
>>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>    
> 
> I see, you did print out stats for each layer.
> 
> I don't think we should take 2/2.  I don't mind QMP having more features 
> than the user monitor.

I agree, just posted it because it felt incomplete without it and I
wanted to give you the choice.

The only use case in the user monitor I saw for it would have been qemu
development anyway. I can keep the patch in a local branch for this (or
just rewrite it when/if I need it - it's small enough).

Kevin

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Luiz Capitulino]

On Wed, 28 Apr 2010 17:56:20 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> This adds the wr_highest_sector blockstat which implements what is generally
> known as the high watermark. It is the highest offset of a sector written to
> the respective BlockDriverState since it has been opened.
> 
> The query-blockstat QMP command is extended to add this value to the result,
> and also to add the statistics of the underlying protocol in a new "parent"
> field. Note that to get the "high watermark" of a qcow2 image, you need to look
> into the wr_highest_sector field of the parent (which can be a file, a
> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
> is the highest offset on the _virtual_ disk that the guest has written to.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  block_int.h |    1 +
>  2 files changed, 62 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 91fecab..b75cef2 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>          set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>      }
>  
> +    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
> +        bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +    }
> +
>      return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
>  }
>  
> @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
>                          qdict_get_int(qdict, "wr_bytes"),
>                          qdict_get_int(qdict, "rd_operations"),
>                          qdict_get_int(qdict, "wr_operations"));
> +    if (qdict_haskey(qdict, "parent")) {
> +        QObject *parent = qdict_get(qdict, "parent");
> +        bdrv_stats_iter(parent, mon);
> +    }
>  }
>  
>  void bdrv_stats_print(Monitor *mon, const QObject *data)
> @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>      qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon);
>  }
>  
> +static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
> +{
> +    QObject *res;
> +    QObject *parent = NULL;
> +
> +    if (bs->file) {
> +        parent = bdrv_info_stats_bs(bs->file);
> +    }

 Doesn't build, I guess you meant 'backing_hd'.

> +
> +    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> +                             "'rd_bytes': %" PRId64 ","
> +                             "'wr_bytes': %" PRId64 ","
> +                             "'rd_operations': %" PRId64 ","
> +                             "'wr_operations': %" PRId64 ","
> +                             "'wr_highest_offset': %" PRId64
> +                             "} }",
> +                             bs->device_name,
> +                             bs->rd_bytes, bs->wr_bytes,
> +                             bs->rd_ops, bs->wr_ops,
> +                             bs->wr_highest_sector * 512);
> +    if (parent) {
> +        QDict *dict = qobject_to_qdict(res);
> +        qdict_put_obj(dict, "parent", parent);
> +    }
> +
> +    return res;
> +}
> +
>  /**
>   * bdrv_info_stats(): show block device statistics
>   *
> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>   *     - "wr_bytes": bytes written
>   *     - "rd_operations": read operations
>   *     - "wr_operations": write operations
> - * 
> + *     - "wr_highest_offset": Highest offset of a sector written since the
> + *       BlockDriverState has been opened
> + *     - "parent": Contains recursively the statistics of the underlying
> + *       protocol (e.g. the host file for a qcow2 image). If there is no
> + *       underlying protocol, this field is omitted.

 Looks like you are not pushing 'protocol', we already have 'format'
though (not sure if they overlap).

 Also, 'device' is "" for 'parent'. You should consider not pushing the
key in this case (and noting that it's optional in the doc above).

> + *
>   * Example:
>   *
>   * [ { "device": "ide0-hd0",
>   *               "stats": { "rd_bytes": 512,
>   *                          "wr_bytes": 0,
>   *                          "rd_operations": 1,
> - *                          "wr_operations": 0 } },
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0,
> + *                          "parent": {
> + *                              "stats": { "rd_bytes": 1024,
> + *                                         "wr_bytes": 0,
> + *                                         "rd_operations": 2,
> + *                                         "wr_operations": 0,
> + *                                         "wr_highest_offset": 0,
> + *                              }
> + *                          } } },
>   *   { "device": "ide1-cd0",
>   *               "stats": { "rd_bytes": 0,
>   *                          "wr_bytes": 0,
>   *                          "rd_operations": 0,
> - *                          "wr_operations": 0 } } ]
> + *                          "wr_operations": 0,
> + *                          "wr_highest_offset": 0 } },
>   */
>  void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>  {
> @@ -1567,15 +1618,7 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
>      devices = qlist_new();
>  
>      QTAILQ_FOREACH(bs, &bdrv_states, list) {
> -        obj = qobject_from_jsonf("{ 'device': %s, 'stats': {"
> -                                 "'rd_bytes': %" PRId64 ","
> -                                 "'wr_bytes': %" PRId64 ","
> -                                 "'rd_operations': %" PRId64 ","
> -                                 "'wr_operations': %" PRId64
> -                                 "} }",
> -                                 bs->device_name,
> -                                 bs->rd_bytes, bs->wr_bytes,
> -                                 bs->rd_ops, bs->wr_ops);
> +        obj = bdrv_info_stats_bs(bs);
>          qlist_append_obj(devices, obj);
>      }
>  
> @@ -1834,9 +1877,12 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                 cb, opaque);
>  
>      if (ret) {
> -	/* Update stats even though technically transfer has not happened. */
> -	bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> -	bs->wr_ops ++;
> +        /* Update stats even though technically transfer has not happened. */
> +        bs->wr_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> +        bs->wr_ops ++;
> +        if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
> +            bs->wr_highest_sector = sector_num + nb_sectors - 1;
> +        }
>      }
>  
>      return ret;
> diff --git a/block_int.h b/block_int.h
> index a3afe63..1a7240c 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -167,6 +167,7 @@ struct BlockDriverState {
>      uint64_t wr_bytes;
>      uint64_t rd_ops;
>      uint64_t wr_ops;
> +    uint64_t wr_highest_sector;
>  
>      /* Whether the disk can expand beyond total_sectors */
>      int growable;

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Kevin Wolf]

Am 28.04.2010 19:31, schrieb Luiz Capitulino:
> On Wed, 28 Apr 2010 17:56:20 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> This adds the wr_highest_sector blockstat which implements what is generally
>> known as the high watermark. It is the highest offset of a sector written to
>> the respective BlockDriverState since it has been opened.
>>
>> The query-blockstat QMP command is extended to add this value to the result,
>> and also to add the statistics of the underlying protocol in a new "parent"
>> field. Note that to get the "high watermark" of a qcow2 image, you need to look
>> into the wr_highest_sector field of the parent (which can be a file, a
>> host_device, ...). The wr_highest_sector of the qcow2 BlockDriverState itself
>> is the highest offset on the _virtual_ disk that the guest has written to.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>>  block_int.h |    1 +
>>  2 files changed, 62 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 91fecab..b75cef2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -880,6 +880,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>>          set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
>>      }
>>  
>> +    if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>> +        bs->wr_highest_sector = sector_num + nb_sectors - 1;
>> +    }
>> +
>>      return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
>>  }
>>  
>> @@ -1523,6 +1527,10 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
>>                          qdict_get_int(qdict, "wr_bytes"),
>>                          qdict_get_int(qdict, "rd_operations"),
>>                          qdict_get_int(qdict, "wr_operations"));
>> +    if (qdict_haskey(qdict, "parent")) {
>> +        QObject *parent = qdict_get(qdict, "parent");
>> +        bdrv_stats_iter(parent, mon);
>> +    }
>>  }
>>  
>>  void bdrv_stats_print(Monitor *mon, const QObject *data)
>> @@ -1530,6 +1538,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>>      qlist_iter(qobject_to_qlist(data), bdrv_stats_iter, mon);
>>  }
>>  
>> +static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
>> +{
>> +    QObject *res;
>> +    QObject *parent = NULL;
>> +
>> +    if (bs->file) {
>> +        parent = bdrv_info_stats_bs(bs->file);
>> +    }
> 
>  Doesn't build, I guess you meant 'backing_hd'.

The patch is against git://repo.or.cz/qemu/kevin.git block, not master
(like most block related patches lately).

backing_hd is for qcow base images, so that's a different thing.

>> +
>> +    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
>> +                             "'rd_bytes': %" PRId64 ","
>> +                             "'wr_bytes': %" PRId64 ","
>> +                             "'rd_operations': %" PRId64 ","
>> +                             "'wr_operations': %" PRId64 ","
>> +                             "'wr_highest_offset': %" PRId64
>> +                             "} }",
>> +                             bs->device_name,
>> +                             bs->rd_bytes, bs->wr_bytes,
>> +                             bs->rd_ops, bs->wr_ops,
>> +                             bs->wr_highest_sector * 512);
>> +    if (parent) {
>> +        QDict *dict = qobject_to_qdict(res);
>> +        qdict_put_obj(dict, "parent", parent);
>> +    }
>> +
>> +    return res;
>> +}
>> +
>>  /**
>>   * bdrv_info_stats(): show block device statistics
>>   *
>> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
>>   *     - "wr_bytes": bytes written
>>   *     - "rd_operations": read operations
>>   *     - "wr_operations": write operations
>> - * 
>> + *     - "wr_highest_offset": Highest offset of a sector written since the
>> + *       BlockDriverState has been opened
>> + *     - "parent": Contains recursively the statistics of the underlying
>> + *       protocol (e.g. the host file for a qcow2 image). If there is no
>> + *       underlying protocol, this field is omitted.
> 
>  Looks like you are not pushing 'protocol', we already have 'format'
> though (not sure if they overlap).

You're talking about a new key 'protocol'? I'm not sure what it should
describe, to be honest.

>  Also, 'device' is "" for 'parent'. You should consider not pushing the
> key in this case (and noting that it's optional in the doc above).

Ok, will change that for v2.

Kevin

[Qemu-devel] Re: [PATCH 1/2] block: Add wr_highest_sector blockstat

[Luiz Capitulino]

On Thu, 29 Apr 2010 12:00:45 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> >>  /**
> >>   * bdrv_info_stats(): show block device statistics
> >>   *
> >> @@ -1544,19 +1580,34 @@ void bdrv_stats_print(Monitor *mon, const QObject *data)
> >>   *     - "wr_bytes": bytes written
> >>   *     - "rd_operations": read operations
> >>   *     - "wr_operations": write operations
> >> - * 
> >> + *     - "wr_highest_offset": Highest offset of a sector written since the
> >> + *       BlockDriverState has been opened
> >> + *     - "parent": Contains recursively the statistics of the underlying
> >> + *       protocol (e.g. the host file for a qcow2 image). If there is no
> >> + *       underlying protocol, this field is omitted.
> > 
> >  Looks like you are not pushing 'protocol', we already have 'format'
> > though (not sure if they overlap).
> 
> You're talking about a new key 'protocol'? I'm not sure what it should
> describe, to be honest.

 I thought I read somewhere you were going to have a 'protocol' key as well
(in addition to 'wr_highest_offset' and 'parent') but looks like I was just
confused.

 Sorry for the noise.

[Qemu-devel] [PATCH 2/2] block: Extend info blockstats monitor command

[Kevin Wolf]

Now the high watermark and statistics of the underlying images are exposed via
QMP, but they are missing in the user monitor. This patch changes the user
monitor to provide the same functionality.

Note that it's not possible to maintain compatibility with older clients that
try to parse the output of this command. They need to use QMP for a stable API.

The new info blockstats output looks like this (and yes, the example shows that
read/write statistics are broken currently, they ignore synchronous I/O):

(qemu) info blockstats
ide0-hd0:
  qcow2         rd_bytes=814592 rd_ops=1591
                wr_bytes=0 wr_ops=0 wr_highest_offset=0

  file          rd_bytes=814592 rd_ops=1591
                wr_bytes=0 wr_ops=0 wr_highest_offset=0

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   35 +++++++++++++++++++++--------------
 qdict.c |    2 +-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index b75cef2..b22325e 100644
--- a/block.c
+++ b/block.c
@@ -1517,19 +1517,25 @@ static void bdrv_stats_iter(QObject *data, void *opaque)
     qdict = qobject_to_qdict(data);
     monitor_printf(mon, "%s:", qdict_get_str(qdict, "device"));
 
-    qdict = qobject_to_qdict(qdict_get(qdict, "stats"));
-    monitor_printf(mon, " rd_bytes=%" PRId64
-                        " wr_bytes=%" PRId64
-                        " rd_operations=%" PRId64
-                        " wr_operations=%" PRId64
-                        "\n",
-                        qdict_get_int(qdict, "rd_bytes"),
-                        qdict_get_int(qdict, "wr_bytes"),
-                        qdict_get_int(qdict, "rd_operations"),
-                        qdict_get_int(qdict, "wr_operations"));
-    if (qdict_haskey(qdict, "parent")) {
-        QObject *parent = qdict_get(qdict, "parent");
-        bdrv_stats_iter(parent, mon);
+    while (qdict) {
+        QDict *stats = qobject_to_qdict(qdict_get(qdict, "stats"));
+
+        monitor_printf(mon, "\n  %-12s  rd_bytes=%" PRId64
+                            " rd_ops=%" PRId64
+                            "\n  %-12s  wr_bytes=%" PRId64
+                            " wr_ops=%" PRId64
+                            " wr_highest_offset=%" PRId64
+                            "\n",
+                            qdict_get_str(qdict, "format"),
+                            qdict_get_int(stats, "rd_bytes"),
+                            qdict_get_int(stats, "rd_operations"),
+                            "",
+                            qdict_get_int(stats, "wr_bytes"),
+                            qdict_get_int(stats, "wr_operations"),
+                            qdict_get_int(stats, "wr_highest_offset"));
+
+
+        qdict = qobject_to_qdict(qdict_get(qdict, "parent"));
     }
 }
 
@@ -1547,7 +1553,7 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
         parent = bdrv_info_stats_bs(bs->file);
     }
 
-    res = qobject_from_jsonf("{ 'device': %s, 'stats': {"
+    res = qobject_from_jsonf("{ 'device': %s, 'format': %s, 'stats': {"
                              "'rd_bytes': %" PRId64 ","
                              "'wr_bytes': %" PRId64 ","
                              "'rd_operations': %" PRId64 ","
@@ -1555,6 +1561,7 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
                              "'wr_highest_offset': %" PRId64
                              "} }",
                              bs->device_name,
+                             bs->drv ? bs->drv->format_name : "none",
                              bs->rd_bytes, bs->wr_bytes,
                              bs->rd_ops, bs->wr_ops,
                              bs->wr_highest_sector * 512);
diff --git a/qdict.c b/qdict.c
index aae57bf..32491db 100644
--- a/qdict.c
+++ b/qdict.c
@@ -46,7 +46,7 @@ QDict *qdict_new(void)
  */
 QDict *qobject_to_qdict(const QObject *obj)
 {
-    if (qobject_type(obj) != QTYPE_QDICT)
+    if (!obj || qobject_type(obj) != QTYPE_QDICT)
         return NULL;
 
     return container_of(obj, QDict, base);
-- 
1.6.6.1