[Qemu-devel] [PATCH 0/3 v2] allow online resizing of block devices

[Qemu-devel] [PATCH 0/3 v2] allow online resizing of block devices

[Christoph Hellwig]

This patchset adds support for online resizing of block devices.

The first patch adds a new resize monitor command which call into
the existing image resize code.  This is the meat of the series
and probably needs quite a bit of review and help as I'm not sure
about how to implement the error handling for monitor commands
correctly.  Am I really supposed to add a new QERR_ definition
for each possible error?  And if yes how am I supposed to define
them?  The macros for them aren't exactly self-explaining.

The second patch adds a way to tell drivers about a resize, and the
third one adds a guest notification for config changes to virtio-blk
which allows the guest to pick it up without a rescan.  I've just sent
the corresponding Linux guest driver patch to Rusty.

Changes from version 1 to version 2:
 - also add a QMP command (block_resize)
 - use the o format for the size in the monitor command
 - fix typos
 - use QERR_UNDEFINED_ERROR for errors instead of unstructured strings
 - remove the CDROM hint check
 - add a reason argument to the change callback

[Qemu-devel] [PATCH 1/3] block: add resize monitor command

[Christoph Hellwig]

Add a monitor command that allows resizing of block devices while
qemu is running.  It uses the existing bdrv_truncate method already
used by qemu-img to do it's work.  Compared to qemu-img the size
parsing is very simplicistic, but I think having a properly numering
object is more useful for non-humand monitor users than having
the units and relative resize parsing.

For SCSI devices the new size can be updated in Linux guests by
doing the following shell command:

	echo > /sys/class/scsi_device/0:0:0:0/device/rescan

For ATA devices I don't know of a way to update the block device
size in Linux system, and for virtio-blk the next two patches
will provide an automatic update of the size when this command
is issued on the host.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx	2011-01-19 17:47:10.444004409 +0100
+++ qemu/hmp-commands.hx	2011-01-19 17:49:51.673254095 +0100
@@ -53,6 +53,25 @@ Quit the emulator.
 ETEXI
 
     {
+        .name       = "resize",
+        .args_type  = "id:s,size:o",
+        .params     = "device size",
+        .help       = "resize a block image",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_resize,
+    },
+
+STEXI
+@item resize
+@findex resize
+Resize a block image while a guest is running.  Usually requires guest
+action to see the updated size.  Resize to a lower size is supported,
+but should be used with extreme caution.  Note that this command only
+resizes image files, it can not resize block devices like LVM volumes.
+ETEXI
+
+
+    {
         .name       = "eject",
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c	2011-01-19 17:47:10.455004828 +0100
+++ qemu/blockdev.c	2011-01-19 17:49:51.674253955 +0100
@@ -706,3 +706,33 @@ int do_drive_del(Monitor *mon, const QDi
 
     return 0;
 }
+
+/*
+ * XXX: replace the QERR_UNDEFINED_ERROR errors with real values once the
+ * existing QERR_ macro mess is cleaned up.  A good example for better
+ * error reports can be found in the qemu-img resize code.
+ */
+int do_resize(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    int64_t size = qdict_get_int(qdict, "size");
+    BlockDriverState *bs;
+
+    bs = bdrv_find(id);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+        return -1;
+    }
+
+    if (size < 0) {
+        qerror_report(QERR_UNDEFINED_ERROR);
+        return -1;
+    }
+
+    if (bdrv_truncate(bs, size)) {
+        qerror_report(QERR_UNDEFINED_ERROR);
+        return -1;
+    }
+
+    return 0;
+}
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h	2011-01-19 17:47:10.464012091 +0100
+++ qemu/blockdev.h	2011-01-19 17:49:51.677282590 +0100
@@ -53,5 +53,6 @@ int do_change_block(Monitor *mon, const
                     const char *filename, const char *fmt);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx	2011-01-19 17:47:10.478012371 +0100
+++ qemu/qmp-commands.hx	2011-01-19 17:50:07.406016841 +0100
@@ -601,6 +601,34 @@ Example:
 -> { "execute": "netdev_del", "arguments": { "id": "netdev1" } }
 <- { "return": {} }
 
+
+EQMP
+
+    {
+        .name       = "block_resize",
+        .args_type  = "id:s,size:o",
+        .params     = "id size",
+        .help       = "resize a block image",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_resize,
+    },
+
+SQMP
+block_resize
+------------
+
+Resize a block image while a guest is running.
+
+Arguments:
+
+- "id": the device's ID, must be unique (json-string)
+- "size": new size
+
+Example:
+
+-> { "execute": "block_resize", "arguments": { "id": "scratch", "size": 1073741824 } }
+<- { "return": {} }
+
 EQMP
 
     {

[Qemu-devel] [PATCH 2/3] block: tell drivers about an image resize

[Christoph Hellwig]

Extend the change_cb callback with a reason argument, and use it
to tell drivers about size changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c	2011-01-18 20:54:45.246021572 +0100
+++ qemu/block.c	2011-01-18 20:56:54.117255612 +0100
@@ -645,7 +645,7 @@ int bdrv_open(BlockDriverState *bs, cons
         /* call the change callback */
         bs->media_changed = 1;
         if (bs->change_cb)
-            bs->change_cb(bs->change_opaque);
+            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);
     }
 
     return 0;
@@ -684,7 +684,7 @@ void bdrv_close(BlockDriverState *bs)
         /* call the change callback */
         bs->media_changed = 1;
         if (bs->change_cb)
-            bs->change_cb(bs->change_opaque);
+            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);
     }
 }
 
@@ -1135,6 +1135,8 @@ int bdrv_truncate(BlockDriverState *bs,
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+        if (bs->change_cb)
+            bs->change_cb(bs->change_opaque, CHANGE_SIZE);
     }
     return ret;
 }
@@ -1366,7 +1368,8 @@ int bdrv_enable_write_cache(BlockDriverS
 
 /* XXX: no longer used */
 void bdrv_set_change_cb(BlockDriverState *bs,
-                        void (*change_cb)(void *opaque), void *opaque)
+                        void (*change_cb)(void *opaque, int reason),
+                        void *opaque)
 {
     bs->change_cb = change_cb;
     bs->change_opaque = opaque;
@@ -1411,7 +1414,7 @@ int bdrv_set_key(BlockDriverState *bs, c
         /* call the change callback now, we skipped it on open */
         bs->media_changed = 1;
         if (bs->change_cb)
-            bs->change_cb(bs->change_opaque);
+            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);
     }
     return ret;
 }
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h	2011-01-18 20:57:00.792253448 +0100
+++ qemu/block.h	2011-01-18 20:57:09.771282850 +0100
@@ -182,7 +182,8 @@ int bdrv_is_locked(BlockDriverState *bs)
 void bdrv_set_locked(BlockDriverState *bs, int locked);
 int bdrv_eject(BlockDriverState *bs, int eject_flag);
 void bdrv_set_change_cb(BlockDriverState *bs,
-                        void (*change_cb)(void *opaque), void *opaque);
+                        void (*change_cb)(void *opaque, int reason),
+                        void *opaque);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
 BlockDriverState *bdrv_find(const char *name);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h	2011-01-18 20:55:43.783009908 +0100
