Re: [Qemu-block] [Qemu-devel] [PATCH] block: Apply auto-read-only for ro-whitelist drivers
On 1/22/19 6:19 AM, Kevin Wolf wrote: > If QEMU was configured with a driver in --block-drv-ro-whitelist, trying > to use that driver read-write resulted in an error message even if > auto-read-only=on was set. > > Consider auto-read-only=on for the whitelist checking and use it to > automatically degrade to read-only for block drivers on the read-only > whitelist. > > Signed-off-by: Kevin Wolf > --- > block.c | 20 +--- > 1 file changed, 13 insertions(+), 7 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode
On Tue, 22 Jan 2019, BALATON Zoltan wrote:
The device was initialised in ISA compatibility mode and native PCI
IDE mode was not implemented but no clents are known to need ISA mode
but to the contrary, most clients that want to switch to and use
device in native PCI IDE mode. Therefore implement native PCI mode and
switch default to that.
There are some typos in here so here's a fixed, reworded one:
This device only implemented ISA compatibility mode and native PCI IDE
mode was missing but no clients actually need ISA mode but to the
contrary, they usually want to switch to and use device in native PCI IDE
mode. Therefore implement native PCI mode and switch default to that.
Signed-off-by: BALATON Zoltan
---
hw/ide/via.c | 52 ++--
1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index c6e43a8812..ac9385228c 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -32,15 +32,6 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static const struct {
-int iobase;
-int iobase2;
-int isairq;
-} port_info[] = {
-{0x1f0, 0x3f6, 14},
-{0x170, 0x376, 15},
-};
-
static uint64_t bmdma_read(void *opaque, hwaddr addr,
unsigned size)
{
@@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
}
}
+static void via_ide_set_irq(void *opaque, int n, int level)
+{
+PCIDevice *d = PCI_DEVICE(opaque);
+
+if (level) {
+d->config[0x70 + n * 8] |= 0x80;
+} else {
+d->config[0x70 + n * 8] &= ~0x80;
+}
+
+level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
+n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
+if (n) {
+qemu_set_irq(isa_get_irq(NULL, n), level);
+}
+}
+
static void via_ide_reset(void *opaque)
{
PCIIDEState *d = opaque;
@@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
ide_bus_reset(&d->bus[i]);
}
-pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
+pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
PCI_STATUS_DEVSEL_MEDIUM);
@@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
uint8_t *pci_conf = dev->config;
int i;
-pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
+pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
+dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
qemu_register_reset(via_ide_reset, d);
+
+memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
+ &d->bus[0], "via-ide0-data", 8);
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
+
+memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
+ &d->bus[0], "via-ide0-cmd", 4);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
+
+memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
+ &d->bus[1], "via-ide1-data", 8);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
+
+memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
+ &d->bus[1], "via-ide1-cmd", 4);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
+
bmdma_setup_bar(d);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
@@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-port_info[i].iobase2);
-ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
[Qemu-block] [PATCH 1/3] ide/via: Remove vt82c686b_init_ports() function
This function is only called once from vt82c686b_ide_realize() and its
content is simple enough to not need a separate function but be
included in realize directly (as done in other IDE models except PIIX
currently).
Signed-off-by: BALATON Zoltan
---
hw/ide/via.c | 44
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 987d99c5ec..46cac7b8d6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -32,6 +32,15 @@
#include "hw/ide/pci.h"
#include "trace.h"
+static const struct {
+int iobase;
+int iobase2;
+int isairq;
+} port_info[] = {
+{0x1f0, 0x3f6, 14},
+{0x170, 0x376, 15},
+};
+
static uint64_t bmdma_read(void *opaque, hwaddr addr,
unsigned size)
{
@@ -143,34 +152,12 @@ static void via_reset(void *opaque)
pci_set_long(pci_conf + 0xc0, 0x00020001);
}
-static void vt82c686b_init_ports(PCIIDEState *d) {
-static const struct {
-int iobase;
-int iobase2;
-int isairq;
-} port_info[] = {
-{0x1f0, 0x3f6, 14},
-{0x170, 0x376, 15},
-};
-int i;
-
-for (i = 0; i < 2; i++) {
-ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-port_info[i].iobase2);
-ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
-
-bmdma_init(&d->bus[i], &d->bmdma[i], d);
-d->bmdma[i].bus = &d->bus[i];
-ide_register_restart_cb(&d->bus[i]);
-}
-}
-
/* via ide func */
static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
{
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev->config;
+int i;
pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
@@ -181,7 +168,16 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error
**errp)
vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
-vt82c686b_init_ports(d);
+for (i = 0; i < 2; i++) {
+ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
+ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+port_info[i].iobase2);
+ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+
+bmdma_init(&d->bus[i], &d->bmdma[i], d);
+d->bmdma[i].bus = &d->bus[i];
+ide_register_restart_cb(&d->bus[i]);
+}
}
static void vt82c686b_ide_exitfn(PCIDevice *dev)
--
2.13.7
[Qemu-block] [PATCH 0/3] via-ide clean ups and native mode
Hello, This implements native PCI ATA mode for via-ida device model which is what most guests expect and want to use instead of the current legacy compatibility mode which nothing seems to depend on. Also rename some functions to via_ide to make this device truely independent from the VIA 82C686B chip as it can be used independently as well. This is on top of my previous CMD646 cleanup series. Regards, BALATON Zoltan BALATON Zoltan (3): ide/via: Remove vt82c686b_init_ports() function ide/via: Rename functions to match device name ide/via: Implement and use native PCI IDE mode hw/ide/via.c| 87 ++--- hw/mips/mips_fulong2e.c | 2 +- include/hw/ide.h| 2 +- 3 files changed, 55 insertions(+), 36 deletions(-) -- 2.13.7
[Qemu-block] [PATCH 2/3] ide/via: Rename functions to match device name
The device is called via-ide and the modelled IDE controller is not
specific to 82C686B but is also usable independently. Therefore, change
function name prefixes accordingly to match device name.
Signed-off-by: BALATON Zoltan
---
hw/ide/via.c| 15 +++
hw/mips/mips_fulong2e.c | 2 +-
include/hw/ide.h| 2 +-
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 46cac7b8d6..c6e43a8812 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -110,7 +110,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
}
}
-static void via_reset(void *opaque)
+static void via_ide_reset(void *opaque)
{
PCIIDEState *d = opaque;
PCIDevice *pd = PCI_DEVICE(d);
@@ -152,8 +152,7 @@ static void via_reset(void *opaque)
pci_set_long(pci_conf + 0xc0, 0x00020001);
}
-/* via ide func */
-static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
+static void via_ide_realize(PCIDevice *dev, Error **errp)
{
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev->config;
@@ -162,7 +161,7 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error
**errp)
pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
-qemu_register_reset(via_reset, d);
+qemu_register_reset(via_ide_reset, d);
bmdma_setup_bar(d);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
@@ -180,7 +179,7 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error
**errp)
}
}
-static void vt82c686b_ide_exitfn(PCIDevice *dev)
+static void via_ide_exitfn(PCIDevice *dev)
{
PCIIDEState *d = PCI_IDE(dev);
unsigned i;
@@ -191,7 +190,7 @@ static void vt82c686b_ide_exitfn(PCIDevice *dev)
}
}
-void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
{
PCIDevice *dev;
@@ -204,8 +203,8 @@ static void via_ide_class_init(ObjectClass *klass, void
*data)
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-k->realize = vt82c686b_ide_realize;
-k->exit = vt82c686b_ide_exitfn;
+k->realize = via_ide_realize;
+k->exit = via_ide_exitfn;
k->vendor_id = PCI_VENDOR_ID_VIA;
k->device_id = PCI_DEVICE_ID_VIA_IDE;
k->revision = 0x06;
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 2fbba32c48..42d09f6892 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -249,7 +249,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
ide_drive_get(hd, ARRAY_SIZE(hd));
-vt82c686b_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
+via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 3ae087c572..28d8a06439 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -18,7 +18,7 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo
**hd_table, int devfn);
PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
-void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
/* ide-mmio.c */
void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
--
2.13.7
[Qemu-block] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode
The device was initialised in ISA compatibility mode and native PCI
IDE mode was not implemented but no clents are known to need ISA mode
but to the contrary, most clients that want to switch to and use
device in native PCI IDE mode. Therefore implement native PCI mode and
switch default to that.
Signed-off-by: BALATON Zoltan
---
hw/ide/via.c | 52 ++--
1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index c6e43a8812..ac9385228c 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -32,15 +32,6 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static const struct {
-int iobase;
-int iobase2;
-int isairq;
-} port_info[] = {
-{0x1f0, 0x3f6, 14},
-{0x170, 0x376, 15},
-};
-
static uint64_t bmdma_read(void *opaque, hwaddr addr,
unsigned size)
{
@@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
}
}
+static void via_ide_set_irq(void *opaque, int n, int level)
+{
+PCIDevice *d = PCI_DEVICE(opaque);
+
+if (level) {
+d->config[0x70 + n * 8] |= 0x80;
+} else {
+d->config[0x70 + n * 8] &= ~0x80;
+}
+
+level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
+n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
+if (n) {
+qemu_set_irq(isa_get_irq(NULL, n), level);
+}
+}
+
static void via_ide_reset(void *opaque)
{
PCIIDEState *d = opaque;
@@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
ide_bus_reset(&d->bus[i]);
}
-pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
+pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
PCI_STATUS_DEVSEL_MEDIUM);
@@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
uint8_t *pci_conf = dev->config;
int i;
-pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
+pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
+dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
qemu_register_reset(via_ide_reset, d);
+
+memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
+ &d->bus[0], "via-ide0-data", 8);
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
+
+memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
+ &d->bus[0], "via-ide0-cmd", 4);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
+
+memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
+ &d->bus[1], "via-ide1-data", 8);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
+
+memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
+ &d->bus[1], "via-ide1-cmd", 4);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
+
bmdma_setup_bar(d);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
@@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-port_info[i].iobase2);
-ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
--
2.13.7
[Qemu-block] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev
This patch forbids attaching a disk to a SCSI device if its using a
different AioContext. Test case included.
Signed-off-by: Alberto Garcia
---
hw/scsi/virtio-scsi.c | 7 +++
tests/qemu-iotests/240 | 22 ++
tests/qemu-iotests/240.out | 20
3 files changed, 49 insertions(+)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e1f7b208c7..eb90288f47 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -791,9 +791,16 @@ static void virtio_scsi_hotplug(HotplugHandler
*hotplug_dev, DeviceState *dev,
SCSIDevice *sd = SCSI_DEVICE(dev);
if (s->ctx && !s->dataplane_fenced) {
+AioContext *ctx;
if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
return;
}
+ctx = blk_get_aio_context(sd->conf.blk);
+if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
+error_setg(errp, "Cannot attach a blockdev that is using "
+ "a different iothread");
+return;
+}
virtio_scsi_acquire(s);
blk_set_aio_context(sd->conf.blk, s->ctx);
virtio_scsi_release(s);
diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
index 5d499c9a00..65cc3b39b1 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -101,6 +101,28 @@ run_qemu <
[Qemu-block] [PATCH 0/3] iothread-related fixes for virtio-scsi
Hi,
here are three patches with iothread-related fixes for virtio-scsi,
with their test cases.
This series fixes the following bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1656276
https://bugzilla.redhat.com/show_bug.cgi?id=1662508
I also wanted to do prepare a similar one for virtio-blk but I'm
unsure about how to proceed, because I'm unable to detect during
realize() that the blockdev is using a different iothread. This
crashes QEMU:
{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name":
"hd0", "read-only": true}}
{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id":
"iothread0"}}
{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id":
"iothread1"}}
{ "execute": "device_add", "arguments": {"id": "virtio-blk0", "driver":
"virtio-blk", "iothread": "iothread0", "drive": "hd0"}}
{ "execute": "system_reset"}
(wait for the events)
{ "execute": "device_add", "arguments": {"id": "virtio-blk1", "driver":
"virtio-blk", "iothread": "iothread1", "drive": "hd0"}}
Regards,
Berto
Alberto Garcia (3):
virtio-scsi: Move BlockBackend back to the main AioContext on unplug
scsi-disk: Acquire the AioContext in scsi_*_realize()
virtio-scsi: Forbid devices with different iothreads sharing a
blockdev
hw/scsi/scsi-disk.c| 23 ++--
hw/scsi/virtio-scsi.c | 13 +
tests/qemu-iotests/240 | 129 +
tests/qemu-iotests/240.out | 54 +++
tests/qemu-iotests/group | 1 +
5 files changed, 217 insertions(+), 3 deletions(-)
create mode 100755 tests/qemu-iotests/240
create mode 100644 tests/qemu-iotests/240.out
--
2.11.0
Re: [Qemu-block] [Qemu-devel] [QEMU PATCH] block: Remove blk_attach_dev_legacy() / legacy_dev code
On 2019-01-22 15:46, Kevin Wolf wrote:
> Am 22.01.2019 um 15:19 hat Thomas Huth geschrieben:
>> On 2018-12-18 17:11, Thomas Huth wrote:
>>> The last user of blk_attach_dev_legacy() is the code in xen_disk.c.
>>> It passes a pointer to a XenBlkDev as second parameter. XenBlkDev
>>> is derived from XenDevice which in turn is derived from DeviceState
>>> since commit 3a6c9172ac5951e ("xen: create qdev for each backend device").
>>> Thus the code can also simply use blk_attach_dev() with a pointer
>>> to the DeviceState instead.
>>> So we can finally remove all code related to the "legacy_dev" flag, too,
>>> and turn the related "void *" in block-backend.c into "DeviceState *"
>>> to fix some of the remaining TODOs there.
>>>
>>> Signed-off-by: Thomas Huth
>>> ---
>>> Note: I haven't tested the Xen code since I don't have a working Xen
>>> installation at hand. I'd appreciate if someone could check it...
>>
>> Ping?
>
> This needs a rebase. xen_disk.c doesn't even exist any more and
> blk_attach_dev_legacy() is really dead code now.
Ah, great, that makes it even easier! I'll send a v2 to remove the
remainders...
Thomas
[Qemu-block] [PATCH 1/3] virtio-scsi: Move BlockBackend back to the main AioContext on unplug
This fixes a crash when attaching a disk to a SCSI device using
iothreads, then detaching it and reattaching it again. Test case
included.
Signed-off-by: Alberto Garcia
---
hw/scsi/virtio-scsi.c | 6
tests/qemu-iotests/240 | 89 ++
tests/qemu-iotests/240.out | 18 ++
tests/qemu-iotests/group | 1 +
4 files changed, 114 insertions(+)
create mode 100755 tests/qemu-iotests/240
create mode 100644 tests/qemu-iotests/240.out
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3aa99717e2..e1f7b208c7 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -824,6 +824,12 @@ static void virtio_scsi_hotunplug(HotplugHandler
*hotplug_dev, DeviceState *dev,
virtio_scsi_release(s);
}
+if (s->ctx) {
+virtio_scsi_acquire(s);
+blk_set_aio_context(sd->conf.blk, qemu_get_aio_context());
+virtio_scsi_release(s);
+}
+
qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
}
diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
new file mode 100755
index 00..ead7ee08eb
--- /dev/null
+++ b/tests/qemu-iotests/240
@@ -0,0 +1,89 @@
+#!/bin/bash
+#
+# Test hot plugging and unplugging with iothreads
+#
+# Copyright (C) 2019 Igalia, S.L.
+# Author: Alberto Garcia
+#
+# 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
[Qemu-block] [PATCH 2/3] scsi-disk: Acquire the AioContext in scsi_*_realize()
This fixes a crash when attaching two disks with the same blockdev to
a SCSI device that is using iothreads. Test case included.
Signed-off-by: Alberto Garcia
---
hw/scsi/scsi-disk.c| 23 ---
tests/qemu-iotests/240 | 18 ++
tests/qemu-iotests/240.out | 16
3 files changed, 54 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0e9027c8f3..b049026219 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2381,10 +2381,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+AioContext *ctx = NULL;
/* can happen for devices without drive. The error message for missing
* backend will be issued in scsi_realize
*/
if (s->qdev.conf.blk) {
+ctx = blk_get_aio_context(s->qdev.conf.blk);
+aio_context_acquire(ctx);
blkconf_blocksizes(&s->qdev.conf);
}
s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -2393,11 +2396,15 @@ static void scsi_hd_realize(SCSIDevice *dev, Error
**errp)
s->product = g_strdup("QEMU HARDDISK");
}
scsi_realize(&s->qdev, errp);
+if (ctx) {
+aio_context_release(ctx);
+}
}
static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+AioContext *ctx;
int ret;
if (!dev->conf.blk) {
@@ -2408,6 +2415,8 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
assert(ret == 0);
}
+ctx = blk_get_aio_context(dev->conf.blk);
+aio_context_acquire(ctx);
s->qdev.blocksize = 2048;
s->qdev.type = TYPE_ROM;
s->features |= 1 << SCSI_DISK_F_REMOVABLE;
@@ -2415,6 +2424,7 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
s->product = g_strdup("QEMU CD-ROM");
}
scsi_realize(&s->qdev, errp);
+aio_context_release(ctx);
}
static void scsi_disk_realize(SCSIDevice *dev, Error **errp)
@@ -2553,6 +2563,7 @@ static int get_device_type(SCSIDiskState *s)
static void scsi_block_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+AioContext *ctx;
int sg_version;
int rc;
@@ -2567,6 +2578,9 @@ static void scsi_block_realize(SCSIDevice *dev, Error
**errp)
"be removed in a future version");
}
+ctx = blk_get_aio_context(s->qdev.conf.blk);
+aio_context_acquire(ctx);
+
/* check we are using a driver managing SG_IO (version 3 and after) */
rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
if (rc < 0) {
@@ -2574,18 +2588,18 @@ static void scsi_block_realize(SCSIDevice *dev, Error
**errp)
if (rc != -EPERM) {
error_append_hint(errp, "Is this a SCSI device?\n");
}
-return;
+goto out;
}
if (sg_version < 3) {
error_setg(errp, "scsi generic interface too old");
-return;
+goto out;
}
/* get device type from INQUIRY data */
rc = get_device_type(s);
if (rc < 0) {
error_setg(errp, "INQUIRY failed");
-return;
+goto out;
}
/* Make a guess for the block size, we'll fix it when the guest sends.
@@ -2605,6 +2619,9 @@ static void scsi_block_realize(SCSIDevice *dev, Error
**errp)
scsi_realize(&s->qdev, errp);
scsi_generic_read_device_inquiry(&s->qdev);
+
+out:
+aio_context_release(ctx);
}
typedef struct SCSIBlockReq {
diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
index ead7ee08eb..5d499c9a00 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -83,6 +83,24 @@ run_qemu <
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()
On Fri 18 Jan 2019 11:14:15 AM CET, Kevin Wolf wrote:
> There are two ways to trigger the crash even without
> x-blockdev-set-iothread:
>
> * device_del, then device_add for a device with iothread (virtio-scsi;
> may or may not exist with virtio-blk)
> https://bugzilla.redhat.com/show_bug.cgi?id=1656276
>
> * Simply attach two devices with iothread to the the same node
> https://bugzilla.redhat.com/show_bug.cgi?id=1662508
While having a look at this I found another crash. Here's how to
reproduce it (wait for the events after each system_reset):
{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name":
"hd0"}}
{ "execute": "device_add", "arguments": {"id": "vb0", "driver":
"virtio-blk", "drive": "hd0"}}
{ "execute": "system_reset"}
{ "execute": "device_del", "arguments": {"id": "vb0"}}
{ "execute": "system_reset"}
{ "execute": "device_add", "arguments": {"id": "vb0", "driver":
"virtio-blk", "drive": "hd0"}}
{ "execute": "system_reset"}
{ "execute": "device_del", "arguments": {"id": "vb0"}}
{ "execute": "system_reset"}
{ "execute": "device_add", "arguments": {"id": "vb0", "driver":
"virtio-blk", "drive": "hd0"}}
{ "execute": "system_reset"}
kvm_mem_ioeventfd_add: error adding ioeventfd: No space left on device
Aborted
git-bisect points to this commit:
commit 3ac7d43a6fbb5d4a3d01fc9a055c218030af3727
Author: Paolo Bonzini
Date: Wed Nov 28 17:28:45 2018 +0100
memory: update coalesced_range on transaction_commit
The e1000 driver calls memory_region_add_coalescing but
kvm_coalesce_mmio_region is never called for those regions. The bug
dates back to the introduction of the memory region API; to fix it,
delete and re-add coalesced MMIO ranges when building the FlatViews.
Because coalesced MMIO regions apply to all address spaces, the
has_coalesced_range flag has to be changed into an int.
Berto
[Qemu-block] [PATCH] block: Apply auto-read-only for ro-whitelist drivers
If QEMU was configured with a driver in --block-drv-ro-whitelist, trying
to use that driver read-write resulted in an error message even if
auto-read-only=on was set.
Consider auto-read-only=on for the whitelist checking and use it to
automatically degrade to read-only for block drivers on the read-only
whitelist.
Signed-off-by: Kevin Wolf
---
block.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 92a3b45a78..0eba8ebe5c 100644
--- a/block.c
+++ b/block.c
@@ -1438,13 +1438,19 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
-error_setg(errp,
- !bs->read_only && bdrv_is_whitelisted(drv, true)
-? "Driver '%s' can only be used for read-only devices"
-: "Driver '%s' is not whitelisted",
- drv->format_name);
-ret = -ENOTSUP;
-goto fail_opts;
+if (!bs->read_only && bdrv_is_whitelisted(drv, true)) {
+ret = bdrv_apply_auto_read_only(bs, NULL, NULL);
+} else {
+ret = -ENOTSUP;
+}
+if (ret < 0) {
+error_setg(errp,
+ !bs->read_only && bdrv_is_whitelisted(drv, true)
+ ? "Driver '%s' can only be used for read-only devices"
+ : "Driver '%s' is not whitelisted",
+ drv->format_name);
+goto fail_opts;
+}
}
/* bdrv_new() and bdrv_close() make it so */
--
2.20.1
Re: [Qemu-block] [Qemu-devel] [QEMU PATCH] block: Remove blk_attach_dev_legacy() / legacy_dev code
Am 22.01.2019 um 15:19 hat Thomas Huth geschrieben:
> On 2018-12-18 17:11, Thomas Huth wrote:
> > The last user of blk_attach_dev_legacy() is the code in xen_disk.c.
> > It passes a pointer to a XenBlkDev as second parameter. XenBlkDev
> > is derived from XenDevice which in turn is derived from DeviceState
> > since commit 3a6c9172ac5951e ("xen: create qdev for each backend device").
> > Thus the code can also simply use blk_attach_dev() with a pointer
> > to the DeviceState instead.
> > So we can finally remove all code related to the "legacy_dev" flag, too,
> > and turn the related "void *" in block-backend.c into "DeviceState *"
> > to fix some of the remaining TODOs there.
> >
> > Signed-off-by: Thomas Huth
> > ---
> > Note: I haven't tested the Xen code since I don't have a working Xen
> > installation at hand. I'd appreciate if someone could check it...
>
> Ping?
This needs a rebase. xen_disk.c doesn't even exist any more and
blk_attach_dev_legacy() is really dead code now.
The work on the Xen block device is the reason why I didn't merge this
patch yet. I thought I had sent a reply to that effect, but it seems I
hadn't. Sorry for that!
Kevin
Re: [Qemu-block] [Qemu-devel] [QEMU PATCH] block: Remove blk_attach_dev_legacy() / legacy_dev code
On 2018-12-18 17:11, Thomas Huth wrote:
> The last user of blk_attach_dev_legacy() is the code in xen_disk.c.
> It passes a pointer to a XenBlkDev as second parameter. XenBlkDev
> is derived from XenDevice which in turn is derived from DeviceState
> since commit 3a6c9172ac5951e ("xen: create qdev for each backend device").
> Thus the code can also simply use blk_attach_dev() with a pointer
> to the DeviceState instead.
> So we can finally remove all code related to the "legacy_dev" flag, too,
> and turn the related "void *" in block-backend.c into "DeviceState *"
> to fix some of the remaining TODOs there.
>
> Signed-off-by: Thomas Huth
> ---
> Note: I haven't tested the Xen code since I don't have a working Xen
> installation at hand. I'd appreciate if someone could check it...
Ping?
> block/block-backend.c | 54
> +++---
> hw/block/xen_disk.c| 6 +++--
> include/sysemu/block-backend.h | 5 ++--
> 3 files changed, 15 insertions(+), 50 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 60d37a0..3c3118f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -47,9 +47,7 @@ struct BlockBackend {
> QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
> BlockBackendPublic public;
>
> -void *dev; /* attached device model, if any */
> -bool legacy_dev;/* true if dev is not a DeviceState */
> -/* TODO change to DeviceState when all users are qdevified */
> +DeviceState *dev; /* attached device model, if any */
> const BlockDevOps *dev_ops;
> void *dev_opaque;
>
> @@ -836,7 +834,11 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm,
> uint64_t *shared_perm)
> *shared_perm = blk->shared_perm;
> }
>
> -static int blk_do_attach_dev(BlockBackend *blk, void *dev)
> +/*
> + * Attach device model @dev to @blk.
> + * Return 0 on success, -EBUSY when a device model is attached already.
> + */
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> {
> if (blk->dev) {
> return -EBUSY;
> @@ -851,40 +853,16 @@ static int blk_do_attach_dev(BlockBackend *blk, void
> *dev)
>
> blk_ref(blk);
> blk->dev = dev;
> -blk->legacy_dev = false;
> blk_iostatus_reset(blk);
>
> return 0;
> }
>
> /*
> - * Attach device model @dev to @blk.
> - * Return 0 on success, -EBUSY when a device model is attached already.
> - */
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> -{
> -return blk_do_attach_dev(blk, dev);
> -}
> -
> -/*
> - * Attach device model @dev to @blk.
> - * @blk must not have a device model attached already.
> - * TODO qdevified devices don't use this, remove when devices are qdevified
> - */
> -void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
> -{
> -if (blk_do_attach_dev(blk, dev) < 0) {
> -abort();
> -}
> -blk->legacy_dev = true;
> -}
> -
> -/*
> * Detach device model @dev from @blk.
> * @dev must be currently attached to @blk.
> */
> -void blk_detach_dev(BlockBackend *blk, void *dev)
> -/* TODO change to DeviceState *dev when all users are qdevified */
> +void blk_detach_dev(BlockBackend *blk, DeviceState *dev)
> {
> assert(blk->dev == dev);
> blk->dev = NULL;
> @@ -898,8 +876,7 @@ void blk_detach_dev(BlockBackend *blk, void *dev)
> /*
> * Return the device model attached to @blk if any, else null.
> */
> -void *blk_get_attached_dev(BlockBackend *blk)
> -/* TODO change to return DeviceState * when all users are qdevified */
> +DeviceState *blk_get_attached_dev(BlockBackend *blk)
> {
> return blk->dev;
> }
> @@ -908,10 +885,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
> * device attached to the BlockBackend. */
> char *blk_get_attached_dev_id(BlockBackend *blk)
> {
> -DeviceState *dev;
> -
> -assert(!blk->legacy_dev);
> -dev = blk->dev;
> +DeviceState *dev = blk->dev;
>
> if (!dev) {
> return g_strdup("");
> @@ -949,11 +923,6 @@ BlockBackend *blk_by_dev(void *dev)
> void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
> void *opaque)
> {
> -/* All drivers that use blk_set_dev_ops() are qdevified and we want to
> keep
> - * it that way, so we can assume blk->dev, if present, is a DeviceState
> if
> - * blk->dev_ops is set. Non-device users may use dev_ops without device.
> */
> -assert(!blk->legacy_dev);
> -
> blk->dev_ops = ops;
> blk->dev_opaque = opaque;
>
> @@ -979,8 +948,6 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool
> load, Error **errp)
> bool tray_was_open, tray_is_open;
> Error *local_err = NULL;
>
> -assert(!blk->legacy_dev);
> -
> tray_was_open = blk_dev_is_tray_open(blk);
> blk->dev_ops->change_media_cb(blk->dev_opaque, load, &local_err);
> if (local_err) {
> @@ -1779,9 +1746,6 @@ void blk_eject(
Re: [Qemu-block] [PATCH] block: don't probe zeroes in bs->file by default on block_status
Am 11.01.2019 um 12:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 11.01.2019 13:41, Kevin Wolf wrote:
> > Am 10.01.2019 um 14:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> drv_co_block_status digs bs->file for additional, more accurate search
> >> for hole inside region, reported as DATA by bs since 5daa74a6ebc.
> >>
> >> This accuracy is not free: assume we have qcow2 disk. Actually, qcow2
> >> knows, where are holes and where is data. But every block_status
> >> request calls lseek additionally. Assume a big disk, full of
> >> data, in any iterative copying block job (or img convert) we'll call
> >> lseek(HOLE) on every iteration, and each of these lseeks will have to
> >> iterate through all metadata up to the end of file. It's obviously
> >> ineffective behavior. And for many scenarios we don't need this lseek
> >> at all.
> >>
> >> So, let's "5daa74a6ebc" by default, leaving an option to return
> >> previous behavior, which is needed for scenarios with preallocated
> >> images.
> >>
> >> Add iotest illustrating new option semantics.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >
> > I still think that an option isn't a good solution and we should try use
> > some heuristics instead.
>
> Do you think that heuristics would be better than fair cache for lseek
> results?
I just played a bit with this (qemu-img convert only), and how much
caching lseek() results helps depends completely on the image. As it
happened, my test image was the worst case where caching didn't buy us
much. Obviously, I can just as easily construct an image where it makes
a huge difference. I think that most real-world images should be able to
take good advantage of it, though, and it doesn't hurt, so maybe that's
a first thing that we can do in any case. It might not be the complete
solution, though.
Let me explain my test images: The case where all of this actually
matters for qemu-img convert is fragmented qcow2 images. If your image
isn't fragmented, we don't do lseek() a lot anyway because a single
bdrv_block_status() call already gives you the information for the whole
image. So I constructed a fragmented image, by writing to it backwards:
./qemu-img create -f qcow2 /tmp/test.qcow2 1G
for i in $(seq 16384 -1 0); do
echo "write $((i * 65536)) 64k"
done | ./qemu-io /tmp/test.qcow2
It's not really surprising that caching the lseek() results doesn't help
much there as we're moving backwards and lseek() only returns results
about the things after the current position, not before the current
position. So this is probably the worst case.
So I constructed a second image, which is fragmented, too, but starts at
the beginning of the image file:
./qemu-img create -f qcow2 /tmp/test_forward.qcow2 1G
for i in $(seq 0 2 16384); do
echo "write $((i * 65536)) 64k"
done | ./qemu-io /tmp/test_forward.qcow2
for i in $(seq 1 2 16384); do
echo "write $((i * 65536)) 64k"
done | ./qemu-io /tmp/test_forward.qcow2
Here caching makes a huge difference:
time ./qemu-img convert -p -n $IMG null-co://
uncachedcached
test.qcow2 ~145s ~70s
test_forward.qcow2 ~110s~0.2s
Not completely sure why there is such a big difference even in the
uncached case, but it seems to be reproducible. I haven't looked into
that more closely.
> I don't see good heuristics, neither want to implement lseek optimizations.
> In our cases we don't need lseek under qcow2 at all, and it's obviously better
> just don't lseek in these cases.
I also did the same thing with an image where I allocated 2 MB chunks
instead of 64k (backwards), and that brings it down to ~3.5s without
caching and ~2s with caching.
So if we implemented the heuristics and lseek caching, maybe we're good?
Kevin
diff --git a/block/file-posix.c b/block/file-posix.c
index 8aee7a3fb8..7272c7c99d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -168,6 +168,12 @@ typedef struct BDRVRawState {
bool needs_alignment;
bool check_cache_dropped;
+struct seek_data_cache {
+boolvalid;
+uint64_tstart;
+uint64_tend;
+} seek_data_cache;
+
PRManager *pr_mgr;
} BDRVRawState;
@@ -1555,8 +1561,17 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
{
RawPosixAIOData *aiocb = opaque;
BDRVRawState *s G_GNUC_UNUSED = aiocb->bs->opaque;
+struct seek_data_cache *sdc;
int ret;
+/* Invalidate seek_data_cache if it overlaps */
+sdc = &s->seek_data_cache;
+if (sdc->valid && !(sdc->end < aiocb->aio_offset ||
+sdc->start > aiocb->aio_offset + aiocb->aio_nbytes))
+{
+sdc->valid = false;
+}
+
/* First try to write zeros and unmap at the same time */
#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
@@ -1634,11 +1649,20 @@ static int handle_aiocb_discard(void *opaque)
RawPosixAIOData *aiocb = opaque;
int ret = -EOPNOTSUPP;
BDRVRawState *s = aiocb->bs->opaqu
[Qemu-block] [PATCH] hw/block: clean up stake xen_disk trace entries
This should have been removed then xen_disk.c was removed but I missed them. Signed-off-by: Paul Durrant --- Cc: Kevin Wolf Cc: Max Reitz --- hw/block/trace-events | 7 --- 1 file changed, 7 deletions(-) diff --git a/hw/block/trace-events b/hw/block/trace-events index 55e5a5500c..d0851953c5 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -121,13 +121,6 @@ nvme_ub_db_wr_invalid_cqhead(uint32_t qid, uint16_t new_head) "completion queue nvme_ub_db_wr_invalid_sq(uint32_t qid) "submission queue doorbell write for nonexistent queue, sqid=%"PRIu32", ignoring" nvme_ub_db_wr_invalid_sqtail(uint32_t qid, uint16_t new_tail) "submission queue doorbell write value beyond queue size, sqid=%"PRIu32", new_head=%"PRIu16", ignoring" -# hw/block/xen_disk.c -xen_disk_alloc(char *name) "%s" -xen_disk_init(char *name) "%s" -xen_disk_connect(char *name) "%s" -xen_disk_disconnect(char *name) "%s" -xen_disk_free(char *name) "%s" - # hw/block/xen-block.c xen_block_realize(const char *type, uint32_t disk, uint32_t partition) "%s d%up%u" xen_block_connect(const char *type, uint32_t disk, uint32_t partition) "%s d%up%u" -- 2.20.1.2.gb21ebb6
Re: [Qemu-block] [PATCH v3 4/9] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
16.01.2019 2:29, Paolo Bonzini wrote:
> On 08/01/19 18:06, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2-threads.c | 10 +++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>> index 20b2616529..156e0667be 100644
>> --- a/block/qcow2-threads.c
>> +++ b/block/qcow2-threads.c
>> @@ -158,15 +158,19 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest,
>> size_t dest_size,
>> .func = func,
>> };
>>
>> +qemu_co_mutex_lock(&s->lock);
>> while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
>> -qemu_co_queue_wait(&s->compress_wait_queue, NULL);
>> +qemu_co_queue_wait(&s->compress_wait_queue, &s->lock);
>> }
>> -
>> s->nb_compress_threads++;
>> +qemu_co_mutex_unlock(&s->lock);
>> +
>> thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
>> -s->nb_compress_threads--;
>>
>> +qemu_co_mutex_lock(&s->lock);
>> +s->nb_compress_threads--;
>> qemu_co_queue_next(&s->compress_wait_queue);
>> +qemu_co_mutex_unlock(&s->lock);
>>
>> return arg.ret;
>> }
>>
>
> Reviewed-by: Paolo Bonzini
>
> but, some information would be nice to have in the commit message.
Could you suggest one?
Just, "To make it thread-safe."? I don't have good understanding
of moving from "AioContext lock usage" to other locks, as well as
I don't know how BlockDriverState is going to be used in parallel
threads, and how much of other code is already prepared to work
in multiple-threads environment.
I looked through your commit messages on patches which added protection
to similar places about co-queues, but didn't find detailed description.
>
> Paolo
>
--
Best regards,
Vladimir
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command
17.01.2019 18:50, Eric Blake wrote:
> On 1/17/19 9:33 AM, Alberto Garcia wrote:
>
>>
>> Changing options
>>
>> The general idea is quite straightforward, but the devil is in the
>> details. Since this command tries to mimic blockdev-add, the state of
>> the block device after being reopened should generally be equivalent
>> to that of a block device added with the same set of options.
>>
>> There are two important things with this:
>>
>>a) Some options cannot be changed (most drivers don't allow that, in
>> fact).
>>b) If an option is missing, it should be reset to its default value
>> (rather than keeping its previous value).
>
> Could we use QAPI alternate types where we could state that:
>
> "option":"new" - set the option
> "option":null - reset the option to its default
> omit option - leave the option unchanged
>
> basically, making each of the options an Alternate with JSON null, so
> that a request to reset to the default becomes an explicit action?
+1
Sorry, I missed the previous discussion. What is the reason to reset option
to default if it missed? It looks more common to not touch things which are
not asked to.
Also, should we consider an option which may be changed during life-cycle of
a device not by user request to change it? If yes, it should be safer to not
reset it.
>
>>
>> Example: the default value of l2-cache-size is 1MB. If we set a
>> different value (2MB) on blockdev-add but then omit the option in
>> x-blockdev-reopen, then it should be reset back to 1MB. The current
>> bdrv_reopen() code keeps the previous value.
>>
>> Trying to change an option that doesn't allow it is already being
>> handled by the code. The loop at the end of bdrv_reopen_prepare()
>> checks all options that were not handled by the block driver and sees
>> if any of them has been modified.
>>
>> However, as I explained earlier in (b), omitting an option is supposed
>> to reset it to its default value, so it's also a way of changing
>> it. If the option cannot be changed then we should detect it and
>> return an error. What I'm doing in this series is making every driver
>> publish its list of run-time options, and which ones of them can be
>> modified.
>>
>> Backing files
>> =
>> This command allows reconfigurations in the node graph. Currently it
>> only allows changing an image's backing file, but it should be
>> possible to implement similar functionalities in drivers that have
>> other children, like blkverify or quorum.
>>
>> Let's add an image with its backing file (hd1 <- hd0) like this:
>>
>> { "execute": "blockdev-add",
>>"arguments": {
>>"driver": "qcow2",
>>"node-name": "hd0",
>>"file": {
>>"driver": "file",
>>"filename": "hd0.qcow2",
>>"node-name": "hd0f"
>>},
>>"backing": {
>>"driver": "qcow2",
>>"node-name": "hd1",
>>"file": {
>>"driver": "file",
>>"filename": "hd1.qcow2",
>>"node-name": "hd1f"
>>}
>>}
>> }
>> }
>>
>> Removing the backing file can be done by simply passing the option {
>> ..., "backing": null } to x-blockdev-reopen.
>>
>> Replacing it is not so straightforward. If we pass something like {
>> ..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
>> assume that we're trying to change the options of the existing backing
>> file (and it will fail in most cases because it'll likely think that
>> we're trying to change the node name, among other options).
>>
>> So I decided to use a reference instead: { ..., "backing": "hd2" },
>> where "hd2" is of an existing node that has been added previously.
>>
>> Leaving out the "backing" option can be ambiguous on some cases (what
>> should it do? keep the current backing file? open the default one
>> specified in the image file?) so x-blockdev-reopen requires that this
>> option is present. For convenience, if the BDS doesn't have a backing
>> file then we allow leaving the option out.
>
> Hmm - that makes my proposal of "option":null as an explicit request to
> the default a bit trickier, if we are already using null for a different
> meaning for backing files.
And here: if we are going to reset to default for not listed options, than
just not listing "backing" should remove it (as default is null, obviously),
and we don't need this special "null" parameter.
Moreover, backing is an example of an option I mentioned, it definitely may
change after block-stream for example, so resetting to default is unsafe,
and user will have to carefully set backing on reopen (not tha backing, that
was used with first blockdev-add, but backing which we have after block-stream)
>
>>
>> As you can see in the patch series, I wrote a relatively large amount
>> of tests for many different scenarios, including some tricky one
