Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Jeff Cody]

On Wed, Feb 24, 2016 at 08:28:20PM +0100, Peter Lieven wrote:
> Am 24.02.2016 um 14:40 schrieb Jeff Cody:
> > On Wed, Feb 24, 2016 at 02:07:18PM +0100, Kevin Wolf wrote:
> >> Am 24.02.2016 um 13:44 hat Peter Lieven geschrieben:
> >>> if the size is forced I would set the chs values to max. this way no
> >>> new creator String is needed and it is even backwards compatible. this
> >>> is what disk2vhd does.
> >> Does disk2vhd do it this way even if the size is smaller than the
> >> maximum that can be represented with CHS?
> >>
> > I don't know about disk2vhd, but I just created a 5G dynamic VHD
> > image on Hyper-V, and it produced:
> >
> > cyl: 10402, heads: 16, secs: 63
> > virtual size: 5.0G (5368709120 bytes)
> >
> > (the virtual size as calculated by CHS in that case would have been
> > 5368430592 bytes)
> >
> > I then tested the reverse - I modified qemu to create a VHD image with
> > 5G as the current_size, but maxed out CHS parameters.  I imported it
> > into Hyper-V, and it worked fine - just recognized as a 5G disk with
> > 5368709120 bytes.
> 
> So the idea to set CHS to MAX is not that bad.
> 

I agree.  I just did a test with disk2vhd, against an empty (but
formatted) 7.5G usb drive.  Here is what it created:

# ./qemu-img info /mnt/nfs-2/tmp/WINtsts2.VHD
cyls: 65535, heads: 16, sectors: 255
image: /mnt/nfs-2/tmp/WINtsts2.VHD
file format: vpc
virtual size: 7.5G (8095006720 bytes)

I really like how one company has 3 different tools that all handle
this differently.

> >
> > But with all that, it seems like it may be better to mimic the Hyper-V
> > behavior, and use a new creator app string, with the normal CHS
> > values.
> 
> But this means that it is not backwards compatible. Maxing out CHS
> when forcing the size would mean all old qemu version would look
> at current size and not CHS.
>

That is a valid concern.  So, how about this:

With 'force_size' during image create, set the CHS parameters to MAX
(as d2v does), but also set the creator app to 'qem2'.  This at least
gives other software a chance to learn that new creator app and handle
it differently, if needed (and it shouldn't hurt anything, and will
still be backwards compatible).


> Might it be that Hyper-V and others use CHS when an ATA disk is emulated
> and the cur_size otherwise? I think this is what virtualbox does.

I tested the Hyper-V created image under a Linux VM in Hyper-V.  The
VM is using IDE controllers, and the image size was the current_size,
not the CHS calculation.

Jeff

[Qemu-block] [PULL 15/23] virtio-blk: do not use vring in dataplane

[Michael S. Tsirkin]

From: Paolo Bonzini 

Signed-off-by: Paolo Bonzini 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Reviewed-by: Fam Zheng 
Acked-by: Stefan Hajnoczi 
---
 hw/block/dataplane/virtio-blk.h |   1 +
 include/hw/virtio/virtio-blk.h  |   3 --
 hw/block/dataplane/virtio-blk.c | 112 +---
 hw/block/virtio-blk.c   |  49 +++---
 4 files changed, 19 insertions(+), 146 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
index c88d40e..0714c11 100644
--- a/hw/block/dataplane/virtio-blk.h
+++ b/hw/block/dataplane/virtio-blk.h
@@ -26,5 +26,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_drain(VirtIOBlockDataPlane *s);
+void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s);
 
 #endif /* HW_DATAPLANE_VIRTIO_BLK_H */
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 781969d..ae84d92 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,9 +53,6 @@ typedef struct VirtIOBlock {
 unsigned short sector_mask;
 bool original_wce;
 VMChangeStateEntry *change;
-/* Function to push to vq and notify guest */
-void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
-Notifier migration_state_notifier;
 bool dataplane_started;
 struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index cc521c1..36f3d2b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -18,8 +18,6 @@
 #include "qemu/thread.h"
 #include "qemu/error-report.h"
 #include "hw/virtio/virtio-access.h"
-#include "hw/virtio/dataplane/vring.h"
-#include "hw/virtio/dataplane/vring-accessors.h"
 #include "sysemu/block-backend.h"
 #include "hw/virtio/virtio-blk.h"
 #include "virtio-blk.h"
@@ -35,7 +33,7 @@ struct VirtIOBlockDataPlane {
 VirtIOBlkConf *conf;
 
 VirtIODevice *vdev;
-Vring vring;/* virtqueue vring */
+VirtQueue *vq;  /* virtqueue vring */
 EventNotifier *guest_notifier;  /* irq */
 QEMUBH *bh; /* bh for guest notification */
 
@@ -48,94 +46,26 @@ struct VirtIOBlockDataPlane {
  */
 IOThread *iothread;
 AioContext *ctx;
-EventNotifier host_notifier;/* doorbell */
 
 /* Operation blocker on BDS */
 Error *blocker;
-void (*saved_complete_request)(struct VirtIOBlockReq *req,
-   unsigned char status);
 };
 
 /* Raise an interrupt to signal guest, if necessary */
-static void notify_guest(VirtIOBlockDataPlane *s)
+void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s)
 {
-if (!vring_should_notify(s->vdev, >vring)) {
-return;
-}
-
-event_notifier_set(s->guest_notifier);
+qemu_bh_schedule(s->bh);
 }
 
 static void notify_guest_bh(void *opaque)
 {
 VirtIOBlockDataPlane *s = opaque;
 
-notify_guest(s);
-}
-
-static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
-{
-VirtIOBlockDataPlane *s = req->dev->dataplane;
-stb_p(>in->status, status);
-
-vring_push(s->vdev, >dev->dataplane->vring, >elem, req->in_len);
-
-/* Suppress notification to guest by BH and its scheduled
- * flag because requests are completed as a batch after io
- * plug & unplug is introduced, and the BH can still be
- * executed in dataplane aio context even after it is
- * stopped, so needn't worry about notification loss with BH.
- */
-qemu_bh_schedule(s->bh);
-}
-
-static void handle_notify(EventNotifier *e)
-{
-VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
-   host_notifier);
-VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
-
-event_notifier_test_and_clear(>host_notifier);
-blk_io_plug(s->conf->conf.blk);
-for (;;) {
-MultiReqBuffer mrb = {};
-
-/* Disable guest->host notifies to avoid unnecessary vmexits */
-vring_disable_notification(s->vdev, >vring);
-
-for (;;) {
-VirtIOBlockReq *req = vring_pop(s->vdev, >vring,
-sizeof(VirtIOBlockReq));
-
-if (req == NULL) {
-break; /* no more requests */
-}
-
-virtio_blk_init_request(vblk, req);
-trace_virtio_blk_data_plane_process_request(s, req->elem.out_num,
-req->elem.in_num,
-req->elem.index);
-
-virtio_blk_handle_request(req, );
-}
-
-if 

[Qemu-block] [PULL 14/23] virtio-blk: fix "disabled data plane" mode

[Michael S. Tsirkin]

From: Paolo Bonzini 

In disabled mode, virtio-blk dataplane seems to be enabled, but flow
actually goes through the normal virtio path.  This patch simplifies a bit
the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
might be called even if s->dataplane is not NULL.

This is a bit tricky, because the current check for s->dataplane will
always trigger, causing a continuous stream of calls to
virtio_blk_data_plane_start.  Unfortunately, these calls will not
do anything.  To fix this, set the "started" flag even in disabled
mode, and skip virtio_blk_data_plane_start if the started flag is true.
The resulting changes also prepare the code for the next patch, were
virtio-blk dataplane will reuse the same virtio_blk_handle_output function
as "regular" virtio-blk.

Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
to move s->dataplane->started inside struct VirtIOBlock.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Reviewed-by: Cornelia Huck 
Reviewed-by: Fam Zheng 
Acked-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio-blk.h  |  1 +
 hw/block/dataplane/virtio-blk.c | 21 +
 hw/block/virtio-blk.c   |  2 +-
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 199bb0e..781969d 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
 /* Function to push to vq and notify guest */
 void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
 Notifier migration_state_notifier;
+bool dataplane_started;
 struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
 
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 03b81bc..cc521c1 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -28,7 +28,6 @@
 #include "qom/object_interfaces.h"
 
 struct VirtIOBlockDataPlane {
-bool started;
 bool starting;
 bool stopping;
 bool disabled;
@@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 VirtQueue *vq;
 int r;
 
-if (s->started || s->disabled) {
-return;
-}
-
-if (s->starting) {
+if (vblk->dataplane_started || s->starting) {
 return;
 }
 
@@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 vblk->complete_request = complete_request_vring;
 
 s->starting = false;
-s->started = true;
+vblk->dataplane_started = true;
 trace_virtio_blk_data_plane_start(s);
 
 blk_set_aio_context(s->conf->conf.blk, s->ctx);
@@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
 vring_teardown(>vring, s->vdev, 0);
-s->disabled = true;
   fail_vring:
+s->disabled = true;
 s->starting = false;
+vblk->dataplane_started = true;
 }
 
 /* Context: QEMU global mutex held */
@@ -331,13 +327,14 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
 
+if (!vblk->dataplane_started || s->stopping) {
+return;
+}
 
 /* Better luck next time. */
 if (s->disabled) {
 s->disabled = false;
-return;
-}
-if (!s->started || s->stopping) {
+vblk->dataplane_started = false;
 return;
 }
 s->stopping = true;
@@ -364,6 +361,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 /* Clean up guest notifier (irq) */
 k->set_guest_notifiers(qbus->parent, 1, false);
 
-s->started = false;
+vblk->dataplane_started = false;
 s->stopping = false;
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c427698..e04c8f5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -589,7 +589,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, 
VirtQueue *vq)
 /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
  * dataplane here instead of waiting for .set_status().
  */
-if (s->dataplane) {
+if (s->dataplane && !s->dataplane_started) {
 virtio_blk_data_plane_start(s->dataplane);
 return;
 }
-- 
MST

[Qemu-block] [PULL 11/23] vring: make vring_enable_notification return void

[Michael S. Tsirkin]

From: Paolo Bonzini 

Make the API more similar to the regular virtqueue API.  This will
help when modifying the code to not use vring.c anymore.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Acked-by: Cornelia Huck 
Reviewed-by: Fam Zheng 
Acked-by: Stefan Hajnoczi 
---
 include/hw/virtio/dataplane/vring.h | 2 +-
 hw/block/dataplane/virtio-blk.c | 3 ++-
 hw/virtio/dataplane/vring.c | 3 +--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/virtio/dataplane/vring.h 
b/include/hw/virtio/dataplane/vring.h
index e80985e..e1c2a65 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -42,7 +42,7 @@ static inline void vring_set_broken(Vring *vring)
 bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
 void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
 void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
-bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
+void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
 void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
 void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0d99781..03b81bc 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -128,7 +128,8 @@ static void handle_notify(EventNotifier *e)
 /* Re-enable guest->host notifies and stop processing the vring.
  * But if the guest has snuck in more descriptors, keep processing.
  */
-if (vring_enable_notification(s->vdev, >vring)) {
+vring_enable_notification(s->vdev, >vring);
+if (!vring_more_avail(s->vdev, >vring)) {
 break;
 }
 } else { /* fatal error */
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 4308d9f..157e8b8 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -175,7 +175,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring 
*vring)
  *
  * Return true if the vring is empty, false if there are more requests.
  */
-bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
+void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
 {
 if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
 vring_avail_event(>vr) = vring->vr.avail->idx;
@@ -183,7 +183,6 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring 
*vring)
 vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
 }
 smp_mb(); /* ensure update is seen before reading avail_idx */
-return !vring_more_avail(vdev, vring);
 }
 
 /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
-- 
MST

[Qemu-block] [PULL 10/23] block-migration: acquire AioContext as necessary

[Michael S. Tsirkin]

From: Paolo Bonzini 

This is needed because dataplane will run during block migration as well.

The block device migration code is quite liberal in taking the iothread
mutex.  For simplicity, keep it the same way, even though one could
actually choose between the BQL (for regular BlockDriverStates) and
the AioContext (for dataplane BlockDriverStates).  When the block layer
is made fully thread safe, aio_context_acquire shall go away altogether.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Acked-by: Stefan Hajnoczi 
Reviewed-by: Fam Zheng 
---
 migration/block.c | 65 ---
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 3a8330a..72883d7 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -54,17 +54,25 @@ typedef struct BlkMigDevState {
 int shared_base;
 int64_t total_sectors;
 QSIMPLEQ_ENTRY(BlkMigDevState) entry;
+Error *blocker;
 
 /* Only used by migration thread.  Does not need a lock.  */
 int bulk_completed;
 int64_t cur_sector;
 int64_t cur_dirty;
 
-/* Protected by block migration lock.  */
+/* Data in the aio_bitmap is protected by block migration lock.
+ * Allocation and free happen during setup and cleanup respectively.
+ */
 unsigned long *aio_bitmap;
+
+/* Protected by block migration lock.  */
 int64_t completed_sectors;
+
+/* During migration this is protected by iothread lock / AioContext.
+ * Allocation and free happen during setup and cleanup respectively.
+ */
 BdrvDirtyBitmap *dirty_bitmap;
-Error *blocker;
 } BlkMigDevState;
 
 typedef struct BlkMigBlock {
@@ -100,7 +108,7 @@ typedef struct BlkMigState {
 int prev_progress;
 int bulk_completed;
 
-/* Lock must be taken _inside_ the iothread lock.  */
+/* Lock must be taken _inside_ the iothread lock and any AioContexts.  */
 QemuMutex lock;
 } BlkMigState;
 
@@ -264,11 +272,13 @@ static int mig_save_device_bulk(QEMUFile *f, 
BlkMigDevState *bmds)
 
 if (bmds->shared_base) {
 qemu_mutex_lock_iothread();
+aio_context_acquire(bdrv_get_aio_context(bs));
 while (cur_sector < total_sectors &&
!bdrv_is_allocated(bs, cur_sector, MAX_IS_ALLOCATED_SEARCH,
   _sectors)) {
 cur_sector += nr_sectors;
 }
+aio_context_release(bdrv_get_aio_context(bs));
 qemu_mutex_unlock_iothread();
 }
 
@@ -302,11 +312,21 @@ static int mig_save_device_bulk(QEMUFile *f, 
BlkMigDevState *bmds)
 block_mig_state.submitted++;
 blk_mig_unlock();
 
+/* We do not know if bs is under the main thread (and thus does
+ * not acquire the AioContext when doing AIO) or rather under
+ * dataplane.  Thus acquire both the iothread mutex and the
+ * AioContext.
+ *
+ * This is ugly and will disappear when we make bdrv_* thread-safe,
+ * without the need to acquire the AioContext.
+ */
 qemu_mutex_lock_iothread();
+aio_context_acquire(bdrv_get_aio_context(bmds->bs));
 blk->aiocb = bdrv_aio_readv(bs, cur_sector, >qiov,
 nr_sectors, blk_mig_read_cb, blk);
 
 bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
+aio_context_release(bdrv_get_aio_context(bmds->bs));
 qemu_mutex_unlock_iothread();
 
 bmds->cur_sector = cur_sector + nr_sectors;
@@ -321,8 +341,10 @@ static int set_dirty_tracking(void)
 int ret;
 
 QSIMPLEQ_FOREACH(bmds, _mig_state.bmds_list, entry) {
+aio_context_acquire(bdrv_get_aio_context(bmds->bs));
 bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
   NULL, NULL);
+aio_context_release(bdrv_get_aio_context(bmds->bs));
 if (!bmds->dirty_bitmap) {
 ret = -errno;
 goto fail;
@@ -333,18 +355,24 @@ static int set_dirty_tracking(void)
 fail:
 QSIMPLEQ_FOREACH(bmds, _mig_state.bmds_list, entry) {
 if (bmds->dirty_bitmap) {
+aio_context_acquire(bdrv_get_aio_context(bmds->bs));
 bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
+aio_context_release(bdrv_get_aio_context(bmds->bs));
 }
 }
 return ret;
 }
 
+/* Called with iothread lock taken.  */
+
 static void unset_dirty_tracking(void)
 {
 BlkMigDevState *bmds;
 
 QSIMPLEQ_FOREACH(bmds, _mig_state.bmds_list, entry) {
+aio_context_acquire(bdrv_get_aio_context(bmds->bs));
 bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
+aio_context_release(bdrv_get_aio_context(bmds->bs));
 }
 }
 
@@ -444,7 +472,7 @@ static void blk_mig_reset_dirty_cursor(void)
 }
 }
 
-/* Called with iothread 

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Peter Lieven]

Am 24.02.2016 um 14:07 schrieb Kevin Wolf:
> Am 24.02.2016 um 13:44 hat Peter Lieven geschrieben:
>> if the size is forced I would set the chs values to max. this way no
>> new creator String is needed and it is even backwards compatible. this
>> is what disk2vhd does.
> Does disk2vhd do it this way even if the size is smaller than the
> maximum that can be represented with CHS?

Afaik it always sets it to MAX. This is why we first introduced a check
for the d2v creator in Qemu because all VHD Images created by
disk2vhd where 127GB regardless of their size.

Peter

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Peter Lieven]

Am 24.02.2016 um 14:40 schrieb Jeff Cody:
> On Wed, Feb 24, 2016 at 02:07:18PM +0100, Kevin Wolf wrote:
>> Am 24.02.2016 um 13:44 hat Peter Lieven geschrieben:
>>> if the size is forced I would set the chs values to max. this way no
>>> new creator String is needed and it is even backwards compatible. this
>>> is what disk2vhd does.
>> Does disk2vhd do it this way even if the size is smaller than the
>> maximum that can be represented with CHS?
>>
> I don't know about disk2vhd, but I just created a 5G dynamic VHD
> image on Hyper-V, and it produced:
>
> cyl: 10402, heads: 16, secs: 63
> virtual size: 5.0G (5368709120 bytes)
>
> (the virtual size as calculated by CHS in that case would have been
> 5368430592 bytes)
>
> I then tested the reverse - I modified qemu to create a VHD image with
> 5G as the current_size, but maxed out CHS parameters.  I imported it
> into Hyper-V, and it worked fine - just recognized as a 5G disk with
> 5368709120 bytes.

So the idea to set CHS to MAX is not that bad.

>
> But with all that, it seems like it may be better to mimic the Hyper-V
> behavior, and use a new creator app string, with the normal CHS
> values.

But this means that it is not backwards compatible. Maxing out CHS
when forcing the size would mean all old qemu version would look
at current size and not CHS.

Might it be that Hyper-V and others use CHS when an ATA disk is emulated
and the cur_size otherwise? I think this is what virtualbox does.

Peter

Re: [Qemu-block] [PATCH 1/2] hmp: 'drive_add -n' for creating a node without BB

[Kevin Wolf]

Am 24.02.2016 um 18:50 hat Max Reitz geschrieben:
> On 23.02.2016 18:16, Kevin Wolf wrote:
> > This patch adds an option to the drive_add HMP command to create only a
> > BlockDriverState without a BlockBackend on top.
> > 
> > The motivation for this is that libvirt needs to specify options to a
> > migration target (specifically, detect-zeroes). drive-mirror doesn't
> > allow specifying options, and the proper way to do this is to create the
> > target BDS separately with blockdev-add (where you can specify options)
> > and then use blockdev-mirror to that BDS.
> > 
> > However, libvirt can't use blockdev-add as long as it is still
> > experimental, and we're expecting that it will still take some time, so
> > we need to resort to drive_add.
> > 
> > The problem with drive_add is that so far it always created a BB, and
> > BDSes with a BB can't be used as a mirroring target as long as we don't
> > support multiple BBs per BDS - and while we're working towards that
> > goal, it's another thing that will still take some time.
> > 
> > So to achieve the goal, the simplest solution to provide the
> > functionality now without adding one-off options to the mirror QMP
> > commands is to extend drive_add to create nodes without BBs.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  blockdev.c| 30 ++
> >  device-hotplug.c  |  7 +++
> >  hmp-commands.hx   |  4 ++--
> >  include/block/block_int.h |  2 ++
> >  4 files changed, 41 insertions(+), 2 deletions(-)
> > 
> 
> Patch looks good to me (well, except for it being a pity we have to fall
> back on this HMP command), I only have a minor suggestion:
> 
> [...]
> 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index bb52e4d..3b44e52 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1200,8 +1200,8 @@ ETEXI
> >  
> >  {
> >  .name   = "drive_add",
> > -.args_type  = "pci_addr:s,opts:s",
> > -.params = "[[:]:]\n"
> > +.args_type  = "node:-n,pci_addr:s,opts:s",
> > +.params = "[-n] [[:]:]\n"
> >"[file=file][,if=type][,bus=n]\n"
> >"[,unit=m][,media=d][,index=i]\n"
> >"[,cyls=c,heads=h,secs=s[,trans=t]]\n"
> 
> The description reads:
> 
> > Add drive to PCI storage controller.
> 
> Maybe this should be extended now?

It was already wrong before this patch, but I guess I could just add
another patch to the series anyway.

Kevin


pgpjqnCBJtJl_.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH 2/2] hmp: Extend drive_del to delete nodes without BB

[Kevin Wolf]

Am 24.02.2016 um 18:54 hat Max Reitz geschrieben:
> On 23.02.2016 18:16, Kevin Wolf wrote:
> > Now that we can use drive_add to create new nodes without a BB, we also
> > want to be able to delete such nodes again.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  blockdev.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 3f46bc1..b76b6cd 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2816,6 +2816,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >  AioContext *aio_context;
> >  Error *local_err = NULL;
> >  
> > +bs = bdrv_find_node(id);
> > +if (bs) {
> > +qmp_x_blockdev_del(false, NULL, true, id, _err);
> > +if (local_err) {
> > +error_report_err(local_err);
> > +}
> > +return;
> > +}
> > +
> >  blk = blk_by_name(id);
> >  if (!blk) {
> >  error_report("Device '%s' not found", id);
> > 
> 
> It's a bit strange to require the user to specify the node name using
> "node-name" for drive_add, but the to use "id" in drive_del; especially
> because x-blockdev-del uses "node-name", too.

Not sure I understand. For the user of drive_del that's simply a
positional parameter, so they use neither "id" nor "node-name". Am I
missing something?

Kevin

> Up to your discretion.
> 
> Reviewed-by: Max Reitz 
> 




pgpGKTSewcTW7.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH 2/2] hmp: Extend drive_del to delete nodes without BB

[Max Reitz]

On 23.02.2016 18:16, Kevin Wolf wrote:
> Now that we can use drive_add to create new nodes without a BB, we also
> want to be able to delete such nodes again.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockdev.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3f46bc1..b76b6cd 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2816,6 +2816,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>  AioContext *aio_context;
>  Error *local_err = NULL;
>  
> +bs = bdrv_find_node(id);
> +if (bs) {
> +qmp_x_blockdev_del(false, NULL, true, id, _err);
> +if (local_err) {
> +error_report_err(local_err);
> +}
> +return;
> +}
> +
>  blk = blk_by_name(id);
>  if (!blk) {
>  error_report("Device '%s' not found", id);
> 

It's a bit strange to require the user to specify the node name using
"node-name" for drive_add, but the to use "id" in drive_del; especially
because x-blockdev-del uses "node-name", too.

Up to your discretion.

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] hmp: 'drive_add -n' for creating a node without BB

[Max Reitz]

On 23.02.2016 18:16, Kevin Wolf wrote:
> This patch adds an option to the drive_add HMP command to create only a
> BlockDriverState without a BlockBackend on top.
> 
> The motivation for this is that libvirt needs to specify options to a
> migration target (specifically, detect-zeroes). drive-mirror doesn't
> allow specifying options, and the proper way to do this is to create the
> target BDS separately with blockdev-add (where you can specify options)
> and then use blockdev-mirror to that BDS.
> 
> However, libvirt can't use blockdev-add as long as it is still
> experimental, and we're expecting that it will still take some time, so
> we need to resort to drive_add.
> 
> The problem with drive_add is that so far it always created a BB, and
> BDSes with a BB can't be used as a mirroring target as long as we don't
> support multiple BBs per BDS - and while we're working towards that
> goal, it's another thing that will still take some time.
> 
> So to achieve the goal, the simplest solution to provide the
> functionality now without adding one-off options to the mirror QMP
> commands is to extend drive_add to create nodes without BBs.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockdev.c| 30 ++
>  device-hotplug.c  |  7 +++
>  hmp-commands.hx   |  4 ++--
>  include/block/block_int.h |  2 ++
>  4 files changed, 41 insertions(+), 2 deletions(-)
> 

Patch looks good to me (well, except for it being a pity we have to fall
back on this HMP command), I only have a minor suggestion:

[...]

> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb52e4d..3b44e52 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1200,8 +1200,8 @@ ETEXI
>  
>  {
>  .name   = "drive_add",
> -.args_type  = "pci_addr:s,opts:s",
> -.params = "[[:]:]\n"
> +.args_type  = "node:-n,pci_addr:s,opts:s",
> +.params = "[-n] [[:]:]\n"
>"[file=file][,if=type][,bus=n]\n"
>"[,unit=m][,media=d][,index=i]\n"
>"[,cyls=c,heads=h,secs=s[,trans=t]]\n"

The description reads:

> Add drive to PCI storage controller.

Maybe this should be extended now?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/3] block/backup: avoid copying less than full target clusters

[Eric Blake]

On 02/23/2016 05:52 PM, John Snow wrote:

> "Couldn't determine the cluster size of the target image, which has no
> backing file" is more correct. The problem is not why we couldn't
> determine it, but instead that we were unable to AND there is no backing
> file, so our inability to determine it is a problem.
> 
> What's our policy on error message strings that eclipse 80 columns? I
> was told once (at gunpoint; I live in the USA) to never split up long
> strings because it makes them harder to grep for.

I don't mind error messages longer than 80 columns; and I also don't
mind string concatenation to fit the source of the message into
reasonable lengths (when I grep for a message, I just grep for the first
few words, not the whole message, precisely so that line wraps later in
the message don't cause me grief).  There's varying opinions from other
contributors on whether an error message can be split, but I don't see
anything in HACKING that says which style must prevail.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 4/4] block/vpc: add tests for image creation force_size parameter

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/146 | 52 ++
 tests/qemu-iotests/146.out | 32 
 2 files changed, 84 insertions(+)

diff --git a/tests/qemu-iotests/146 b/tests/qemu-iotests/146
index 14579ec..6603c61 100755
--- a/tests/qemu-iotests/146
+++ b/tests/qemu-iotests/146
@@ -87,6 +87,58 @@ echo
 
 ${QEMU_IO} -c "open -o driver=vpc,force_size_calc=chs ${TEST_IMG}" -c 'map'
 
+_cleanup_test_img
+
+echo
+echo === Testing Image create, default ===
+echo
+
+TEST_IMG="${TEST_DIR}/vpc-create-test.vpc"
+
+_make_test_img 4G
+
+echo
+echo === Read created image, default opts 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc ${TEST_IMG}" -c 'map'
+
+echo
+echo === Read created image, force_size_calc=chs 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=chs ${TEST_IMG}" -c 'map'
+
+echo
+echo === Read created image, force_size_calc=current_size 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=current_size ${TEST_IMG}" -c 
'map'
+
+echo
+echo === Testing Image create, force_size ===
+echo
+
+_make_test_img -o force_size 4G
+
+echo
+echo === Read created image, default opts 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc ${TEST_IMG}" -c 'map'
+
+echo
+echo === Read created image, force_size_calc=chs 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=chs ${TEST_IMG}" -c 'map'
+
+echo
+echo === Read created image, force_size_calc=current_size 
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=current_size ${TEST_IMG}" -c 
'map'
+
 
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/146.out b/tests/qemu-iotests/146.out
index 47b2eb2..831bee8 100644
--- a/tests/qemu-iotests/146.out
+++ b/tests/qemu-iotests/146.out
@@ -23,4 +23,36 @@ QA output created by 146
 === Testing Hyper-V with chs force ===
 
 [   0]  266334240/ 266334240 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing Image create, default ===
+
+Formatting 'TEST_DIR/IMGFMT-create-test.IMGFMT', fmt=IMGFMT size=4294967296
+
+=== Read created image, default opts 
+
+[   0]  8389584/ 8389584 sectors not allocated at offset 0 
bytes (0)
+
+=== Read created image, force_size_calc=chs 
+
+[   0]  8389584/ 8389584 sectors not allocated at offset 0 
bytes (0)
+
+=== Read created image, force_size_calc=current_size 
+
+[   0]  8389584/ 8389584 sectors not allocated at offset 0 
bytes (0)
+
+=== Testing Image create, force_size ===
+
+Formatting 'TEST_DIR/IMGFMT-create-test.IMGFMT', fmt=IMGFMT size=4294967296 
force_size=on
+
+=== Read created image, default opts 
+
+[   0]  8388608/ 8388608 sectors not allocated at offset 0 
bytes (0)
+
+=== Read created image, force_size_calc=chs 
+
+[   0]  8389584/ 8389584 sectors not allocated at offset 0 
bytes (0)
+
+=== Read created image, force_size_calc=current_size 
+
+[   0]  8388608/ 8388608 sectors not allocated at offset 0 
bytes (0)
 *** done
-- 
1.9.3

[Qemu-block] [PATCH v2 0/4] VHD/VPC format compatibility

[Jeff Cody]

Changes from v2:

Patches 2,4: Just use qemu-io instead of a qemu instance (thanks Kevin, Max)
Patch 3: Use "qem2" for the creator app field, when forcing the size (thanks 
Kevin)

This is a long-standing issue that has come up many times, and has had
several different patches posted to fix it.  Virtual PC, and Hyper-V
calculate the disk geometry differently for VHD, leading to compatibility
issues.

We want to fix these compatibility problems, however we want to make sure
we do not break backwards compatibility.

There are two areas of compatibility addressed:

* Reading images (Patch 1)
* Creating images (Patch 3)

Please see the commit messages in Patches 1,3 for details.

Jeff Cody (4):
  block/vpc: choose size calculation method based on creator_app field
  block/vpc: tests for auto-detecting VPC and Hyper-V VHD images
  block/vpc: give option to force the current_size field in .bdrv_create
  block/vpc: add tests for image creation force_size parameter

 block/vpc.c| 111 ++--
 tests/qemu-iotests/146 | 145 +
 tests/qemu-iotests/146.out |  58 +
 tests/qemu-iotests/group   |   1 +
 .../sample_images/hyperv2012r2-dynamic.vhd.bz2 | Bin 0 -> 214 bytes
 .../sample_images/virtualpc-dynamic.vhd.bz2| Bin 0 -> 212 bytes
 6 files changed, 307 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/146
 create mode 100644 tests/qemu-iotests/146.out
 create mode 100644 
tests/qemu-iotests/sample_images/hyperv2012r2-dynamic.vhd.bz2
 create mode 100644 tests/qemu-iotests/sample_images/virtualpc-dynamic.vhd.bz2

-- 
1.9.3

[Qemu-block] [PATCH v2 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Jeff Cody]

This tests auto-detection, and overrides, of VHD image sizes created
by Virtual PC and Hyper-V.

This adds two sample images:

hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/146 |  93 +
 tests/qemu-iotests/146.out |  26 ++
 tests/qemu-iotests/group   |   1 +
 .../sample_images/hyperv2012r2-dynamic.vhd.bz2 | Bin 0 -> 214 bytes
 .../sample_images/virtualpc-dynamic.vhd.bz2| Bin 0 -> 212 bytes
 5 files changed, 120 insertions(+)
 create mode 100755 tests/qemu-iotests/146
 create mode 100644 tests/qemu-iotests/146.out
 create mode 100644 
tests/qemu-iotests/sample_images/hyperv2012r2-dynamic.vhd.bz2
 create mode 100644 tests/qemu-iotests/sample_images/virtualpc-dynamic.vhd.bz2

diff --git a/tests/qemu-iotests/146 b/tests/qemu-iotests/146
new file mode 100755
index 000..14579ec
--- /dev/null
+++ b/tests/qemu-iotests/146
@@ -0,0 +1,93 @@
+#!/bin/bash
+#
+# Test VHD image format creator detection and override
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt vpc
+_supported_proto file
+_supported_os Linux
+
+
+qemu_comm_method="monitor"
+silent=
+
+echo
+echo === Testing VPC Autodetect ===
+echo
+_use_sample_img virtualpc-dynamic.vhd.bz2
+
+${QEMU_IO} -c "open -o driver=vpc ${TEST_IMG}" -c 'map'
+
+echo
+echo === Testing VPC with current_size force ===
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=current_size ${TEST_IMG}" -c 
'map'
+
+echo
+echo === Testing VPC with chs force ===
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=chs ${TEST_IMG}" -c 'map'
+
+_cleanup_test_img
+
+echo
+echo === Testing Hyper-V Autodetect ===
+echo
+_use_sample_img hyperv2012r2-dynamic.vhd.bz2
+
+${QEMU_IO} -c "open -o driver=vpc ${TEST_IMG}" -c 'map'
+
+echo
+echo === Testing Hyper-V with current_size force ===
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=current_size ${TEST_IMG}" -c 
'map'
+
+echo
+echo === Testing Hyper-V with chs force ===
+echo
+
+${QEMU_IO} -c "open -o driver=vpc,force_size_calc=chs ${TEST_IMG}" -c 'map'
+
+
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/146.out b/tests/qemu-iotests/146.out
new file mode 100644
index 000..47b2eb2
--- /dev/null
+++ b/tests/qemu-iotests/146.out
@@ -0,0 +1,26 @@
+QA output created by 146
+
+=== Testing VPC Autodetect ===
+
+[   0]  266334240/ 266334240 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing VPC with current_size force ===
+
+[   0]  266338304/ 266338304 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing VPC with chs force ===
+
+[   0]  266334240/ 266334240 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing Hyper-V Autodetect ===
+
+[   0]  266338304/ 266338304 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing Hyper-V with current_size force ===
+
+[   0]  266338304/ 266338304 sectors not allocated at 
offset 0 bytes (0)
+
+=== Testing Hyper-V with chs force ===
+
+[   0]  266334240/ 266334240 sectors not allocated at 
offset 0 bytes (0)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 47fd40c..1211149 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -148,3 +148,4 @@
 143 auto quick
 144 rw auto quick
 145 auto quick
+146 auto quick
diff --git a/tests/qemu-iotests/sample_images/hyperv2012r2-dynamic.vhd.bz2 
b/tests/qemu-iotests/sample_images/hyperv2012r2-dynamic.vhd.bz2
new file mode 100644
index 
..bfeccf7b9f2596f119bb35d7e650a29f8e6f17ef
GIT binary patch
literal 214
zcmV;{04e`MT4*^jL0KkKS<7>@aR2~j|Ns3EPyxUK5D)+a0HCTX+~fcS06+jB01!Yx

[Qemu-block] [PATCH v2 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Jeff Cody]

When QEMU creates a VHD image, it goes by the original spec,
calculating the current_size based on the nearest CHS geometry (with an
exception for disks > 127GB).

Apparently, Azure will only allow images that are sized to the nearest
MB, and the current_size as calculated from CHS cannot guarantee that.

Allow QEMU to create images similar to how Hyper-V creates images, by
setting current_size to the specified virtual disk size.  This
introduces an option, force_size, to be passed to the vpc format during
image creation, e.g.:

qemu-img convert -f raw -o force_size -O vpc test.img test.vhd

When using the "force_size" option, the creator app field used by
QEMU will be "qem2" instead of "qemu", to indicate the difference.
In light of this, we also add parsing of the "qem2" field during
vpc_open.

Bug reference: https://bugs.launchpad.net/qemu/+bug/1490611

Signed-off-by: Jeff Cody 
---
 block/vpc.c | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 54a38a7..8f84b17 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -49,6 +49,8 @@ enum vhd_type {
 #define VHD_MAX_SECTORS   (65535LL * 255 * 255)
 #define VHD_MAX_GEOMETRY  (65535LL *  16 * 255)
 
+#define VPC_OPT_FORCE_SIZE "force_size"
+
 // always big-endian
 typedef struct vhd_footer {
 charcreator[8]; // "conectix"
@@ -288,6 +290,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  *  Known creator apps:
  *  'vpc '  :  CHS  Virtual PC (uses disk geometry)
  *  'qemu'  :  CHS  QEMU (uses disk geometry)
+ *  'qem2'  :  current_size QEMU (uses current_size)
  *  'win '  :  current_size Hyper-V
  *  'd2v '  :  current_size Disk2vhd
  *
@@ -296,6 +299,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  *  that have CHS geometry of the maximum size.
  */
 use_chs = (!!strncmp(footer->creator_app, "win ", 4) &&
+   !!strncmp(footer->creator_app, "qem2", 4) &&
!!strncmp(footer->creator_app, "d2v ", 4)) || s->force_use_chs;
 
 if (!use_chs || bs->total_sectors == VHD_MAX_GEOMETRY || s->force_use_sz) {
@@ -850,6 +854,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, 
Error **errp)
 int64_t total_size;
 int disk_type;
 int ret = -EIO;
+bool force_size;
 Error *local_err = NULL;
 BlockDriverState *bs = NULL;
 
@@ -870,6 +875,8 @@ static int vpc_create(const char *filename, QemuOpts *opts, 
Error **errp)
 disk_type = VHD_DYNAMIC;
 }
 
+force_size = qemu_opt_get_bool_del(opts, VPC_OPT_FORCE_SIZE, false);
+
 ret = bdrv_create_file(filename, opts, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
@@ -896,7 +903,8 @@ static int vpc_create(const char *filename, QemuOpts *opts, 
Error **errp)
 calculate_geometry(total_sectors + i, , , _per_cyl);
 }
 
-if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
+if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY ||
+force_size) {
 total_sectors = total_size / BDRV_SECTOR_SIZE;
 /* Allow a maximum disk size of approximately 2 TB */
 if (total_sectors > VHD_MAX_SECTORS) {
@@ -912,8 +920,11 @@ static int vpc_create(const char *filename, QemuOpts 
*opts, Error **errp)
 memset(buf, 0, 1024);
 
 memcpy(footer->creator, "conectix", 8);
-/* TODO Check if "qemu" creator_app is ok for VPC */
-memcpy(footer->creator_app, "qemu", 4);
+if (force_size) {
+memcpy(footer->creator_app, "qem2", 4);
+} else {
+memcpy(footer->creator_app, "qemu", 4);
+}
 memcpy(footer->creator_os, "Wi2k", 4);
 
 footer->features = cpu_to_be32(0x02);
@@ -994,6 +1005,13 @@ static QemuOptsList vpc_create_opts = {
 "Type of virtual hard disk format. Supported formats are "
 "{dynamic (default) | fixed} "
 },
+{
+.name = VPC_OPT_FORCE_SIZE,
+.type = QEMU_OPT_BOOL,
+.help = "Force disk size calculation to use the actual size "
+"specified, rather than using the nearest CHS-based "
+"calculation"
+},
 { /* end of list */ }
 }
 };
