* [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 16:47 ` Anthony Liguori
2013-04-05 13:28 ` [Qemu-devel] [PATCH 2/8] usb-storage: Forward serial number to scsi-disk Kevin Wolf
` (6 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.
2013-04-05 13:28 ` [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization Kevin Wolf
@ 2013-04-05 16:47 ` Anthony Liguori
2013-04-05 17:56 ` Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2013-04-05 16:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.
2013-04-05 16:47 ` Anthony Liguori
@ 2013-04-05 17:56 ` Peter Maydell
2013-04-05 19:24 ` Anthony Liguori
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2013-04-05 17:56 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Kevin Wolf, qemu-devel, KONRAD Frédéric
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.
2013-04-05 17:56 ` Peter Maydell
@ 2013-04-05 19:24 ` Anthony Liguori
2013-04-06 19:31 ` KONRAD Frédéric
0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2013-04-05 19:24 UTC (permalink / raw)
To: Peter Maydell; +Cc: Kevin Wolf, qemu-devel, KONRAD Frédéric
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization.
2013-04-05 19:24 ` Anthony Liguori
@ 2013-04-06 19:31 ` KONRAD Frédéric
0 siblings, 0 replies; 13+ messages in thread
From: KONRAD Frédéric @ 2013-04-06 19:31 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Kevin Wolf, Peter Maydell, qemu-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/8] usb-storage: Forward serial number to scsi-disk
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 3/8] block: fix I/O throttling accounting blind spot Kevin Wolf
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 3/8] block: fix I/O throttling accounting blind spot
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 1/8] virtio-blk-x: fix configuration synchronization Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 2/8] usb-storage: Forward serial number to scsi-disk Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 4/8] block: keep I/O throttling slice time constant Kevin Wolf
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 4/8] block: keep I/O throttling slice time constant
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
` (2 preceding siblings ...)
2013-04-05 13:28 ` [Qemu-devel] [PATCH 3/8] block: fix I/O throttling accounting blind spot Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 5/8] block: drop duplicated slice extension code Kevin Wolf
` (3 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 5/8] block: drop duplicated slice extension code
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
` (3 preceding siblings ...)
2013-04-05 13:28 ` [Qemu-devel] [PATCH 4/8] block: keep I/O throttling slice time constant Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 6/8] block: clean up I/O throttling wait_time code Kevin Wolf
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 6/8] block: clean up I/O throttling wait_time code
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
` (4 preceding siblings ...)
2013-04-05 13:28 ` [Qemu-devel] [PATCH 5/8] block: drop duplicated slice extension code Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 7/8] qcow2: Return real error in qcow2_update_snapshot_refcount Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 8/8] qcow2: Fix L1 write error handling " Kevin Wolf
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 7/8] qcow2: Return real error in qcow2_update_snapshot_refcount
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
` (5 preceding siblings ...)
2013-04-05 13:28 ` [Qemu-devel] [PATCH 6/8] block: clean up I/O throttling wait_time code Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
2013-04-05 13:28 ` [Qemu-devel] [PATCH 8/8] qcow2: Fix L1 write error handling " Kevin Wolf
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Qemu-devel] [PATCH 8/8] qcow2: Fix L1 write error handling in qcow2_update_snapshot_refcount
2013-04-05 13:28 [Qemu-devel] [PULL 0/8] Block patches Kevin Wolf
` (6 preceding siblings ...)
2013-04-05 13:28 ` [Qemu-devel] [PATCH 7/8] qcow2: Return real error in qcow2_update_snapshot_refcount Kevin Wolf
@ 2013-04-05 13:28 ` Kevin Wolf
7 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2013-04-05 13:28 UTC (permalink / raw)
To: anthony; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread