[Qemu-devel] [PULL 0/8] Block patches

[Qemu-devel] [PULL 0/8] Block patches

[Kevin Wolf]

The following changes since commit 162cbbd1736de2bca43fdefa7e98c54a361ee60d:

  Merge remote-tracking branch 'luiz/queue/qmp' into staging (2013-04-02 14:07:35 -0500)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git for-anthony

for you to fetch changes up to 094e751448359417c712ed1395d151c79ccd2538:

  qcow2: Fix L1 write error handling in qcow2_update_snapshot_refcount (2013-04-05 13:17:49 +0200)

----------------------------------------------------------------
KONRAD Frederic (1):
      virtio-blk-x: fix configuration synchronization.

Kevin Wolf (3):
      usb-storage: Forward serial number to scsi-disk
      qcow2: Return real error in qcow2_update_snapshot_refcount
      qcow2: Fix L1 write error handling in qcow2_update_snapshot_refcount

Stefan Hajnoczi (4):
      block: fix I/O throttling accounting blind spot
      block: keep I/O throttling slice time constant
      block: drop duplicated slice extension code
      block: clean up I/O throttling wait_time code

 block.c                    | 49 +++++++++++++++++++++-------------------------
 block/qcow2-refcount.c     | 25 +++++++++++------------
 blockdev.c                 |  1 -
 hw/pci/pci-hotplug.c       |  2 +-
 hw/s390x/s390-virtio-bus.c |  4 ++--
 hw/s390x/s390-virtio-bus.h |  1 -
 hw/s390x/virtio-ccw.c      |  4 ++--
 hw/s390x/virtio-ccw.h      |  1 -
 hw/scsi-bus.c              |  8 ++++++--
 hw/scsi.h                  |  3 ++-
 hw/usb/dev-storage.c       |  2 +-
 hw/virtio-blk.c            |  7 -------
 hw/virtio-blk.h            |  2 --
 hw/virtio-pci.c            |  7 ++++---
 hw/virtio-pci.h            |  1 -
 include/block/block_int.h  |  3 +--
 16 files changed, 54 insertions(+), 66 deletions(-)

[Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.

[Kevin Wolf]

From: KONRAD Frederic <fred.konrad@greensocs.com>

The virtio-blk-x configuration is not in sync with virtio-blk configuration.
So this patch remove the virtio-blk-x configuration field, and use virtio-blk
one for setting the properties.

This also remove a useless configuration copy in virtio_blk_device_init.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/s390x/s390-virtio-bus.c | 4 ++--
 hw/s390x/s390-virtio-bus.h | 1 -
 hw/s390x/virtio-ccw.c      | 4 ++--
 hw/s390x/virtio-ccw.h      | 1 -
 hw/virtio-blk.c            | 7 -------
 hw/virtio-blk.h            | 2 --
 hw/virtio-pci.c            | 7 ++++---
 hw/virtio-pci.h            | 1 -
 8 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 8c529c1..3824181 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -166,7 +166,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev)
 {
     VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
+
     qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
@@ -443,7 +443,7 @@ static const TypeInfo s390_virtio_net = {
 };
 
 static Property s390_virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
+    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, vdev.blk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h
index ebe8794..06f9479 100644
--- a/hw/s390x/s390-virtio-bus.h
+++ b/hw/s390x/s390-virtio-bus.h
@@ -127,7 +127,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev);
 typedef struct VirtIOBlkS390 {
     VirtIOS390Device parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 } VirtIOBlkS390;
 
 /* virtio-scsi-s390 */
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 5dce791..78a8f0e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -574,7 +574,7 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
 {
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
+
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
@@ -761,7 +761,7 @@ static const TypeInfo virtio_ccw_net = {
 static Property virtio_ccw_blk_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
+    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, vdev.blk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index d580510..5179374 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -113,7 +113,6 @@ typedef struct VirtIOSCSICcw {
 typedef struct VirtIOBlkCcw {
     VirtioCcwDevice parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 } VirtIOBlkCcw;
 
 /* virtio-balloon-ccw */
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index f2143fd..0414a8a 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -619,12 +619,6 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
-{
-    VirtIOBlock *s = VIRTIO_BLK(dev);
-    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
-}
-
 static int virtio_blk_device_init(VirtIODevice *vdev)
 {
     DeviceState *qdev = DEVICE(vdev);
@@ -656,7 +650,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
     vdev->reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
     s->conf = &blk->conf;
-    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 8c6c78b..e228057 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -148,6 +148,4 @@ typedef struct VirtIOBlock {
         DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
 #endif /* __linux__ */
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
-
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index fb20722..fbcc5f6 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1303,10 +1303,11 @@ static Property virtio_blk_pci_properties[] = {
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false),
+    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, vdev.blk.data_plane, 0,
+                                                                         false),
 #endif
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
+    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, vdev.blk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1314,7 +1315,7 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
+
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index f99f2eb..108ed12 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -115,7 +115,6 @@ struct VirtIOSCSIPCI {
 struct VirtIOBlkPCI {
     VirtIOPCIProxy parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 };
 
 /*
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/8] usb-storage: Forward serial number to scsi-disk

[Kevin Wolf]

usb-storage takes care to fetch the USB serial number from -drive
options, but it neglected to pass its own 'serial' property to the
scsi-disk it creates. With this patch, the 'serial' qdev property and
the 'serial' option in -drive behave the same and correctly apply the
serial number on both USB and SCSI level.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pci-hotplug.c | 2 +-
 hw/scsi-bus.c        | 8 ++++++--
 hw/scsi.h            | 3 ++-
 hw/usb/dev-storage.c | 2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/pci/pci-hotplug.c b/hw/pci/pci-hotplug.c
index f38df30..180ee07 100644
--- a/hw/pci/pci-hotplug.c
+++ b/hw/pci/pci-hotplug.c
@@ -99,7 +99,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
     dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
     dinfo->bus = scsibus->busnr;
     scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit,
-                                        false, -1);
+                                        false, -1, NULL);
     if (!scsidev) {
         return -1;
     }
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 08787c2..ac2093a 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -207,7 +207,8 @@ static int scsi_qdev_exit(DeviceState *qdev)
 
 /* handle legacy '-drive if=scsi,...' cmd line args */
 SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
-                                      int unit, bool removable, int bootindex)
+                                      int unit, bool removable, int bootindex,
+                                      const char *serial)
 {
     const char *driver;
     DeviceState *dev;
@@ -221,6 +222,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
     if (object_property_find(OBJECT(dev), "removable", NULL)) {
         qdev_prop_set_bit(dev, "removable", removable);
     }
+    if (serial) {
+        qdev_prop_set_string(dev, "serial", serial);
+    }
     if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
         qdev_free(dev);
         return NULL;
@@ -243,7 +247,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
             continue;
         }
         qemu_opts_loc_restore(dinfo->opts);
-        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false, -1)) {
+        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false, -1, NULL)) {
             res = -1;
             break;
         }
diff --git a/hw/scsi.h b/hw/scsi.h
index 33e2e0b..02a1497 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -160,7 +160,8 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
 }
 
 SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
-                                      int unit, bool removable, int bootindex);
+                                      int unit, bool removable, int bootindex,
+                                      const char *serial);
 int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
 
 /*
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index d3f01aa..21651b3 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -625,7 +625,7 @@ static int usb_msd_initfn_storage(USBDevice *dev)
     usb_desc_init(dev);
     scsi_bus_new(&s->bus, &s->dev.qdev, &usb_msd_scsi_info_storage);
     scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
-                                            s->conf.bootindex);
+                                            s->conf.bootindex, s->serial);
     if (!scsi_dev) {
         return -1;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/8] block: fix I/O throttling accounting blind spot

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

I/O throttling relies on bdrv_acct_done() which is called when a request
completes.  This leaves a blind spot since we only charge for completed
requests, not submitted requests.

For example, if there is 1 operation remaining in this time slice the
guest could submit 3 operations and they will all be submitted
successfully since they don't actually get accounted for until they
complete.

Originally we probably thought this is okay since the requests will be
accounted when the time slice is extended.  In practice it causes
fluctuations since the guest can exceed its I/O limit and it will be
punished for this later on.

Account for I/O upon submission so that I/O limits are enforced
properly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 21 ++++++++++-----------
 include/block/block_int.h |  2 +-
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 0ae2e93..25976b5 100644
--- a/block.c
+++ b/block.c
@@ -141,7 +141,6 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
     bs->slice_start = 0;
     bs->slice_end   = 0;
     bs->slice_time  = 0;
-    memset(&bs->io_base, 0, sizeof(bs->io_base));
 }
 
 static void bdrv_block_timer(void *opaque)
@@ -1436,8 +1435,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->slice_time         = bs_src->slice_time;
     bs_dest->slice_start        = bs_src->slice_start;
     bs_dest->slice_end          = bs_src->slice_end;
+    bs_dest->slice_submitted    = bs_src->slice_submitted;
     bs_dest->io_limits          = bs_src->io_limits;
-    bs_dest->io_base            = bs_src->io_base;
     bs_dest->throttled_reqs     = bs_src->throttled_reqs;
     bs_dest->block_timer        = bs_src->block_timer;
     bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
@@ -3768,9 +3767,9 @@ static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
     slice_time = bs->slice_end - bs->slice_start;
     slice_time /= (NANOSECONDS_PER_SECOND);
     bytes_limit = bps_limit * slice_time;
-    bytes_base  = bs->nr_bytes[is_write] - bs->io_base.bytes[is_write];
+    bytes_base  = bs->slice_submitted.bytes[is_write];
     if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]) {
-        bytes_base += bs->nr_bytes[!is_write] - bs->io_base.bytes[!is_write];
+        bytes_base += bs->slice_submitted.bytes[!is_write];
     }
 
     /* bytes_base: the bytes of data which have been read/written; and
@@ -3828,9 +3827,9 @@ static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
     slice_time = bs->slice_end - bs->slice_start;
     slice_time /= (NANOSECONDS_PER_SECOND);
     ios_limit  = iops_limit * slice_time;
-    ios_base   = bs->nr_ops[is_write] - bs->io_base.ios[is_write];
+    ios_base   = bs->slice_submitted.ios[is_write];
     if (bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL]) {
-        ios_base += bs->nr_ops[!is_write] - bs->io_base.ios[!is_write];
+        ios_base += bs->slice_submitted.ios[!is_write];
     }
 
     if (ios_base + 1 <= ios_limit) {
@@ -3875,11 +3874,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
         bs->slice_start = now;
         bs->slice_end   = now + bs->slice_time;
 
-        bs->io_base.bytes[is_write]  = bs->nr_bytes[is_write];
-        bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
-
-        bs->io_base.ios[is_write]    = bs->nr_ops[is_write];
-        bs->io_base.ios[!is_write]   = bs->nr_ops[!is_write];
+        memset(&bs->slice_submitted, 0, sizeof(bs->slice_submitted));
     }
 
     elapsed_time  = now - bs->slice_start;
@@ -3907,6 +3902,10 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
         *wait = 0;
     }
 
+    bs->slice_submitted.bytes[is_write] += (int64_t)nb_sectors *
+                                           BDRV_SECTOR_SIZE;
+    bs->slice_submitted.ios[is_write]++;
+
     return false;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0986a2d..83941d8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -256,7 +256,7 @@ struct BlockDriverState {
     int64_t slice_start;
     int64_t slice_end;
     BlockIOLimit io_limits;
-    BlockIOBaseValue  io_base;
+    BlockIOBaseValue slice_submitted;
     CoQueue      throttled_reqs;
     QEMUTimer    *block_timer;
     bool         io_limits_enabled;
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/8] block: keep I/O throttling slice time constant

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

It is not necessary to adjust the slice time at runtime.  We already
extend the current slice in order to carry over accounting into the next
slice.  Changing the actual slice time value introduces oscillations.

The guest may experience large changes in throughput or IOPS from one
moment to the next when slice times are adjusted.

Reported-by: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 19 +++++++++----------
 blockdev.c                |  1 -
 include/block/block_int.h |  1 -
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 25976b5..00eca27 100644
--- a/block.c
+++ b/block.c
@@ -140,7 +140,6 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
 
     bs->slice_start = 0;
     bs->slice_end   = 0;
-    bs->slice_time  = 0;
 }
 
 static void bdrv_block_timer(void *opaque)
@@ -1432,7 +1431,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->enable_write_cache = bs_src->enable_write_cache;
 
     /* i/o timing parameters */
-    bs_dest->slice_time         = bs_src->slice_time;
     bs_dest->slice_start        = bs_src->slice_start;
     bs_dest->slice_end          = bs_src->slice_end;
     bs_dest->slice_submitted    = bs_src->slice_submitted;
@@ -3749,6 +3747,7 @@ static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
                  bool is_write, double elapsed_time, uint64_t *wait)
 {
     uint64_t bps_limit = 0;
+    uint64_t extension;
     double   bytes_limit, bytes_base, bytes_res;
     double   slice_time, wait_time;
 
@@ -3796,8 +3795,10 @@ static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
      * info can be kept until the timer fire, so it is increased and tuned
      * based on the result of experiment.
      */
-    bs->slice_time = wait_time * BLOCK_IO_SLICE_TIME * 10;
-    bs->slice_end += bs->slice_time - 3 * BLOCK_IO_SLICE_TIME;
+    extension = wait_time * NANOSECONDS_PER_SECOND;
+    extension = DIV_ROUND_UP(extension, BLOCK_IO_SLICE_TIME) *
+                BLOCK_IO_SLICE_TIME;
+    bs->slice_end += extension;
     if (wait) {
         *wait = wait_time * BLOCK_IO_SLICE_TIME * 10;
     }
@@ -3848,8 +3849,8 @@ static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
         wait_time = 0;
     }
 
-    bs->slice_time = wait_time * BLOCK_IO_SLICE_TIME * 10;
-    bs->slice_end += bs->slice_time - 3 * BLOCK_IO_SLICE_TIME;
+    /* Exceeded current slice, extend it by another slice time */
+    bs->slice_end += BLOCK_IO_SLICE_TIME;
     if (wait) {
         *wait = wait_time * BLOCK_IO_SLICE_TIME * 10;
     }
@@ -3868,12 +3869,10 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
     now = qemu_get_clock_ns(vm_clock);
     if ((bs->slice_start < now)
         && (bs->slice_end > now)) {
-        bs->slice_end = now + bs->slice_time;
+        bs->slice_end = now + BLOCK_IO_SLICE_TIME;
     } else {
-        bs->slice_time  =  5 * BLOCK_IO_SLICE_TIME;
         bs->slice_start = now;
-        bs->slice_end   = now + bs->slice_time;
-
+        bs->slice_end   = now + BLOCK_IO_SLICE_TIME;
         memset(&bs->slice_submitted, 0, sizeof(bs->slice_submitted));
     }
 
diff --git a/blockdev.c b/blockdev.c
index 8cdc9ce..6dc999d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1069,7 +1069,6 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     }
 
     bs->io_limits = io_limits;
-    bs->slice_time = BLOCK_IO_SLICE_TIME;
 
     if (!bs->io_limits_enabled && bdrv_io_limits_enabled(bs)) {
         bdrv_io_limits_enable(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 83941d8..9aa98b5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -252,7 +252,6 @@ struct BlockDriverState {
     unsigned int copy_on_read_in_flight;
 
     /* the time for latest disk I/O */
-    int64_t slice_time;
     int64_t slice_start;
     int64_t slice_end;
     BlockIOLimit io_limits;
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/8] block: drop duplicated slice extension code

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The current slice is extended when an I/O request exceeds the limit.
There is no need to extend the slice every time we check a request.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 00eca27..aa16fc4 100644
--- a/block.c
+++ b/block.c
@@ -3867,10 +3867,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
     int      bps_ret, iops_ret;
 
     now = qemu_get_clock_ns(vm_clock);
-    if ((bs->slice_start < now)
-        && (bs->slice_end > now)) {
-        bs->slice_end = now + BLOCK_IO_SLICE_TIME;
-    } else {
+    if (now > bs->slice_end) {
         bs->slice_start = now;
         bs->slice_end   = now + BLOCK_IO_SLICE_TIME;
         memset(&bs->slice_submitted, 0, sizeof(bs->slice_submitted));
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/8] block: clean up I/O throttling wait_time code

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The wait_time variable is in seconds.  Reflect this in a comment and use
NANOSECONDS_PER_SECOND instead of BLOCK_IO_SLICE_TIME * 10 (which
happens to have the right value).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index aa16fc4..602d8a4 100644
--- a/block.c
+++ b/block.c
@@ -3800,7 +3800,7 @@ static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
                 BLOCK_IO_SLICE_TIME;
     bs->slice_end += extension;
     if (wait) {
-        *wait = wait_time * BLOCK_IO_SLICE_TIME * 10;
+        *wait = wait_time * NANOSECONDS_PER_SECOND;
     }
 
     return true;
@@ -3841,7 +3841,7 @@ static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
         return false;
     }
 
-    /* Calc approx time to dispatch */
+    /* Calc approx time to dispatch, in seconds */
     wait_time = (ios_base + 1) / iops_limit;
     if (wait_time > elapsed_time) {
         wait_time = wait_time - elapsed_time;
@@ -3852,7 +3852,7 @@ static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
     /* Exceeded current slice, extend it by another slice time */
     bs->slice_end += BLOCK_IO_SLICE_TIME;
     if (wait) {
-        *wait = wait_time * BLOCK_IO_SLICE_TIME * 10;
+        *wait = wait_time * NANOSECONDS_PER_SECOND;
     }
 
     return true;
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/8] qcow2: Return real error in qcow2_update_snapshot_refcount

[Kevin Wolf]

This fixes the error message triggered by the following script:

    cat > /tmp/blkdebug.cfg <<EOF
    [inject-error]
    event = "cluster_free"
    errno = "28"
    immediately = "off"
    EOF

    $qemu_img create -f qcow2 test.qcow2 10G
    $qemu_img snapshot -c snap test.qcow2
    $qemu_img snapshot -d snap blkdebug:/tmp/blkdebug.cfg:test.qcow2

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c38e970..4799681 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -747,10 +747,9 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     if (l1_table_offset != s->l1_table_offset) {
         l1_table = g_malloc0(align_offset(l1_size2, 512));
         l1_allocated = 1;
-        if (bdrv_pread(bs->file, l1_table_offset,
-                       l1_table, l1_size2) != l1_size2)
-        {
-            ret = -EIO;
+
+        ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
+        if (ret < 0) {
             goto fail;
         }
 
@@ -802,7 +801,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                         }
 
                         if (refcount < 0) {
-                            ret = -EIO;
+                            ret = refcount;
                             goto fail;
                         }
                     }
@@ -833,7 +832,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                 refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
             }
             if (refcount < 0) {
-                ret = -EIO;
+                ret = refcount;
                 goto fail;
             } else if (refcount == 1) {
                 l2_offset |= QCOW_OFLAG_COPIED;
-- 
1.8.1.4

[Qemu-devel] [PATCH 8/8] qcow2: Fix L1 write error handling in qcow2_update_snapshot_refcount

[Kevin Wolf]

It ignored the error code, and at least the 'goto fail' is obvious
nonsense as it creates an endless loop (if the next attempt doesn't
magically succeed) and leaves the in-memory L1 table in big-endian
instead of converting it back.

In error cases, there's no point in writing an updated L1 table, so
skip this part for them.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4799681..b32738f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -851,14 +851,16 @@ fail:
     }
 
     /* Update L1 only if it isn't deleted anyway (addend = -1) */
-    if (addend >= 0 && l1_modified) {
-        for(i = 0; i < l1_size; i++)
+    if (ret == 0 && addend >= 0 && l1_modified) {
+        for (i = 0; i < l1_size; i++) {
             cpu_to_be64s(&l1_table[i]);
-        if (bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table,
-                        l1_size2) < 0)
-            goto fail;
-        for(i = 0; i < l1_size; i++)
+        }
+
+        ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1_size2);
+
+        for (i = 0; i < l1_size; i++) {
             be64_to_cpus(&l1_table[i]);
+        }
     }
     if (l1_allocated)
         g_free(l1_table);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> The virtio-blk-x configuration is not in sync with virtio-blk configuration.
> So this patch remove the virtio-blk-x configuration field, and use virtio-blk
> one for setting the properties.
>
> This also remove a useless configuration copy in virtio_blk_device_init.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

This breaks -M pc-1.0 as it breaks global properties.

Specifically, we have stuff like:

        },{\
            .driver   = "virtio-blk-pci",\
            .property = "config-wce",\
            .value    = "off",\
        }

for -M pc-1.0

But:

[aliguori@ccnode4 qemu]$ x86_64-softmmu/qemu-system-x86_64 -vnc none -monitor stdio -M pc-1.0 -drive file=dummy.img,snapshot=on,if=virtio
QEMU 1.4.50 monitor - type 'help' for more information
(qemu) info qtree
bus: main-system-bus
  type System
<snip>
  dev: i440FX-pcihost, id ""
    irq 0
    bus: pci.0
      type PCI
      dev: virtio-blk-pci, id ""
        class = 0x0
        ioeventfd = off
        vectors = 2
        x-data-plane = off
        indirect_desc = on
        event_idx = on
        drive = virtio0
        logical_block_size = 512
        physical_block_size = 512
        min_io_size = 0
        opt_io_size = 0
        bootindex = -1
        discard_granularity = 4294967295
        cyls = 16383
        heads = 16
        secs = 63
        serial = <null>
        config-wce = on

^^^

This should be config-wce = off when we use -M pc-1.0.

Regards,

Anthony Liguori

> ---
>  hw/s390x/s390-virtio-bus.c | 4 ++--
>  hw/s390x/s390-virtio-bus.h | 1 -
>  hw/s390x/virtio-ccw.c      | 4 ++--
>  hw/s390x/virtio-ccw.h      | 1 -
>  hw/virtio-blk.c            | 7 -------
>  hw/virtio-blk.h            | 2 --
>  hw/virtio-pci.c            | 7 ++++---
>  hw/virtio-pci.h            | 1 -
>  8 files changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 8c529c1..3824181 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -166,7 +166,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev)
>  {
>      VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
> +
>      qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> @@ -443,7 +443,7 @@ static const TypeInfo s390_virtio_net = {
>  };
>  
>  static Property s390_virtio_blk_properties[] = {
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, vdev.blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h
> index ebe8794..06f9479 100644
> --- a/hw/s390x/s390-virtio-bus.h
> +++ b/hw/s390x/s390-virtio-bus.h
> @@ -127,7 +127,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev);
>  typedef struct VirtIOBlkS390 {
>      VirtIOS390Device parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  } VirtIOBlkS390;
>  
>  /* virtio-scsi-s390 */
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 5dce791..78a8f0e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -574,7 +574,7 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
>  {
>      VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
> +
>      qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> @@ -761,7 +761,7 @@ static const TypeInfo virtio_ccw_net = {
>  static Property virtio_ccw_blk_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, vdev.blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index d580510..5179374 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -113,7 +113,6 @@ typedef struct VirtIOSCSICcw {
>  typedef struct VirtIOBlkCcw {
>      VirtioCcwDevice parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  } VirtIOBlkCcw;
>  
>  /* virtio-balloon-ccw */
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index f2143fd..0414a8a 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -619,12 +619,6 @@ static const BlockDevOps virtio_block_ops = {
>      .resize_cb = virtio_blk_resize,
>  };
>  
> -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
> -{
> -    VirtIOBlock *s = VIRTIO_BLK(dev);
> -    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
> -}
> -
>  static int virtio_blk_device_init(VirtIODevice *vdev)
>  {
>      DeviceState *qdev = DEVICE(vdev);
> @@ -656,7 +650,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>      vdev->reset = virtio_blk_reset;
>      s->bs = blk->conf.bs;
>      s->conf = &blk->conf;
> -    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
>      s->rq = NULL;
>      s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
>  
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 8c6c78b..e228057 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -148,6 +148,4 @@ typedef struct VirtIOBlock {
>          DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
>  #endif /* __linux__ */
>  
> -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
> -
>  #endif
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index fb20722..fbcc5f6 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -1303,10 +1303,11 @@ static Property virtio_blk_pci_properties[] = {
>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> -    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false),
> +    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, vdev.blk.data_plane, 0,
> +                                                                         false),
>  #endif
>      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
> +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, vdev.blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1314,7 +1315,7 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev)
>  {
>      VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> -    virtio_blk_set_conf(vdev, &(dev->blk));
> +
>      qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
>      if (qdev_init(vdev) < 0) {
>          return -1;
> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
> index f99f2eb..108ed12 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -115,7 +115,6 @@ struct VirtIOSCSIPCI {
>  struct VirtIOBlkPCI {
>      VirtIOPCIProxy parent_obj;
>      VirtIOBlock vdev;
> -    VirtIOBlkConf blk;
>  };
>  
>  /*
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.

[Peter Maydell]

On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> The virtio-blk-x configuration is not in sync with virtio-blk configuration.
>> So this patch remove the virtio-blk-x configuration field, and use virtio-blk
>> one for setting the properties.
>>
>> This also remove a useless configuration copy in virtio_blk_device_init.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
> This breaks -M pc-1.0 as it breaks global properties.

The first level analysis of why this happens:

 (1) global values are set as part of DeviceState's instance_init function
     [and we set the config_wce field to 0 here]
 (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second
 (3) it calls object_initialize on its embedded virtio-blk object
 (4) the DeviceState instance_init for that child object sets the
     properties to their init values [thus resetting config_wce to 1]

The effect is that at the moment you can't have a property on
a parent object whose value is stored in the child object
(unless you do something hacky like read the default before
calling object_initialize and restore it afterwards].

It seems to me that the underlying problem here is that we
set global values too early -- they should be set after a
device is completely instance_init'd, not halfway through init.

-- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> The virtio-blk-x configuration is not in sync with virtio-blk configuration.
>>> So this patch remove the virtio-blk-x configuration field, and use virtio-blk
>>> one for setting the properties.
>>>
>>> This also remove a useless configuration copy in virtio_blk_device_init.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>> This breaks -M pc-1.0 as it breaks global properties.
>
> The first level analysis of why this happens:
>
>  (1) global values are set as part of DeviceState's instance_init function
>      [and we set the config_wce field to 0 here]
>  (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second
>  (3) it calls object_initialize on its embedded virtio-blk object
>  (4) the DeviceState instance_init for that child object sets the
>      properties to their init values [thus resetting config_wce to 1]
>
> The effect is that at the moment you can't have a property on
> a parent object whose value is stored in the child object
> (unless you do something hacky like read the default before
> calling object_initialize and restore it afterwards].
>
> It seems to me that the underlying problem here is that we
> set global values too early -- they should be set after a
> device is completely instance_init'd, not halfway through init.

Here is an in-progress series that forwards properties from the
virtio-blk-pci to virtio-blk.

I think it's the best solution.  The approach needs some polishing but I
wanted to share early.

https://github.com/aliguori/qemu/commits/qom-forward.1

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.

[KONRAD Frédéric]

On 05/04/2013 21:24, Anthony Liguori wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>>
>>>> The virtio-blk-x configuration is not in sync with virtio-blk configuration.
>>>> So this patch remove the virtio-blk-x configuration field, and use virtio-blk
>>>> one for setting the properties.
>>>>
>>>> This also remove a useless configuration copy in virtio_blk_device_init.
>>>>
>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> This breaks -M pc-1.0 as it breaks global properties.
>> The first level analysis of why this happens:
>>
>>   (1) global values are set as part of DeviceState's instance_init function
>>       [and we set the config_wce field to 0 here]
>>   (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second
>>   (3) it calls object_initialize on its embedded virtio-blk object
>>   (4) the DeviceState instance_init for that child object sets the
>>       properties to their init values [thus resetting config_wce to 1]
>>
>> The effect is that at the moment you can't have a property on
>> a parent object whose value is stored in the child object
>> (unless you do something hacky like read the default before
>> calling object_initialize and restore it afterwards].
>>
>> It seems to me that the underlying problem here is that we
>> set global values too early -- they should be set after a
>> device is completely instance_init'd, not halfway through init.
> Here is an in-progress series that forwards properties from the
> virtio-blk-pci to virtio-blk.
>
> I think it's the best solution.  The approach needs some polishing but I
> wanted to share early.
>
> https://github.com/aliguori/qemu/commits/qom-forward.1
>
> Regards,
>
> Anthony Liguori
>
>> -- PMM
Seems, it's what we need here.

I think it is the same think for all other backends?

Fred