+++ qemu/block_int.h	2011-01-18 20:56:58.912004806 +0100
@@ -153,7 +153,7 @@ struct BlockDriverState {
     int valid_key; /* if true, a valid encryption key has been set */
     int sg;        /* if true, the device is a /dev/sg* */
     /* event callback when inserting/removing */
-    void (*change_cb)(void *opaque);
+    void (*change_cb)(void *opaque, int reason);
     void *change_opaque;
 
     BlockDriver *drv; /* NULL means no media */
@@ -203,6 +203,9 @@ struct BlockDriverState {
     void *private;
 };
 
+#define CHANGE_MEDIA	0x01
+#define CHANGE_SIZE	0x02
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/hw/ide/core.c
===================================================================
--- qemu.orig/hw/ide/core.c	2011-01-18 20:57:17.321260780 +0100
+++ qemu/hw/ide/core.c	2011-01-18 20:59:46.242256451 +0100
@@ -1625,11 +1625,15 @@ static void ide_cfata_metadata_write(IDE
 }
 
 /* called when the inserted state of the media has changed */
-static void cdrom_change_cb(void *opaque)
+static void cdrom_change_cb(void *opaque, int reason)
 {
     IDEState *s = opaque;
     uint64_t nb_sectors;
 
+    if (!(reason & CHANGE_MEDIA)) {
+        return;
+    }
+
     bdrv_get_geometry(s->bs, &nb_sectors);
     s->nb_sectors = nb_sectors;
 
Index: qemu/hw/sd.c
===================================================================
--- qemu.orig/hw/sd.c	2011-01-18 20:57:48.978261549 +0100
+++ qemu/hw/sd.c	2011-01-18 20:59:37.000000000 +0100
@@ -422,9 +422,14 @@ static void sd_reset(SDState *sd, BlockD
     sd->pwd_len = 0;
 }
 
-static void sd_cardchange(void *opaque)
+static void sd_cardchange(void *opaque, int reason)
 {
     SDState *sd = opaque;
+
+    if (!(reason & CHANGE_MEDIA)) {
+        return;
+    }
+
     qemu_set_irq(sd->inserted_cb, bdrv_is_inserted(sd->bdrv));
     if (bdrv_is_inserted(sd->bdrv)) {
         sd_reset(sd, sd->bdrv);

[Qemu-devel] [PATCH 3/3] virtio-blk: tell the guest about size changes

[Christoph Hellwig]

Raise a config change interrupt when the size changed.  This allows
virtio-blk guest drivers to read-read the information from the
config space once it got the config chaged interrupt.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: qemu/hw/virtio-blk.c
===================================================================
--- qemu.orig/hw/virtio-blk.c	2011-01-19 17:47:10.335004828 +0100
+++ qemu/hw/virtio-blk.c	2011-01-19 17:50:10.748272881 +0100
@@ -504,6 +504,15 @@ static int virtio_blk_load(QEMUFile *f,
     return 0;
 }
 
+static void virtio_blk_change_cb(void *opaque, int reason)
+{
+    VirtIOBlock *s = opaque;
+
+    if (reason & CHANGE_SIZE) {
+        virtio_notify_config(&s->vdev);
+    }
+}
+
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
 {
     VirtIOBlock *s;
@@ -546,6 +555,7 @@ VirtIODevice *virtio_blk_init(DeviceStat
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
                     virtio_blk_save, virtio_blk_load, s);
     bdrv_set_removable(s->bs, 0);
+    bdrv_set_change_cb(s->bs, virtio_blk_change_cb, s);
     s->bs->buffer_alignment = conf->logical_block_size;
 
     add_boot_device_path(conf->bootindex, dev, "/disk@0,0");

[Qemu-devel] Re: [PATCH 0/3 v2] allow online resizing of block devices

[Juan Quintela]

Christoph Hellwig <hch@lst.de> wrote:
> This patchset adds support for online resizing of block devices.
>
> The first patch adds a new resize monitor command which call into
> the existing image resize code.  This is the meat of the series
> and probably needs quite a bit of review and help as I'm not sure
> about how to implement the error handling for monitor commands
> correctly.  Am I really supposed to add a new QERR_ definition
> for each possible error?  And if yes how am I supposed to define
> them?  The macros for them aren't exactly self-explaining.
>
> The second patch adds a way to tell drivers about a resize, and the
> third one adds a guest notification for config changes to virtio-blk
> which allows the guest to pick it up without a rescan.  I've just sent
> the corresponding Linux guest driver patch to Rusty.
>
> Changes from version 1 to version 2:
>  - also add a QMP command (block_resize)
>  - use the o format for the size in the monitor command
>  - fix typos
>  - use QERR_UNDEFINED_ERROR for errors instead of unstructured strings
>  - remove the CDROM hint check
>  - add a reason argument to the change callback

Does this handle the change of the rtc data?  I can't see how/where.
(Not that I know if it is important at all until reboot).

cmos_init_hb() uses the size to guess the right geometry of the drives,
if we change the size of the drive, should we change that one?



Later, Juan.

hw/pc.c
static void pc_cmos_init_late(void *opaque)
{
    pc_cmos_init_late_arg *arg = opaque;
    ISADevice *s = arg->rtc_state;
    pint val;
    BlockDriverState *hd_table[4];
    int i;

    ide_get_bs(hd_table, arg->idebus0);
    ide_get_bs(hd_table + 2, arg->idebus1);

    rtc_set_memory(s, 0x12, (hd_table[0] ? 0xf0 : 0) | (hd_table[1] ? 0x0f : 0));
    if (hd_table[0])
        cmos_init_hd(0x19, 0x1b, hd_table[0], s);
    if (hd_table[1])
        cmos_init_hd(0x1a, 0x24, hd_table[1], s);

.....
}

static void cmos_init_hd(int type_ofs, int info_ofs, BlockDriverState *hd,
                         ISADevice *s)
{
    int cylinders, heads, sectors;
    bdrv_get_geometry_hint(hd, &cylinders, &heads, &sectors);
    rtc_set_memory(s, type_ofs, 47);
    rtc_set_memory(s, info_ofs, cylinders);
    rtc_set_memory(s, info_ofs + 1, cylinders >> 8);
    rtc_set_memory(s, info_ofs + 2, heads);
....

Re: [Qemu-devel] [PATCH 1/3] block: add resize monitor command

[Stefan Hajnoczi]

On Wed, Jan 19, 2011 at 06:02:48PM +0100, Christoph Hellwig wrote:
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx	2011-01-19 17:47:10.444004409 +0100
> +++ qemu/hmp-commands.hx	2011-01-19 17:49:51.673254095 +0100
> @@ -53,6 +53,25 @@ Quit the emulator.
>  ETEXI
>  
>      {
> +        .name       = "resize",
> +        .args_type  = "id:s,size:o",
> +        .params     = "device size",
> +        .help       = "resize a block image",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_resize,
> +    },
> +
> +STEXI
> +@item resize
> +@findex resize
> +Resize a block image while a guest is running.  Usually requires guest
> +action to see the updated size.  Resize to a lower size is supported,
> +but should be used with extreme caution.  Note that this command only
> +resizes image files, it can not resize block devices like LVM volumes.
> +ETEXI
> +
> +
> +    {
>          .name       = "eject",
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
[...]
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx	2011-01-19 17:47:10.478012371 +0100
> +++ qemu/qmp-commands.hx	2011-01-19 17:50:07.406016841 +0100
> @@ -601,6 +601,34 @@ Example:
>  -> { "execute": "netdev_del", "arguments": { "id": "netdev1" } }
>  <- { "return": {} }
>  
> +
> +EQMP
> +
> +    {
> +        .name       = "block_resize",
> +        .args_type  = "id:s,size:o",
> +        .params     = "id size",
> +        .help       = "resize a block image",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_resize,
> +    },
> +
> +SQMP
> +block_resize
> +------------
> +
> +Resize a block image while a guest is running.
> +
> +Arguments:
> +
> +- "id": the device's ID, must be unique (json-string)
> +- "size": new size
> +
> +Example:
> +
> +-> { "execute": "block_resize", "arguments": { "id": "scratch", "size": 1073741824 } }
> +<- { "return": {} }
> +
>  EQMP

eject, change, block_passwd, and others call the device name argument
"device" instead of "id".  In the interest of a consistent external API
it would be nice to use "device" for the block_resize command too.

Stefan

Re: [Qemu-devel] [PATCH 2/3] block: tell drivers about an image resize

[Stefan Hajnoczi]

On Wed, Jan 19, 2011 at 5:02 PM, Christoph Hellwig <hch@lst.de> wrote:
> Extend the change_cb callback with a reason argument, and use it
> to tell drivers about size changes.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c   2011-01-18 20:54:45.246021572 +0100
> +++ qemu/block.c        2011-01-18 20:56:54.117255612 +0100
> @@ -645,7 +645,7 @@ int bdrv_open(BlockDriverState *bs, cons
>         /* call the change callback */
>         bs->media_changed = 1;
>         if (bs->change_cb)
> -            bs->change_cb(bs->change_opaque);
> +            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);

Cool, this is much nicer than stashing away state like
bs->media_changed = 1.  I just took a peek to see if we can remove
that field completely but IDE seems to use it internally.

Stefan

[Qemu-devel] Re: [PATCH 0/3 v2] allow online resizing of block devices

[Christoph Hellwig]

On Wed, Jan 19, 2011 at 06:26:49PM +0100, Juan Quintela wrote:
> Does this handle the change of the rtc data?  I can't see how/where.
> (Not that I know if it is important at all until reboot).

It doesn't.

> cmos_init_hb() uses the size to guess the right geometry of the drives,
> if we change the size of the drive, should we change that one?

I don't think changing geometry on a live system is a good idea.  But
as mentioned at least for Linux guests there's no way to find out about
the new size of an IDE disk right now.  Although I might send a patch
to allow a manual rescan, similar to SCSI.

Re: [Qemu-devel] [PATCH 1/3] block: add resize monitor command

[Christoph Hellwig]

On Thu, Jan 20, 2011 at 08:56:07AM +0000, Stefan Hajnoczi wrote:
> eject, change, block_passwd, and others call the device name argument
> "device" instead of "id".  In the interest of a consistent external API
> it would be nice to use "device" for the block_resize command too.

Sure, I can do that with the next respin.

Re: [Qemu-devel] [PATCH 2/3] block: tell drivers about an image resize

[Christoph Hellwig]

On Thu, Jan 20, 2011 at 09:14:17AM +0000, Stefan Hajnoczi wrote:
> Cool, this is much nicer than stashing away state like
> bs->media_changed = 1.  I just took a peek to see if we can remove
> that field completely but IDE seems to use it internally.

I think we could get rid of it in generic code, but let's keep this
series simple.

Re: [Qemu-devel] [PATCH 2/3] block: tell drivers about an image resize

[Kevin Wolf]

Am 19.01.2011 18:02, schrieb Christoph Hellwig:
> Extend the change_cb callback with a reason argument, and use it
> to tell drivers about size changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c	2011-01-18 20:54:45.246021572 +0100
> +++ qemu/block.c	2011-01-18 20:56:54.117255612 +0100
> @@ -645,7 +645,7 @@ int bdrv_open(BlockDriverState *bs, cons
>          /* call the change callback */
>          bs->media_changed = 1;
>          if (bs->change_cb)
> -            bs->change_cb(bs->change_opaque);
> +            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);
>      }
>  
>      return 0;
> @@ -684,7 +684,7 @@ void bdrv_close(BlockDriverState *bs)
>          /* call the change callback */
>          bs->media_changed = 1;
>          if (bs->change_cb)
> -            bs->change_cb(bs->change_opaque);
> +            bs->change_cb(bs->change_opaque, CHANGE_MEDIA);
>      }
>  }
>  
> @@ -1135,6 +1135,8 @@ int bdrv_truncate(BlockDriverState *bs,
>      ret = drv->bdrv_truncate(bs, offset);
>      if (ret == 0) {
>          ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> +        if (bs->change_cb)
> +            bs->change_cb(bs->change_opaque, CHANGE_SIZE);

Braces are missing here.

Apart from that and Stefan's s/id/device/ the series looks good to me.

Kevin