-- 
1.9.3

[Qemu-block] [PATCH v2 1/4] block/vpc: choose size calculation method based on creator_app field

[Jeff Cody]

The VHD file format is used by both Virtual PC, and Hyper-V.  However,
how the virtual disk size is calculated varies between the two.

Virtual PC uses the CHS drive parameters to determine the drive size.
Hyper-V, on the other hand, uses the current_size field in the footer
when determining image size.

This is problematic for a few reasons:

* VHD images from Hyper-V, using CHS calculations, will likely be
  trunctated.

* If we just rely always on current_size, then QEMU may have data
  compatibility issues with Virtual PC (we may write too much data
  into a VHD file to be used by Virtual PC, for instance).

* Existing VHD images created by QEMU have used the CHS calculations,
  except for images exceeding the 127GB limit.  We want to remain
  compatible with our own generated images.

Luckily, the VHD specification defines a 'Creator App' field, that is
used to indicate what software created the VHD file.

This patch does two things:

1. Uses the 'Creator App' field to help determine how to calculate
   size, and

2. Adds a VPC format option 'force_size_calc', so that the user can
   override the 'Creator App' auto-detection, in case there exist
   VHD images with unknown or contradictory 'Creator App' entries.

N.B.: We currently use the maximum CHS value as an indication to use the
current_size field.  This patch does not change that, even with the
'force_size_calc' option.

Signed-off-by: Jeff Cody 
---
 block/vpc.c | 87 +
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index f504536..54a38a7 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -128,6 +128,8 @@ typedef struct BDRVVPCState {
 
 uint32_t block_size;
 uint32_t bitmap_size;
+bool force_use_chs;
+bool force_use_sz;
 
 #ifdef CACHE
 uint8_t *pageentry_u8;
@@ -140,6 +142,22 @@ typedef struct BDRVVPCState {
 Error *migration_blocker;
 } BDRVVPCState;
 
+#define VPC_OPT_SIZE_CALC "force_size_calc"
+static QemuOptsList vpc_runtime_opts = {
+.name = "vpc-runtime-opts",
+.head = QTAILQ_HEAD_INITIALIZER(vpc_runtime_opts.head),
+.desc = {
+{
+.name = VPC_OPT_SIZE_CALC,
+.type = QEMU_OPT_STRING,
+.help = "Force disk size calculation to use either CHS geometry, "
+"or use the disk current_size specified in the VHD footer. 
"
+"{chs, current_size}"
+},
+{ /* end of list */ }
+}
+};
+
 static uint32_t vpc_checksum(uint8_t* buf, size_t size)
 {
 uint32_t res = 0;
@@ -159,6 +177,25 @@ static int vpc_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 return 0;
 }
 
+static void vpc_parse_options(BlockDriverState *bs, QemuOpts *opts,
+  Error **errp)
+{
+BDRVVPCState *s = bs->opaque;
+const char *size_calc;
+
+size_calc = qemu_opt_get(opts, VPC_OPT_SIZE_CALC);
+
+if (!size_calc) {
+   /* no override, use autodetect only */
+} else if (!strcmp(size_calc, "current_size")) {
+s->force_use_sz = true;
+} else if (!strcmp(size_calc, "chs")) {
+s->force_use_chs = true;
+} else {
+error_setg(errp, "Invalid size calculation mode: '%s'", size_calc);
+}
+}
+
 static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
@@ -166,6 +203,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 int i;
 VHDFooter *footer;
 VHDDynDiskHeader *dyndisk_header;
+QemuOpts *opts = NULL;
+Error *local_err = NULL;
+bool use_chs;
 uint8_t buf[HEADER_SIZE];
 uint32_t checksum;
 uint64_t computed_size;
@@ -173,6 +213,21 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 int disk_type = VHD_DYNAMIC;
 int ret;
 
+opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+vpc_parse_options(bs, opts, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+
 ret = bdrv_pread(bs->file->bs, 0, s->footer_buf, HEADER_SIZE);
 if (ret < 0) {
 goto fail;
@@ -218,12 +273,34 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 bs->total_sectors = (int64_t)
 be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
 
-/* Images that have exactly the maximum geometry are probably bigger and
- * would be truncated if we adhered to the geometry for them. Rely on
- * footer->current_size for them. */
-if (bs->total_sectors == VHD_MAX_GEOMETRY) {
+/* Microsoft Virtual PC and Microsoft Hyper-V produce and read
+ * VHD image sizes differently.  VPC will rely on CHS geometry,
+ 

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Max Reitz]

On 24.02.2016 16:40, Jeff Cody wrote:
> On Wed, Feb 24, 2016 at 11:23:29AM +0100, Kevin Wolf wrote:
>> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
>>> This tests auto-detection, and overrides, of VHD image sizes created
>>> by Virtual PC and Hyper-V.
>>>
>>> This adds two sample images:
>>>
>>> hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
>>> virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
>>>
>>> Signed-off-by: Jeff Cody 
>>
>>> +echo
>>> +echo === Testing VPC Autodetect ===
>>> +echo
>>> +_use_sample_img virtualpc-dynamic.vhd.bz2
>>> +
>>> +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
>>> +h1=$QEMU_HANDLE
>>> +
>>> +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
>>> +_send_qemu_cmd $h1 'quit' ""
>>
>> I would avoid the big hammer of starting qemu processes when qemu-io can
>> test the same:
>>
>> $QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"
>>
>> Kevin
> 
> The reason I used a qemu process before, was difficulty passing in the
> drive options to qemu-io.  I futzed around a bit with the new
> --image-opts, but I hadn't tried "file=" in the open command with -o,
> so I felt a bit silly after your email.
> 
> However, this doesn't seem to work, now that I've tried it.  Is it
> broken, or am I doing something wrong?  Here is what I get:
> 
> # ./qemu-io -c "open -o file=/tmp/hyperv2012r2-dynamic.vhd,format=vpc" -c 
> "map"
> can't open: Cannot find device=/tmp/hyperv2012r2-dynamic.vhd nor 
> node_name=/tmp/hyperv2012r2-dynamic.vhd
> 
> Technically, I could just rely on image format autodetection since the
> current test images are dynamic and not fixed, and use -o to pass the
> vpc specific options.  I just hate to rely on autodetection anymore.

And I completely skipped this block...

$QEMU_IO resolves to _qemu_io_wrapper; this function will eventually
invoke "$QEMU_IO_PROG" $QEMU_IO_OPTIONS; and $QEMU_IO_OPTIONS resolves
to "-f $IMGFMT --cache $CACHEMODE". Therefore, you don't need to worry
about autodetection anyway.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Jeff Cody]

On Wed, Feb 24, 2016 at 04:44:21PM +0100, Max Reitz wrote:
> On 24.02.2016 16:40, Jeff Cody wrote:
> > On Wed, Feb 24, 2016 at 11:23:29AM +0100, Kevin Wolf wrote:
> >> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> >>> This tests auto-detection, and overrides, of VHD image sizes created
> >>> by Virtual PC and Hyper-V.
> >>>
> >>> This adds two sample images:
> >>>
> >>> hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
> >>> virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
> >>>
> >>> Signed-off-by: Jeff Cody 
> >>
> >>> +echo
> >>> +echo === Testing VPC Autodetect ===
> >>> +echo
> >>> +_use_sample_img virtualpc-dynamic.vhd.bz2
> >>> +
> >>> +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
> >>> +h1=$QEMU_HANDLE
> >>> +
> >>> +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
> >>> +_send_qemu_cmd $h1 'quit' ""
> >>
> >> I would avoid the big hammer of starting qemu processes when qemu-io can
> >> test the same:
> >>
> >> $QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"
> >>
> >> Kevin
> > 
> > The reason I used a qemu process before, was difficulty passing in the
> > drive options to qemu-io.  I futzed around a bit with the new
> > --image-opts, but I hadn't tried "file=" in the open command with -o,
> > so I felt a bit silly after your email.
> > 
> > However, this doesn't seem to work, now that I've tried it.  Is it
> > broken, or am I doing something wrong?  Here is what I get:
> > 
> > # ./qemu-io -c "open -o file=/tmp/hyperv2012r2-dynamic.vhd,format=vpc" -c 
> > "map"
> > can't open: Cannot find device=/tmp/hyperv2012r2-dynamic.vhd nor 
> > node_name=/tmp/hyperv2012r2-dynamic.vhd
> 
> Try:
> 
> $QEMU_IO -c "open -o driver=vpc ${TEST_IMG}"
> 
> Max

Thanks!

> 
> > 
> > Technically, I could just rely on image format autodetection since the
> > current test images are dynamic and not fixed, and use -o to pass the
> > vpc specific options.  I just hate to rely on autodetection anymore.
> > 
> > -Jeff
> > 
> 
> 

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Max Reitz]

On 24.02.2016 16:40, Jeff Cody wrote:
> On Wed, Feb 24, 2016 at 11:23:29AM +0100, Kevin Wolf wrote:
>> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
>>> This tests auto-detection, and overrides, of VHD image sizes created
>>> by Virtual PC and Hyper-V.
>>>
>>> This adds two sample images:
>>>
>>> hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
>>> virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
>>>
>>> Signed-off-by: Jeff Cody 
>>
>>> +echo
>>> +echo === Testing VPC Autodetect ===
>>> +echo
>>> +_use_sample_img virtualpc-dynamic.vhd.bz2
>>> +
>>> +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
>>> +h1=$QEMU_HANDLE
>>> +
>>> +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
>>> +_send_qemu_cmd $h1 'quit' ""
>>
>> I would avoid the big hammer of starting qemu processes when qemu-io can
>> test the same:
>>
>> $QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"
>>
>> Kevin
> 
> The reason I used a qemu process before, was difficulty passing in the
> drive options to qemu-io.  I futzed around a bit with the new
> --image-opts, but I hadn't tried "file=" in the open command with -o,
> so I felt a bit silly after your email.
> 
> However, this doesn't seem to work, now that I've tried it.  Is it
> broken, or am I doing something wrong?  Here is what I get:
> 
> # ./qemu-io -c "open -o file=/tmp/hyperv2012r2-dynamic.vhd,format=vpc" -c 
> "map"
> can't open: Cannot find device=/tmp/hyperv2012r2-dynamic.vhd nor 
> node_name=/tmp/hyperv2012r2-dynamic.vhd

Try:

$QEMU_IO -c "open -o driver=vpc ${TEST_IMG}"

Max

> 
> Technically, I could just rely on image format autodetection since the
> current test images are dynamic and not fixed, and use -o to pass the
> vpc specific options.  I just hate to rely on autodetection anymore.
> 
> -Jeff
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Jeff Cody]

On Wed, Feb 24, 2016 at 11:23:29AM +0100, Kevin Wolf wrote:
> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> > This tests auto-detection, and overrides, of VHD image sizes created
> > by Virtual PC and Hyper-V.
> > 
> > This adds two sample images:
> > 
> > hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
> > virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
> > 
> > Signed-off-by: Jeff Cody 
> 
> > +echo
> > +echo === Testing VPC Autodetect ===
> > +echo
> > +_use_sample_img virtualpc-dynamic.vhd.bz2
> > +
> > +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
> > +h1=$QEMU_HANDLE
> > +
> > +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
> > +_send_qemu_cmd $h1 'quit' ""
> 
> I would avoid the big hammer of starting qemu processes when qemu-io can
> test the same:
> 
> $QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"
> 
> Kevin

The reason I used a qemu process before, was difficulty passing in the
drive options to qemu-io.  I futzed around a bit with the new
--image-opts, but I hadn't tried "file=" in the open command with -o,
so I felt a bit silly after your email.

However, this doesn't seem to work, now that I've tried it.  Is it
broken, or am I doing something wrong?  Here is what I get:

# ./qemu-io -c "open -o file=/tmp/hyperv2012r2-dynamic.vhd,format=vpc" -c "map"
can't open: Cannot find device=/tmp/hyperv2012r2-dynamic.vhd nor 
node_name=/tmp/hyperv2012r2-dynamic.vhd

Technically, I could just rely on image format autodetection since the
current test images are dynamic and not fixed, and use -o to pass the
vpc specific options.  I just hate to rely on autodetection anymore.

-Jeff

Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

On 24.02.2016 10:28, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 23.02.2016 10:48, Markus Armbruster wrote:

[...]

>>> Can you explain the *actual* difference between blk_backends and
>>> monitor_block_backends?  "Actual" in the sense of difference in contents
>>> over time.
>>
>> First difference: The name. That's enough reason for me.
>>
>> Second difference: blk_new() adds any BB to blk_backends automatically.
>> It doesn't do so for monitor_block_backends.
>>
>> Third difference: Often the monitor doesn't take care of signalling that
>> it's releasing its reference, only sometimes (where "sometimes" means
>> every time blk_hide...() is called). blk_delete() will instead
>> automatically remove it from blk_backends, because it's assuming that
>> the last reference had been held by the monitor.
> 
> The reference held in trust for lookup by name exists as long as lookup
> by name is permitted.
> 
> Therefore, it goes away when the BB dies or when it loses its name.

And I'd like it to be more explicit than this. A BB should simply never
have a name anymore when it's deleted. The current life cycle has a
short time where a named BB can have a refcount of 0.

While I know that this time span is considered to be "atomic", it still
seems off.

> The only way for a BB to lose its name is (was?) the messed up HMP
> drive_del: it wants to kill the BB right away, but can't, so it needs to
> hide it instead until it can.
> 
> New functionality may lead to a more complex live cycle where BBs may
> acquire and lose their name in new ways.
> 
> Note that I carefully avoid calling the reference the monitor's
> reference.  Because you made me realize it's not the monitor's alone!
> Lookup by name has more customers than just the monitor.

Maybe, but I'd personally consider this a service offered by the
monitor, and thus a reference by the monitor.

>> The second and third difference are what I referred to as "magic". You
>> could also call them "convenience", if you prefer, but in any case as we
>> can see by the existence blk_hide...(), removing the monitor reference
>> in blk_delete() appears to be wrong. Both should be separate operations,
>> and this is what this patch does.
> 
> I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del()
> shows that the name going away in blk_delete() is wrong.  It must go
> away there, because the thing it names goes away.

Yes, it must, but I believe it shouldn't have a name at that point at all.

> Additional operations to add / remove a BBs name may make sense; I'd
> have to see their users.  See "more complex live cycle" above.

They do make sense, if for nothing else, then because blk_hide...() is
replaced by a function that actually does something that makes sense in
the general life cycle.

>> Assuming that any blk_new() is ultimately done by the monitor, we maybe
>> actually do not need an own monitor_add_blk() function; except that
>> Kevin stated that he does deem it useful when I proposed inlining it
>> (back) into blk_new().
> 
> As Kevin noted, that's not a good assumption.

Which is a reason why we should have said explicit functions.

>> All in all, you have convinced me that the commit message is incorrect.
>> It should state that blk_backends is effectively repurposed to contain
>> the list of all BBs, and that a more explicit monitor_block_backends
>> list will take its place, with explicit operations for the monitor to
>> signal when it takes or releases the reference to a BB.
> 
> A member of monitor_block_backends is always a member of blk_backends.
> Correct?

I sure hope so.

> Is a member of blk_backends with a name always a member of
> monitor_block_backends?

No. Right now, after blk_new() it isn't; but Kevin proposed moving the
name setting to monitor_add_blk(), then it will be.

Apart from that, a BB that's passed to blk_delete() is currently
generally still a member of blk_backends (with a name). As said above, I
don't think it should be, and consequently it will not be a member of
monitor_block_backends.

So I guess you could say that I believe that being a member of
blk_backends hand having a name before this patch should be equivalent
to being a member of monitor_block_backends after this patch. It isn't,
because blk_backends contained some BBs which shouldn't be there, in my
opinion.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Jeff Cody]

On Wed, Feb 24, 2016 at 02:07:18PM +0100, Kevin Wolf wrote:
> Am 24.02.2016 um 13:44 hat Peter Lieven geschrieben:
> > if the size is forced I would set the chs values to max. this way no
> > new creator String is needed and it is even backwards compatible. this
> > is what disk2vhd does.
> 
> Does disk2vhd do it this way even if the size is smaller than the
> maximum that can be represented with CHS?
>

I don't know about disk2vhd, but I just created a 5G dynamic VHD
image on Hyper-V, and it produced:

cyl: 10402, heads: 16, secs: 63
virtual size: 5.0G (5368709120 bytes)

(the virtual size as calculated by CHS in that case would have been
5368430592 bytes)

I then tested the reverse - I modified qemu to create a VHD image with
5G as the current_size, but maxed out CHS parameters.  I imported it
into Hyper-V, and it worked fine - just recognized as a 5G disk with
5368709120 bytes.

But with all that, it seems like it may be better to mimic the Hyper-V
behavior, and use a new creator app string, with the normal CHS
values.


> > > Am 24.02.2016 um 13:24 schrieb Jeff Cody :
> > > 
> > >> On Wed, Feb 24, 2016 at 11:19:37AM +0100, Kevin Wolf wrote:
> > >> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> > >>> When QEMU creates a VHD image, it goes by the original spec,
> > >>> calculating the current_size based on the nearest CHS geometry (with an
> > >>> exception for disks > 127GB).
> > >>> 
> > >>> Apparently, Azure will only allow images that are sized to the nearest
> > >>> MB, and the current_size as calculated from CHS cannot guarantee that.
> > >>> 
> > >>> Allow QEMU to create images similar to how Hyper-V creates images, by
> > >>> setting current_size to the specified virtual disk size.  This
> > >>> introduces an option, force_size, to be passed to the vpc format during
> > >>> image creation, e.g.:
> > >>> 
> > >>>qemu-img convert -f raw -o force_size -O vpc test.img test.vhd
> > >>> 
> > >>> Bug reference: https://bugs.launchpad.net/qemu/+bug/1490611
> > >>> 
> > >>> Signed-off-by: Jeff Cody 
> > >> 
> > >> We need to set a different creator string here that makes vpc_open()
> > >> recognise the image as current_size based.
> > >> 
> > >> Kevin
> > > 
> > > How about "qem2"?  I initially thought about just changing the case on
> > > "qemu", but I was afraid some other software may treat the app creator
> > > string as case-insensitive.
> > > 
> > > I'll also update patch 1, to recognize that string as well.
> 
> I had the same thoughts about our options, and I wasn't fully convinced
> of either, so I didn't propose any. I was leaning towards the case
> change, though, as I don't think it should make a difference and it
> reads nicer. But "qem2" is okay with me, too.
> 
> Kevin

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Peter Lieven]

if the size is forced I would set the chs values to max. this way no new 
creator String is needed and it is even backwards compatible. this is what 
disk2vhd does.

Peter 


> Am 24.02.2016 um 13:24 schrieb Jeff Cody :
> 
>> On Wed, Feb 24, 2016 at 11:19:37AM +0100, Kevin Wolf wrote:
>> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
>>> When QEMU creates a VHD image, it goes by the original spec,
>>> calculating the current_size based on the nearest CHS geometry (with an
>>> exception for disks > 127GB).
>>> 
>>> Apparently, Azure will only allow images that are sized to the nearest
>>> MB, and the current_size as calculated from CHS cannot guarantee that.
>>> 
>>> Allow QEMU to create images similar to how Hyper-V creates images, by
>>> setting current_size to the specified virtual disk size.  This
>>> introduces an option, force_size, to be passed to the vpc format during
>>> image creation, e.g.:
>>> 
>>>qemu-img convert -f raw -o force_size -O vpc test.img test.vhd
>>> 
>>> Bug reference: https://bugs.launchpad.net/qemu/+bug/1490611
>>> 
>>> Signed-off-by: Jeff Cody 
>> 
>> We need to set a different creator string here that makes vpc_open()
>> recognise the image as current_size based.
>> 
>> Kevin
> 
> How about "qem2"?  I initially thought about just changing the case on
> "qemu", but I was afraid some other software may treat the app creator
> string as case-insensitive.
> 
> I'll also update patch 1, to recognize that string as well.

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Jeff Cody]

On Wed, Feb 24, 2016 at 11:19:37AM +0100, Kevin Wolf wrote:
> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> > When QEMU creates a VHD image, it goes by the original spec,
> > calculating the current_size based on the nearest CHS geometry (with an
> > exception for disks > 127GB).
> > 
> > Apparently, Azure will only allow images that are sized to the nearest
> > MB, and the current_size as calculated from CHS cannot guarantee that.
> > 
> > Allow QEMU to create images similar to how Hyper-V creates images, by
> > setting current_size to the specified virtual disk size.  This
> > introduces an option, force_size, to be passed to the vpc format during
> > image creation, e.g.:
> > 
> > qemu-img convert -f raw -o force_size -O vpc test.img test.vhd
> > 
> > Bug reference: https://bugs.launchpad.net/qemu/+bug/1490611
> > 
> > Signed-off-by: Jeff Cody 
> 
> We need to set a different creator string here that makes vpc_open()
> recognise the image as current_size based.
> 
> Kevin

How about "qem2"?  I initially thought about just changing the case on
"qemu", but I was afraid some other software may treat the app creator
string as case-insensitive.

I'll also update patch 1, to recognize that string as well.

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Jeff Cody]

On Wed, Feb 24, 2016 at 11:23:29AM +0100, Kevin Wolf wrote:
> Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> > This tests auto-detection, and overrides, of VHD image sizes created
> > by Virtual PC and Hyper-V.
> > 
> > This adds two sample images:
> > 
> > hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
> > virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
> > 
> > Signed-off-by: Jeff Cody 
> 
> > +echo
> > +echo === Testing VPC Autodetect ===
> > +echo
> > +_use_sample_img virtualpc-dynamic.vhd.bz2
> > +
> > +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
> > +h1=$QEMU_HANDLE
> > +
> > +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
> > +_send_qemu_cmd $h1 'quit' ""
> 
> I would avoid the big hammer of starting qemu processes when qemu-io can
> test the same:
> 
> $QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"
> 
> Kevin

OK, that sounds fine - I'll change that

Re: [Qemu-block] [PATCH 2/4] block/vpc: tests for auto-detecting VPC and Hyper-V VHD images

[Kevin Wolf]

Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> This tests auto-detection, and overrides, of VHD image sizes created
> by Virtual PC and Hyper-V.
> 
> This adds two sample images:
> 
> hyperv2012r2-dynamic.vhd.bz2 - dynamic VHD image created with Hyper-V
> virtualpc-dynamic.vhd.bz2- dynamic VHD image created with Virtual PC
> 
> Signed-off-by: Jeff Cody 

> +echo
> +echo === Testing VPC Autodetect ===
> +echo
> +_use_sample_img virtualpc-dynamic.vhd.bz2
> +
> +_launch_qemu -drive file="${TEST_IMG}",id=disk,format=vpc
> +h1=$QEMU_HANDLE
> +
> +_send_qemu_cmd $h1 'qemu-io disk "map"' "sectors"
> +_send_qemu_cmd $h1 'quit' ""

I would avoid the big hammer of starting qemu processes when qemu-io can
test the same:

$QEMU_IO -c "open -o file=${TEST_IMG},id=disk,format=vpc" -c "map"

Kevin

Re: [Qemu-block] [PATCH 3/4] block/vpc: give option to force the current_size field in .bdrv_create

[Kevin Wolf]

Am 24.02.2016 um 01:47 hat Jeff Cody geschrieben:
> When QEMU creates a VHD image, it goes by the original spec,
> calculating the current_size based on the nearest CHS geometry (with an
> exception for disks > 127GB).
> 
> Apparently, Azure will only allow images that are sized to the nearest
> MB, and the current_size as calculated from CHS cannot guarantee that.
> 
> Allow QEMU to create images similar to how Hyper-V creates images, by
> setting current_size to the specified virtual disk size.  This
> introduces an option, force_size, to be passed to the vpc format during
> image creation, e.g.:
> 
> qemu-img convert -f raw -o force_size -O vpc test.img test.vhd
> 
> Bug reference: https://bugs.launchpad.net/qemu/+bug/1490611
> 
> Signed-off-by: Jeff Cody 

We need to set a different creator string here that makes vpc_open()
recognise the image as current_size based.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

Am 24.02.2016 um 10:28 hat Markus Armbruster geschrieben:
> Max Reitz  writes:
> 
> > On 23.02.2016 10:48, Markus Armbruster wrote:
> >> Max Reitz  writes:
> >> 
> >>> On 22.02.2016 09:24, Markus Armbruster wrote:
>  Max Reitz  writes:
> 
> > On 17.02.2016 17:20, Kevin Wolf wrote:
> >> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
> >>> On 17.02.2016 11:53, Kevin Wolf wrote:
>  Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> > The monitor does hold references to some BlockBackends so it should 
> > have
> 
>  s/does hold/holds/?
> >>>
> >>> It was intentional, so I'd keep it unless you drop the question mark.
> >>
> >> For me it seems to imply something like "contrary to your 
> >> expectations",
> >> but maybe that's just my non-native English needing a fix.
> >>
> >> I don't find it surprising anyway that the monitor holds BB references.
> 
>  Me neither.
> 
> > The contrast I tried to point out is that while we do have these
> > references in theory, and they are reflected by a refcount, too, we do
> > not actually have these references because the monitor does not yet have
> > a list of the BBs it owns.
> 
>  Oh yes, it has.  It's just outsources their actual storage to
>  block-backend.c, but that's detail.
> >>>
> >>> In my opinion it's not a reference made by the monitor, then, especially
> >>> because it's done through magic. With this interpretation,
> >>> block-backend.c considers every BB to be monitor-owned (until
> >>> blk_hide...() is called).
> >> 
> >> block-backend.c holds a reference from blk_backends.  By *why* does it
> >> do that?  Let's go through the uses of blk_backends.
> >> 
> >> 0. blk_backends maintenance: blk_new(), blk_delete(),
> >>blk_hide_on_behalf_of_hmp_drive_del()
> >> 
> >> 1. To permit lookup by name, with blk_by_name().
> >> 
> >>In my view, block-backend.c holds this reference in trust for those
> >>parts of QEMU that reference by name rather than by pointer, most
> >>prominently the monitor.
> >> 
> >>I can't see the point of backing up the reference by name with a
> >>reference by pointer in the monitor, like your patch seems to do.
> >>What's the difference between the two?  Can one ever exist without
> >>the other?  Why in the monitor, and not in any other part looking up
> >>by name?
> >> 
> >> 2. To iterate over all named ones, with blk_next().
> >> 
> >>Since this can only return named BBs, the reference held in trust for
> >>named lookup suffices.
> >> 
> >> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
> >> 
> >>Since this must only return named BBs, the reference held in trust
> >>for named lookup suffices.
> >> 
> >> 4. To do something so unimportant that it's not worth explaining, with
> >>blk_remove_bs().
> >> 
> >>I made a point of giving every single external function a carefully
> >>worded contract, to hopefully shame future contributors into
> >>following suit.  Didn't work.
> >
> > Side note: I consider it very important and there was no other way to do
> > it before this series, because there is no list of all block backends.
> >
> >>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
> >>> then it's a really bad name and I put all the blame on that.
> >> 
> >> Naming is hard.  Feel free to propose better comments and/or better
> >> names.
> >
> > I did. It's "monitor_block_backends".
> >
> > So it's not an "emphasize the verb because it may be contrary to your
> > expectations", but an "emphasize it because it is contrary to what the
> > current code does" (which is not actually referencing these BBs).
> 
>  I disagree :)
> >>>
> >>> Then I'd say "It's contrary to my interpretation of what the current
> >>> code wants to do." Now it's my personal opinion! ;-)
> >>>
> >>> I wouldn't mind removing the "does" from the commit message (obviously),
> >>> but that is not the only thing there which conflicts with your opinion.
> >>> It clearly states that blk_backends is not the list of monitor-owned
> >>> BlockBackends, whereas you are saying that it very much is.
> >>>
> >>> So...? Rephrase it entirely? State that blk_backends is a bad name and
> >>> this commit is basically duplicating blk_backends as
> >>> monitor_block_backends, which is the correct name, and that a follow-up
> >>> commit will make blk_backends contain what it name implies it does? Or
> >>> just call the commit "Remove magic", because it adds explicit functions
> >>> for saying that a BB is monitor-owned or that it is not?
> >> 
> >> Can you explain the *actual* difference between blk_backends and
> >> monitor_block_backends?  "Actual" in the sense of difference in contents
> >> over time.
> >
> > First difference: The name. That's enough reason for me.
> >
> > Second 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Markus Armbruster]

Max Reitz  writes:

> On 23.02.2016 10:48, Markus Armbruster wrote:
>> Max Reitz  writes:
>> 
>>> On 22.02.2016 09:24, Markus Armbruster wrote:
 Max Reitz  writes:

> On 17.02.2016 17:20, Kevin Wolf wrote:
>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>> On 17.02.2016 11:53, Kevin Wolf wrote:
 Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> The monitor does hold references to some BlockBackends so it should 
> have

 s/does hold/holds/?
>>>
>>> It was intentional, so I'd keep it unless you drop the question mark.
>>
>> For me it seems to imply something like "contrary to your expectations",
>> but maybe that's just my non-native English needing a fix.
>>
>> I don't find it surprising anyway that the monitor holds BB references.

 Me neither.

> The contrast I tried to point out is that while we do have these
> references in theory, and they are reflected by a refcount, too, we do
> not actually have these references because the monitor does not yet have
> a list of the BBs it owns.

 Oh yes, it has.  It's just outsources their actual storage to
 block-backend.c, but that's detail.
>>>
>>> In my opinion it's not a reference made by the monitor, then, especially
>>> because it's done through magic. With this interpretation,
>>> block-backend.c considers every BB to be monitor-owned (until
>>> blk_hide...() is called).
>> 
>> block-backend.c holds a reference from blk_backends.  By *why* does it
>> do that?  Let's go through the uses of blk_backends.
>> 
>> 0. blk_backends maintenance: blk_new(), blk_delete(),
>>blk_hide_on_behalf_of_hmp_drive_del()
>> 
>> 1. To permit lookup by name, with blk_by_name().
>> 
>>In my view, block-backend.c holds this reference in trust for those
>>parts of QEMU that reference by name rather than by pointer, most
>>prominently the monitor.
>> 
>>I can't see the point of backing up the reference by name with a
>>reference by pointer in the monitor, like your patch seems to do.
>>What's the difference between the two?  Can one ever exist without
>>the other?  Why in the monitor, and not in any other part looking up
>>by name?
>> 
>> 2. To iterate over all named ones, with blk_next().
>> 
>>Since this can only return named BBs, the reference held in trust for
>>named lookup suffices.
>> 
>> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
>> 
>>Since this must only return named BBs, the reference held in trust
>>for named lookup suffices.
>> 
>> 4. To do something so unimportant that it's not worth explaining, with
>>blk_remove_bs().
>> 
>>I made a point of giving every single external function a carefully
>>worded contract, to hopefully shame future contributors into
>>following suit.  Didn't work.
>
> Side note: I consider it very important and there was no other way to do
> it before this series, because there is no list of all block backends.
>
>>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
>>> then it's a really bad name and I put all the blame on that.
>> 
>> Naming is hard.  Feel free to propose better comments and/or better
>> names.
>
> I did. It's "monitor_block_backends".
>
> So it's not an "emphasize the verb because it may be contrary to your
> expectations", but an "emphasize it because it is contrary to what the
> current code does" (which is not actually referencing these BBs).

 I disagree :)
>>>
>>> Then I'd say "It's contrary to my interpretation of what the current
>>> code wants to do." Now it's my personal opinion! ;-)
>>>
>>> I wouldn't mind removing the "does" from the commit message (obviously),
>>> but that is not the only thing there which conflicts with your opinion.
>>> It clearly states that blk_backends is not the list of monitor-owned
>>> BlockBackends, whereas you are saying that it very much is.
>>>
>>> So...? Rephrase it entirely? State that blk_backends is a bad name and
>>> this commit is basically duplicating blk_backends as
>>> monitor_block_backends, which is the correct name, and that a follow-up
>>> commit will make blk_backends contain what it name implies it does? Or
>>> just call the commit "Remove magic", because it adds explicit functions
>>> for saying that a BB is monitor-owned or that it is not?
>> 
>> Can you explain the *actual* difference between blk_backends and
>> monitor_block_backends?  "Actual" in the sense of difference in contents
>> over time.
>
> First difference: The name. That's enough reason for me.
>
> Second difference: blk_new() adds any BB to blk_backends automatically.
> It doesn't do so for monitor_block_backends.
>
> Third difference: Often the monitor doesn't take care of signalling that
> it's releasing its reference, only sometimes (where "sometimes" means
> every time blk_hide...() is