Re: [PATCH v3 1/7] vhost: recheck dev state in the vhost_migration_log routine
On Mon, Aug 31, 2020 at 5:28 AM Dima Stepanov wrote:
>
> vhost-user devices can get a disconnect in the middle of the VHOST-USER
> handshake on the migration start. If disconnect event happened right
> before sending next VHOST-USER command, then the vhost_dev_set_log()
> call in the vhost_migration_log() function will return error. This error
> will lead to the assert() and close the QEMU migration source process.
> For the vhost-user devices the disconnect event should not break the
> migration process, because:
> - the device will be in the stopped state, so it will not be changed
> during migration
> - if reconnect will be made the migration log will be reinitialized as
> part of reconnect/init process:
> #0 vhost_log_global_start (listener=0x563989cf7be0)
> at hw/virtio/vhost.c:920
> #1 0x56398603d8bc in listener_add_address_space
> (listener=0x563989cf7be0,
> as=0x563986ea4340 )
> at softmmu/memory.c:2664
> #2 0x56398603dd30 in memory_listener_register
> (listener=0x563989cf7be0,
> as=0x563986ea4340 )
> at softmmu/memory.c:2740
> #3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8,
> opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER,
> busyloop_timeout=0)
> at hw/virtio/vhost.c:1385
> #4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990)
> at hw/block/vhost-user-blk.c:315
> #5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990,
> event=CHR_EVENT_OPENED)
> at hw/block/vhost-user-blk.c:379
> Update the vhost-user-blk device with the internal started_vu field which
> will be used for initialization (vhost_user_blk_start) and clean up
> (vhost_user_blk_stop). This additional flag in the VhostUserBlk structure
> will be used to track whether the device really needs to be stopped and
> cleaned up on a vhost-user level.
> The disconnect event will set the overall VHOST device (not vhost-user) to
> the stopped state, so it can be used by the general vhost_migration_log
> routine.
> Such approach could be propogated to the other vhost-user devices, but
> better idea is just to make the same connect/disconnect code for all the
> vhost-user devices.
>
> This migration issue was slightly discussed earlier:
> - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html
> - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html
>
> Signed-off-by: Dima Stepanov
Reviewed-by: Raphael Norwitz
> ---
> hw/block/vhost-user-blk.c | 19 ---
> hw/virtio/vhost.c | 27 ---
> include/hw/virtio/vhost-user-blk.h | 10 ++
> 3 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 39aec42..a076b1e 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -150,6 +150,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> error_report("Error starting vhost: %d", -ret);
> goto err_guest_notifiers;
> }
> +s->started_vu = true;
>
> /* guest_notifier_mask/pending not used yet, so just unmask
> * everything here. virtio-pci will do the right thing by
> @@ -175,6 +176,11 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> int ret;
>
> +if (!s->started_vu) {
> +return;
> +}
> +s->started_vu = false;
> +
> if (!k->set_guest_notifiers) {
> return;
> }
> @@ -341,9 +347,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> }
> s->connected = false;
>
> -if (s->dev.started) {
> -vhost_user_blk_stop(vdev);
> -}
> +vhost_user_blk_stop(vdev);
>
> vhost_dev_cleanup(>dev);
> }
> @@ -399,6 +403,15 @@ static void vhost_user_blk_event(void *opaque,
> QEMUChrEvent event)
> NULL, NULL, false);
> aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh,
> opaque);
> }
> +
> +/*
> + * Move vhost device to the stopped state. The vhost-user device
> + * will be clean up and disconnected in BH. This can be useful in
> + * the vhost migration code. If disconnect was caught there is an
> + * option for the general vhost code to get the dev state without
> + * knowing its type (in this case vhost-user).
> + */
> +s->dev.started = false;
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e..ffef7ab 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -861,21 +861,42 @@ static int vhost_migration_log(MemoryListener
> *listener, bool enable)
> dev->log_enabled = enable;
> return 0;
> }
> +
> +r = 0;
> if (!enable) {
> r = vhost_dev_set_log(dev, false);
> if
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov wrote:
>
> On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote:
> > On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov
> > wrote:
> > >
> > > If the vhost-user-blk daemon provides only one virtqueue, but device was
> > > added with several queues, then QEMU will send more VHOST-USER command
> > > than expected by daemon side. The vhost_virtqueue_start() routine
> > > handles such case by checking the return value from the
> > > virtio_queue_get_desc_addr() function call. Add the same check to the
> > > vhost_dev_set_log() routine.
> > >
> > > Signed-off-by: Dima Stepanov
> > > ---
> > > hw/virtio/vhost.c | 12
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index ffef7ab..a33ffd4 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev,
> > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > > {
> > > int r, i, idx;
> > > +hwaddr addr;
> > > +
> > > r = vhost_dev_set_features(dev, enable_log);
> > > if (r < 0) {
> > > goto err_features;
> > > }
> > > for (i = 0; i < dev->nvqs; ++i) {
> > > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > > +addr = virtio_queue_get_desc_addr(dev->vdev, idx);
> > > +if (!addr) {
> > > +/*
> > > + * The queue might not be ready for start. If this
> > > + * is the case there is no reason to continue the process.
> > > + * The similar logic is used by the vhost_virtqueue_start()
> > > + * routine.
> > > + */
> >
> > Shouldn’t we goto err_vq here to reset the logging state of any vqs
> > which have already been set?
> As i understand it, no we shouldn't reset the state of other queues. In
> general it is pretty valid case. Let's assume that the backend
> vhost-user device supports only two queues. But for instance, the QEMU
> command line is using value 4 to define number of virtqueues of such
> device. In this case only 2 queues will be initializaed.
I see - makes more sense now.
>
> I've tried to reflect it in the comment section, that the
> vhost_virtqueue_start() routine has been alread made the same:
> "if a queue isn't ready for start, just return 0 without any error"
> So i made the same here.
>
In your example is a reason why, if queue 3 is uninitialized, queue 4
must also be uninitialized? I realize queue 4 being initialized while
queue 3 is not is a strange case, but it may still make the code more
robust to use a "continue" here instead of a "break". This also seems
more like the logic in vhost_virtqueue_start()/vhost_dev_start().
> I've found this issue, while testing migration with the default
> vhost-user-blk daemon. It fails with assert or sigsegv (don't remember),
> because it receives NULL for the queues it doesn't have. In general
> the daemon should not fall, because of unexpected VHOST_USER
> communication, but also there is no reason for QEMU to send additional
> packets.
>
> >
> > > +break;
> > > +}
> > > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > enable_log);
> > > if (r < 0) {
> > > --
> > > 2.7.4
> > >
> > >
Re: [PATCH v2 0/2] util/hexdump: Cleanup qemu_hexdump()
On 2020/8/23 上午2:09, Philippe Mathieu-Daudé wrote: - Pass const void* buffer - Reorder arguments Supersedes: <20200822150457.1322519-1-f4...@amsat.org> Philippe Mathieu-Daudé (2): util/hexdump: Convert to take a void pointer argument util/hexdump: Reorder qemu_hexdump() arguments include/qemu-common.h| 3 ++- hw/dma/xlnx_dpdma.c | 2 +- hw/net/fsl_etsec/etsec.c | 2 +- hw/net/fsl_etsec/rings.c | 2 +- hw/sd/sd.c | 2 +- hw/usb/redirect.c| 2 +- net/colo-compare.c | 24 net/net.c| 2 +- util/hexdump.c | 4 +++- util/iov.c | 2 +- 10 files changed, 24 insertions(+), 21 deletions(-) Applied. Thanks
Re: [PATCH] hw/ide: check null block pointer before blk_drain
Le jeu. 27 août 2020 13:47, P J P a écrit :
> From: Prasad J Pandit
>
> While cancelling an i/o operation via ide_cancel_dma_sync(),
> check for null block pointer before calling blk_drain(). Avoid
> null pointer dereference.
>
> ->
> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
> ==1803100==Hint: address points to the zero page.
> #0 blk_bs ../block/block-backend.c:714
> #1 blk_drain ../block/block-backend.c:1715
> #2 ide_cancel_dma_sync ../hw/ide/core.c:723
> #3 bmdma_cmd_writeb ../hw/ide/pci.c:298
> #4 bmdma_write ../hw/ide/piix.c:75
> #5 memory_region_write_accessor ../softmmu/memory.c:483
> #6 access_with_adjusted_size ../softmmu/memory.c:544
> #7 memory_region_dispatch_write ../softmmu/memory.c:1465
> #8 flatview_write_continue ../exec.c:3176
> ...
>
> Reported-by: Ruhr-University
> Signed-off-by: Prasad J Pandit
> ---
> hw/ide/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..038af1cd6b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -718,7 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
> * whole DMA operation will be submitted to disk with a single
> * aio operation with preadv/pwritev.
> */
> -if (s->bus->dma->aiocb) {
> +if (s->blk && s->bus->dma->aiocb) {
>
But s->blk mustn't be null here... IMHO we should assert() here and add a
check earlier.
Don't we already have a Launchpad bug for this BTW?
trace_ide_cancel_dma_sync_remaining();
> blk_drain(s->blk);
> assert(s->bus->dma->aiocb == NULL);
> --
> 2.26.2
>
>
>
[PATCH v4 15/18] [automated] Use OBJECT_DECLARE_TYPE where possible
Replace DECLARE_OBJ_CHECKERS with OBJECT_DECLARE_TYPE where the typedefs can be safely removed. Generated running: $ ./scripts/codeconverter/converter.py -i \ --pattern=DeclareObjCheckers $(git grep -l '' -- '*.[ch]') Reviewed-by: Daniel P. Berrangé Signed-off-by: Eduardo Habkost --- Changes v3 -> v4: none Changes v2 -> v3: * Removed hunks due to rebase conflict: include/hw/ppc/xive.h include/hw/arm/armsse.h * Reviewed-by line from Daniel was kept, as no additional hunks are introduced in this version Changes v1 -> v2: * Script re-run after typedefs and macros were moved, and now the patch also touches: - TYPE_ARM_SSE - TYPE_SD_BUS Signed-off-by: Eduardo Habkost --- Cc: "Marc-André Lureau" Cc: Gerd Hoffmann Cc: "Michael S. Tsirkin" Cc: "Daniel P. Berrangé" Cc: Peter Maydell Cc: Corey Minyard Cc: "Cédric Le Goater" Cc: David Gibson Cc: Cornelia Huck Cc: Thomas Huth Cc: Halil Pasic Cc: Christian Borntraeger Cc: "Philippe Mathieu-Daudé" Cc: Alistair Francis Cc: David Hildenbrand Cc: Laurent Vivier Cc: Amit Shah Cc: Stefano Stabellini Cc: Anthony Perard Cc: Paul Durrant Cc: Paolo Bonzini Cc: Fam Zheng Cc: "Gonglei (Arei)" Cc: Eduardo Habkost Cc: Igor Mammedov Cc: Stefan Berger Cc: Richard Henderson Cc: Michael Rolnik Cc: Sarah Harris Cc: "Edgar E. Iglesias" Cc: Michael Walle Cc: Aleksandar Markovic Cc: Aurelien Jarno Cc: Jiaxun Yang Cc: Aleksandar Rikalo Cc: Anthony Green Cc: Chris Wulff Cc: Marek Vasut Cc: Stafford Horne Cc: Palmer Dabbelt Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Yoshinori Sato Cc: Mark Cave-Ayland Cc: Artyom Tarasenko Cc: Guan Xuetao Cc: Max Filippov Cc: qemu-de...@nongnu.org Cc: qemu-...@nongnu.org Cc: qemu-...@nongnu.org Cc: qemu-s3...@nongnu.org Cc: qemu-block@nongnu.org Cc: xen-de...@lists.xenproject.org Cc: qemu-ri...@nongnu.org Signed-off-by: Eduardo Habkost --- hw/audio/intel-hda.h| 6 ++ hw/display/virtio-vga.h | 6 ++ include/authz/base.h| 6 ++ include/authz/list.h| 6 ++ include/authz/listfile.h| 6 ++ include/authz/pamacct.h | 6 ++ include/authz/simple.h | 6 ++ include/crypto/secret_common.h | 6 ++ include/crypto/secret_keyring.h | 6 ++ include/hw/hyperv/vmbus.h | 6 ++ include/hw/i2c/i2c.h| 6 ++ include/hw/i2c/smbus_slave.h| 6 ++ include/hw/ipack/ipack.h| 6 ++ include/hw/ipmi/ipmi.h | 6 ++ include/hw/mem/pc-dimm.h| 6 ++ include/hw/ppc/pnv.h| 6 ++ include/hw/ppc/pnv_core.h | 6 ++ include/hw/ppc/pnv_homer.h | 6 ++ include/hw/ppc/pnv_occ.h| 6 ++ include/hw/ppc/pnv_psi.h| 6 ++ include/hw/ppc/pnv_xive.h | 6 ++ include/hw/ppc/spapr_cpu_core.h | 6 ++ include/hw/ppc/spapr_drc.h | 6 ++ include/hw/ppc/spapr_vio.h | 6 ++ include/hw/ppc/spapr_xive.h | 6 ++ include/hw/ppc/xics.h | 6 ++ include/hw/s390x/event-facility.h | 6 ++ include/hw/s390x/s390_flic.h| 6 ++ include/hw/s390x/sclp.h | 6 ++ include/hw/sd/sd.h | 6 ++ include/hw/ssi/ssi.h| 6 ++ include/hw/sysbus.h | 6 ++ include/hw/virtio/virtio-gpu.h | 6 ++ include/hw/virtio/virtio-input.h| 6 ++ include/hw/virtio/virtio-mem.h | 6 ++ include/hw/virtio/virtio-pmem.h | 6 ++ include/hw/virtio/virtio-serial.h | 6 ++ include/hw/xen/xen-bus.h| 6 ++ include/io/channel.h| 6 ++ include/io/dns-resolver.h | 6 ++ include/io/net-listener.h | 6 ++ include/scsi/pr-manager.h | 6 ++ include/sysemu/cryptodev.h | 6 ++ include/sysemu/hostmem.h| 6 ++ include/sysemu/rng.h| 6 ++ include/sysemu/tpm_backend.h| 6 ++ include/sysemu/vhost-user-backend.h | 6 ++ target/alpha/cpu-qom.h | 6 ++ target/arm/cpu-qom.h| 6 ++ target/avr/cpu-qom.h| 6 ++ target/cris/cpu-qom.h | 6 ++ target/hppa/cpu-qom.h | 6 ++ target/i386/cpu-qom.h | 6 ++ target/lm32/cpu-qom.h | 6 ++ target/m68k/cpu-qom.h | 6 ++ target/microblaze/cpu-qom.h | 6 ++ target/mips/cpu-qom.h | 6 ++ target/moxie/cpu.h | 6 ++ target/nios2/cpu.h | 6 ++ target/openrisc/cpu.h | 6 ++ target/ppc/cpu-qom.h| 6 ++ target/riscv/cpu.h | 6 ++ target/s390x/cpu-qom.h | 6 ++ target/sh4/cpu-qom.h| 6 ++ target/sparc/cpu-qom.h
[PATCH v4 14/18] [semi-automated] Use DECLARE_*CHECKER* when possible (--force mode)
Separate run of the TypeCheckMacro converter using the --force
flag, for the cases where typedefs weren't found in the same
header nor in typedefs.h.
Generated initially using:
$ ./scripts/codeconverter/converter.py --force -i \
--pattern=TypeCheckMacro $(git grep -l '' -- '*.[ch]')
Then each case was manually reviewed, and a comment was added
indicating what's unusual about those type checking
macros/functions. Despite not following the usual pattern, the
changes in this patch were found to be safe.
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Eduardo Habkost
---
Changes v3 -> v4: none
Changes v2 -> v3: none
Changes v1 -> v2:
* Most of the old changes in this patch are now being handled by
the regular TypeCheckMacro patch (without --force mode)
* Added comments added explaining why these unusual changes
remain
---
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: Peter Maydell
Cc: Beniamino Galvani
Cc: Andrew Baumann
Cc: "Philippe Mathieu-Daudé"
Cc: Andrzej Zaborowski
Cc: David Gibson
Cc: qemu-de...@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
---
include/hw/intc/arm_gic.h | 9 +++--
include/hw/intc/arm_gicv3.h | 8 +++-
include/hw/ppc/xics_spapr.h | 4 +++-
include/hw/virtio/virtio-mmio.h | 9 +++--
hw/intc/apic.c | 5 +++--
hw/intc/arm_gic_kvm.c | 9 +++--
hw/intc/arm_gicv3_its_kvm.c | 8 +++-
hw/intc/arm_gicv3_kvm.c | 9 +++--
hw/sd/allwinner-sdhost.c| 5 +++--
hw/sd/bcm2835_sdhost.c | 5 +++--
hw/sd/pxa2xx_mmci.c | 4 +++-
hw/sd/sdhci.c | 4 +++-
12 files changed, 36 insertions(+), 43 deletions(-)
diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
index 704ef2b751..116ccbb5a9 100644
--- a/include/hw/intc/arm_gic.h
+++ b/include/hw/intc/arm_gic.h
@@ -74,12 +74,9 @@
#define TYPE_ARM_GIC "arm_gic"
typedef struct ARMGICClass ARMGICClass;
-#define ARM_GIC(obj) \
- OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
-#define ARM_GIC_CLASS(klass) \
- OBJECT_CLASS_CHECK(ARMGICClass, (klass), TYPE_ARM_GIC)
-#define ARM_GIC_GET_CLASS(obj) \
- OBJECT_GET_CLASS(ARMGICClass, (obj), TYPE_ARM_GIC)
+/* This is reusing the GICState typedef from TYPE_ARM_GIC_COMMON */
+DECLARE_OBJ_CHECKERS(GICState, ARMGICClass,
+ ARM_GIC, TYPE_ARM_GIC)
struct ARMGICClass {
/*< private >*/
diff --git a/include/hw/intc/arm_gicv3.h b/include/hw/intc/arm_gicv3.h
index 58e9131a33..a81a6ae7ec 100644
--- a/include/hw/intc/arm_gicv3.h
+++ b/include/hw/intc/arm_gicv3.h
@@ -17,11 +17,9 @@
#define TYPE_ARM_GICV3 "arm-gicv3"
typedef struct ARMGICv3Class ARMGICv3Class;
-#define ARM_GICV3(obj) OBJECT_CHECK(GICv3State, (obj), TYPE_ARM_GICV3)
-#define ARM_GICV3_CLASS(klass) \
- OBJECT_CLASS_CHECK(ARMGICv3Class, (klass), TYPE_ARM_GICV3)
-#define ARM_GICV3_GET_CLASS(obj) \
- OBJECT_GET_CLASS(ARMGICv3Class, (obj), TYPE_ARM_GICV3)
+/* This is reusing the GICState typedef from TYPE_ARM_GICV3_COMMON */
+DECLARE_OBJ_CHECKERS(GICv3State, ARMGICv3Class,
+ ARM_GICV3, TYPE_ARM_GICV3)
struct ARMGICv3Class {
/*< private >*/
diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
index 09e428de4e..0b8182e40b 100644
--- a/include/hw/ppc/xics_spapr.h
+++ b/include/hw/ppc/xics_spapr.h
@@ -31,7 +31,9 @@
#include "qom/object.h"
#define TYPE_ICS_SPAPR "ics-spapr"
-#define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR)
+/* This is reusing the ICSState typedef from TYPE_ICS */
+DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
+ TYPE_ICS_SPAPR)
int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
Error **errp);
diff --git a/include/hw/virtio/virtio-mmio.h b/include/hw/virtio/virtio-mmio.h
index dca651fd14..6a1c2c20d4 100644
--- a/include/hw/virtio/virtio-mmio.h
+++ b/include/hw/virtio/virtio-mmio.h
@@ -28,12 +28,9 @@
/* QOM macros */
/* virtio-mmio-bus */
#define TYPE_VIRTIO_MMIO_BUS "virtio-mmio-bus"
-#define VIRTIO_MMIO_BUS(obj) \
-OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_MMIO_BUS)
-#define VIRTIO_MMIO_BUS_GET_CLASS(obj) \
-OBJECT_GET_CLASS(VirtioBusClass, (obj), TYPE_VIRTIO_MMIO_BUS)
-#define VIRTIO_MMIO_BUS_CLASS(klass) \
-OBJECT_CLASS_CHECK(VirtioBusClass, (klass), TYPE_VIRTIO_MMIO_BUS)
+/* This is reusing the VirtioBusState typedef from TYPE_VIRTIO_BUS */
+DECLARE_OBJ_CHECKERS(VirtioBusState, VirtioBusClass,
+ VIRTIO_MMIO_BUS, TYPE_VIRTIO_MMIO_BUS)
/* virtio-mmio */
#define TYPE_VIRTIO_MMIO "virtio-mmio"
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index e055bb3af2..b6a05e5439 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -40,8 +40,9 @@
static APICCommonState *local_apics[MAX_APICS + 1];
#define TYPE_APIC "apic"
-#define APIC(obj) \
-OBJECT_CHECK(APICCommonState, (obj), TYPE_APIC)
+/*This is reusing the APICCommonState
Re: [PATCH] block: always link with zlib
Le ven. 28 août 2020 19:33, Paolo Bonzini a écrit :
> The qcow2 driver needs the zlib dependency. While emulators
> provided it through the migration code, this is not true of
> the tools. Move the dependency from the qcow1 rule directly
> into block_ss so that it is included unconditionally.
>
> Fixes build with --disable-qcow1.
>
> Reported-by: Thomas Huth
> Cc: qemu-block@nongnu.org
> Signed-off-by: Paolo Bonzini
>
Reviewed-by: Philippe Mathieu-Daudé
---
> block/meson.build | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/meson.build b/block/meson.build
> index 4dbbfe60b4..a3e56b7cd1 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -40,9 +40,9 @@ block_ss.add(files(
>'vmdk.c',
>'vpc.c',
>'write-threshold.c',
> -), zstd)
> +), zstd, zlib)
>
> -block_ss.add(when: [zlib, 'CONFIG_QCOW1'], if_true: files('qcow.c'))
> +block_ss.add(when: 'CONFIG_QCOW1', if_true: files('qcow.c'))
> block_ss.add(when: 'CONFIG_VDI', if_true: files('vdi.c'))
> block_ss.add(when: 'CONFIG_CLOOP', if_true: files('cloop.c'))
> block_ss.add(when: 'CONFIG_BOCHS', if_true: files('bochs.c'))
> --
> 2.26.2
>
>
>
Re: [PATCH 0/2] Replace posix_fallocate() with falloate()
Patchew URL:
https://patchew.org/QEMU/20200831140127.657134-1-nsof...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200831140127.657134-1-nsof...@redhat.com
Subject: [PATCH 0/2] Replace posix_fallocate() with falloate()
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20200831140127.657134-1-nsof...@redhat.com ->
patchew/20200831140127.657134-1-nsof...@redhat.com
Switched to a new branch 'test'
70e35ed block: file-posix: Replace posix_fallocate with fallocate
35d89d1 block: file-posix: Extract preallocate helpers
=== OUTPUT BEGIN ===
1/2 Checking commit 35d89d1300e4 (block: file-posix: Extract preallocate
helpers)
ERROR: braces {} are necessary for all arms of this statement
#33: FILE: block/file-posix.c:1841:
+if (offset == current_length)
[...]
total: 1 errors, 0 warnings, 234 lines checked
Patch 1/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/2 Checking commit 70e35eda5bc9 (block: file-posix: Replace posix_fallocate
with fallocate)
ERROR: braces {} are necessary for all arms of this statement
#110: FILE: block/file-posix.c:1967:
+if (result != -ENOTSUP)
[...]
total: 1 errors, 0 warnings, 108 lines checked
Patch 2/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200831140127.657134-1-nsof...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[PATCH 2/2] block: file-posix: Replace posix_fallocate with fallocate
If fallocate() is not supported, posix_fallocate() falls back to
inefficient allocation, writing one byte for every 4k bytes[1]. This is
very slow compared with writing zeros. In oVirt we measured ~400%
improvement in allocation time when replacing posix_fallocate() with
manually writing zeroes[2].
We also know that posix_fallocated() does not work well when using OFD
locks[3]. We don't know the reason yet for this issue yet.
Change preallocate_falloc() to use fallocate() instead of
posix_falloate(), and fall back to full preallocation if not supported.
Here are quick test results with this change.
Before (qemu-img-5.1.0-2.fc32.x86_64):
$ time qemu-img create -f raw -o preallocation=falloc /tmp/nfs3/test.raw 6g
Formatting '/tmp/nfs3/test.raw', fmt=raw size=6442450944 preallocation=falloc
real 0m42.100s
user 0m0.602s
sys 0m4.137s
NFS stats:
calls retrans authrefrshwrite
1571583 0 1572205 1571321
After:
$ time ./qemu-img create -f raw -o preallocation=falloc /tmp/nfs3/test.raw 6g
Formatting '/tmp/nfs3/test.raw', fmt=raw size=6442450944 preallocation=falloc
real 0m15.551s
user 0m0.070s
sys 0m2.623s
NFS stats:
calls retrans authrefrshwrite
24620 0 24624 24567
[1]
https://code.woboq.org/userspace/glibc/sysdeps/posix/posix_fallocate.c.html#96
[2] https://bugzilla.redhat.com/1850267#c25
[3] https://bugzilla.redhat.com/1851097
Signed-off-by: Nir Soffer
---
block/file-posix.c | 32 +-
docs/system/qemu-block-drivers.rst.inc | 11 +
docs/tools/qemu-img.rst| 11 +
qapi/block-core.json | 4 ++--
4 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 341ffb1cb4..eac3c0b412 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1835,36 +1835,24 @@ static int allocate_first_block(int fd, size_t max_size)
static int preallocate_falloc(int fd, int64_t current_length, int64_t offset,
Error **errp)
{
-#ifdef CONFIG_POSIX_FALLOCATE
+#ifdef CONFIG_FALLOCATE
int result;
if (offset == current_length)
return 0;
-/*
- * Truncating before posix_fallocate() makes it about twice slower on
- * file systems that do not support fallocate(), trying to check if a
- * block is allocated before allocating it, so don't do that here.
- */
-
-result = -posix_fallocate(fd, current_length,
- offset - current_length);
+result = do_fallocate(fd, 0, current_length, offset - current_length);
if (result != 0) {
-/* posix_fallocate() doesn't set errno. */
-error_setg_errno(errp, -result,
- "Could not preallocate new data");
+error_setg_errno(errp, -result, "Could not preallocate new data");
return result;
}
if (current_length == 0) {
/*
- * posix_fallocate() uses fallocate() if the filesystem supports
- * it, or fallback to manually writing zeroes. If fallocate()
- * was used, unaligned reads from the fallocated area in
- * raw_probe_alignment() will succeed, hence we need to allocate
- * the first block.
+ * Unaligned reads from the fallocated area in raw_probe_alignment()
+ * will succeed, hence we need to allocate the first block.
*
- * Optimize future alignment probing; ignore failures.
+ * Optimizes future alignment probing; ignore failures.
*/
allocate_first_block(fd, offset);
}
@@ -1973,10 +1961,12 @@ static int handle_aiocb_truncate(void *opaque)
}
switch (prealloc) {
-#ifdef CONFIG_POSIX_FALLOCATE
+#ifdef CONFIG_FALLOCATE
case PREALLOC_MODE_FALLOC:
result = preallocate_falloc(fd, current_length, offset, errp);
-goto out;
+if (result != -ENOTSUP)
+goto out;
+/* If fallocate() is not supported, fallback to full preallocation. */
#endif
case PREALLOC_MODE_FULL:
result = preallocate_full(fd, current_length, offset, errp);
@@ -3080,7 +3070,7 @@ static QemuOptsList raw_create_opts = {
.name = BLOCK_OPT_PREALLOC,
.type = QEMU_OPT_STRING,
.help = "Preallocation mode (allowed values: off"
-#ifdef CONFIG_POSIX_FALLOCATE
+#ifdef CONFIG_FALLOCATE
", falloc"
#endif
", full)"
diff --git a/docs/system/qemu-block-drivers.rst.inc
b/docs/system/qemu-block-drivers.rst.inc
index b052a6d14e..8e4acf397e 100644
--- a/docs/system/qemu-block-drivers.rst.inc
+++ b/docs/system/qemu-block-drivers.rst.inc
@@ -25,11 +25,12 @@ This section describes each format and the options that are
supported for it.
.. program:: raw
.. option:: preallocation
-Preallocation mode (allowed values: ``off``, ``falloc``,
-``full``). ``falloc`` mode preallocates space for image by
-
[PATCH 0/2] Replace posix_fallocate() with falloate()
Change preallocation=falloc to use fallocate() instead of posix_fallocte(), improving performance when legacy filesystem that do not support fallocate, and avoiding issues seen with OFD locks. More work is needed to respect cache mode when using full preallocation and maybe optimize buffer size. Continuing the discussion at: https://lists.nongnu.org/archive/html/qemu-block/2020-08/msg00947.html Nir Soffer (2): block: file-posix: Extract preallocate helpers block: file-posix: Replace posix_fallocate with fallocate block/file-posix.c | 202 ++--- docs/system/qemu-block-drivers.rst.inc | 11 +- docs/tools/qemu-img.rst| 11 +- qapi/block-core.json | 4 +- 4 files changed, 127 insertions(+), 101 deletions(-) -- 2.26.2
[PATCH 1/2] block: file-posix: Extract preallocate helpers
handle_aiocb_truncate() was too big and complex, implementing 3
different preallocation modes. In a future patch I want to introduce a
fallback from "falloc" to "full"; it will be too messy and error prone
with the current code.
Extract a helper for each of the preallocation modes (falloc, full, off)
and leave only the common preparation and cleanup code in
handle_aiocb_truncate().
Signed-off-by: Nir Soffer
---
block/file-posix.c | 206 ++---
1 file changed, 120 insertions(+), 86 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..341ffb1cb4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1832,12 +1832,128 @@ static int allocate_first_block(int fd, size_t
max_size)
return ret;
}
+static int preallocate_falloc(int fd, int64_t current_length, int64_t offset,
+ Error **errp)
+{
+#ifdef CONFIG_POSIX_FALLOCATE
+int result;
+
+if (offset == current_length)
+return 0;
+
+/*
+ * Truncating before posix_fallocate() makes it about twice slower on
+ * file systems that do not support fallocate(), trying to check if a
+ * block is allocated before allocating it, so don't do that here.
+ */
+
+result = -posix_fallocate(fd, current_length,
+ offset - current_length);
+if (result != 0) {
+/* posix_fallocate() doesn't set errno. */
+error_setg_errno(errp, -result,
+ "Could not preallocate new data");
+return result;
+}
+
+if (current_length == 0) {
+/*
+ * posix_fallocate() uses fallocate() if the filesystem supports
+ * it, or fallback to manually writing zeroes. If fallocate()
+ * was used, unaligned reads from the fallocated area in
+ * raw_probe_alignment() will succeed, hence we need to allocate
+ * the first block.
+ *
+ * Optimize future alignment probing; ignore failures.
+ */
+allocate_first_block(fd, offset);
+}
+
+return 0;
+#else
+return -ENOTSUP;
+#endif
+}
+
+static int preallocate_full(int fd, int64_t current_length, int64_t offset,
+Error **errp)
+{
+int64_t num = 0, left = offset - current_length;
+off_t seek_result;
+int result;
+char *buf = NULL;
+
+/*
+ * Knowing the final size from the beginning could allow the file
+ * system driver to do less allocations and possibly avoid
+ * fragmentation of the file.
+ */
+if (ftruncate(fd, offset) != 0) {
+result = -errno;
+error_setg_errno(errp, -result, "Could not resize file");
+goto out;
+}
+
+buf = g_malloc0(65536);
+
+seek_result = lseek(fd, current_length, SEEK_SET);
+if (seek_result < 0) {
+result = -errno;
+error_setg_errno(errp, -result,
+ "Failed to seek to the old end of file");
+goto out;
+}
+
+while (left > 0) {
+num = MIN(left, 65536);
+result = write(fd, buf, num);
+if (result < 0) {
+if (errno == EINTR) {
+continue;
+}
+result = -errno;
+error_setg_errno(errp, -result,
+ "Could not write zeros for preallocation");
+goto out;
+}
+left -= result;
+}
+
+result = fsync(fd);
+if (result < 0) {
+result = -errno;
+error_setg_errno(errp, -result, "Could not flush file to disk");
+goto out;
+}
+
+out:
+g_free(buf);
+
+return result;
+}
+
+static int preallocate_off(int fd, int64_t current_length, int64_t offset,
+ Error **errp)
+{
+if (ftruncate(fd, offset) != 0) {
+int result = -errno;
+error_setg_errno(errp, -result, "Could not resize file");
+return result;
+}
+
+if (current_length == 0 && offset > current_length) {
+/* Optimize future alignment probing; ignore failures. */
+allocate_first_block(fd, offset);
+}
+
+return 0;
+}
+
static int handle_aiocb_truncate(void *opaque)
{
RawPosixAIOData *aiocb = opaque;
int result = 0;
int64_t current_length = 0;
-char *buf = NULL;
struct stat st;
int fd = aiocb->aio_fildes;
int64_t offset = aiocb->aio_offset;
@@ -1859,95 +1975,14 @@ static int handle_aiocb_truncate(void *opaque)
switch (prealloc) {
#ifdef CONFIG_POSIX_FALLOCATE
case PREALLOC_MODE_FALLOC:
-/*
- * Truncating before posix_fallocate() makes it about twice slower on
- * file systems that do not support fallocate(), trying to check if a
- * block is allocated before allocating it, so don't do that here.
- */
-if (offset != current_length) {
-result = -posix_fallocate(fd, current_length,
- offset - current_length);
-if
[PATCH v3 6/7] tests/qtest/vhost-user-test: add migrate_reconnect test
Add new migrate_reconnect test for the vhost-user-blk device. Perform a
disconnect after sending response for the VHOST_USER_SET_LOG_BASE
command.
Signed-off-by: Dima Stepanov
---
tests/qtest/vhost-user-test.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index a8af613..4b715d3 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -146,6 +146,7 @@ static VhostUserMsg m __attribute__ ((unused));
enum {
TEST_FLAGS_OK,
TEST_FLAGS_DISCONNECT,
+TEST_FLAGS_MIGRATE_DISCONNECT,
TEST_FLAGS_BAD,
TEST_FLAGS_END,
};
@@ -436,6 +437,15 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE);
g_cond_broadcast(>data_cond);
+/*
+ * Perform disconnect after sending a response. In this
+ * case the next write command on the QEMU side (for now
+ * it is SET_FEATURES will return -1, because of disconnect.
+ */
+if (s->test_flags == TEST_FLAGS_MIGRATE_DISCONNECT) {
+qemu_chr_fe_disconnect(chr);
+s->test_flags = TEST_FLAGS_BAD;
+}
break;
case VHOST_USER_SET_VRING_BASE:
@@ -737,6 +747,17 @@ static void *vhost_user_test_setup_memfd(GString
*cmd_line, void *arg)
return server;
}
+static void *vhost_user_test_setup_migrate_reconnect(GString *cmd_line,
+void *arg)
+{
+TestServer *server;
+
+server = vhost_user_test_setup_memfd(cmd_line, arg);
+server->test_flags = TEST_FLAGS_MIGRATE_DISCONNECT;
+
+return server;
+}
+
static void test_read_guest_mem(void *obj, void *arg, QGuestAllocator *alloc)
{
TestServer *server = arg;
@@ -1150,5 +1171,9 @@ static void register_vhost_user_test(void)
opts.before = vhost_user_test_setup_memfd;
qos_add_test("migrate", "vhost-user-blk",
test_migrate, );
+
+opts.before = vhost_user_test_setup_migrate_reconnect;
+qos_add_test("migrate_reconnect", "vhost-user-blk",
+ test_migrate, );
}
libqos_init(register_vhost_user_test);
--
2.7.4
[PATCH v3 2/7] vhost: check queue state in the vhost_dev_set_log routine
If the vhost-user-blk daemon provides only one virtqueue, but device was
added with several queues, then QEMU will send more VHOST-USER command
than expected by daemon side. The vhost_virtqueue_start() routine
handles such case by checking the return value from the
virtio_queue_get_desc_addr() function call. Add the same check to the
vhost_dev_set_log() routine.
Signed-off-by: Dima Stepanov
---
hw/virtio/vhost.c | 12
1 file changed, 12 insertions(+)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ffef7ab..a33ffd4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
{
int r, i, idx;
+hwaddr addr;
+
r = vhost_dev_set_features(dev, enable_log);
if (r < 0) {
goto err_features;
}
for (i = 0; i < dev->nvqs; ++i) {
idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
+addr = virtio_queue_get_desc_addr(dev->vdev, idx);
+if (!addr) {
+/*
+ * The queue might not be ready for start. If this
+ * is the case there is no reason to continue the process.
+ * The similar logic is used by the vhost_virtqueue_start()
+ * routine.
+ */
+break;
+}
r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
enable_log);
if (r < 0) {
--
2.7.4
[PATCH v3 3/7] tests/qtest/vhost-user-test: prepare the tests for adding new dev class
For now only vhost-user-net device is supported by the test. Other
vhost-user devices are not tested. As a first step make source code
refactoring so new devices can reuse the same test routines. To make
this provide a new vhost_user_ops structure with the methods to
initialize device, its command line or make a proper vhost-user
responses.
Signed-off-by: Dima Stepanov
---
tests/qtest/vhost-user-test.c | 105 ++
1 file changed, 76 insertions(+), 29 deletions(-)
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 9ee0f1e..3df5322 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -135,6 +135,10 @@ enum {
TEST_FLAGS_END,
};
+enum {
+VHOST_USER_NET,
+};
+
typedef struct TestServer {
gchar *socket_path;
gchar *mig_path;
@@ -154,10 +158,25 @@ typedef struct TestServer {
bool test_fail;
int test_flags;
int queues;
+struct vhost_user_ops *vu_ops;
} TestServer;
+struct vhost_user_ops {
+/* Device types. */
+int type;
+void (*append_opts)(TestServer *s, GString *cmd_line,
+const char *chr_opts);
+
+/* VHOST-USER commands. */
+void (*set_features)(TestServer *s, CharBackend *chr,
+VhostUserMsg *msg);
+void (*get_protocol_features)(TestServer *s,
+CharBackend *chr, VhostUserMsg *msg);
+};
+
static const char *init_hugepagefs(void);
-static TestServer *test_server_new(const gchar *name);
+static TestServer *test_server_new(const gchar *name,
+struct vhost_user_ops *ops);
static void test_server_free(TestServer *server);
static void test_server_listen(TestServer *server);
@@ -167,7 +186,7 @@ enum test_memfd {
TEST_MEMFD_NO,
};
-static void append_vhost_opts(TestServer *s, GString *cmd_line,
+static void append_vhost_net_opts(TestServer *s, GString *cmd_line,
const char *chr_opts)
{
g_string_append_printf(cmd_line, QEMU_CMD_CHR QEMU_CMD_NETDEV,
@@ -332,25 +351,15 @@ static void chr_read(void *opaque, const uint8_t *buf,
int size)
break;
case VHOST_USER_SET_FEATURES:
-g_assert_cmpint(msg.payload.u64 & (0x1ULL <<
VHOST_USER_F_PROTOCOL_FEATURES),
-!=, 0ULL);
-if (s->test_flags == TEST_FLAGS_DISCONNECT) {
-qemu_chr_fe_disconnect(chr);
-s->test_flags = TEST_FLAGS_BAD;
+if (s->vu_ops->set_features) {
+s->vu_ops->set_features(s, chr, );
}
break;
case VHOST_USER_GET_PROTOCOL_FEATURES:
-/* send back features to qemu */
-msg.flags |= VHOST_USER_REPLY_MASK;
-msg.size = sizeof(m.payload.u64);
-msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
-msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_CROSS_ENDIAN;
-if (s->queues > 1) {
-msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
+if (s->vu_ops->get_protocol_features) {
+s->vu_ops->get_protocol_features(s, chr, );
}
-p = (uint8_t *)
-qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
case VHOST_USER_GET_VRING_BASE:
@@ -467,7 +476,8 @@ static const char *init_hugepagefs(void)
#endif
}
-static TestServer *test_server_new(const gchar *name)
+static TestServer *test_server_new(const gchar *name,
+struct vhost_user_ops *ops)
{
TestServer *server = g_new0(TestServer, 1);
char template[] = "/tmp/vhost-test-XX";
@@ -495,6 +505,7 @@ static TestServer *test_server_new(const gchar *name)
server->log_fd = -1;
server->queues = 1;
+server->vu_ops = ops;
return server;
}
@@ -669,11 +680,11 @@ static void vhost_user_test_cleanup(void *s)
static void *vhost_user_test_setup(GString *cmd_line, void *arg)
{
-TestServer *server = test_server_new("vhost-user-test");
+TestServer *server = test_server_new("vhost-user-test", arg);
test_server_listen(server);
append_mem_opts(server, cmd_line, 256, TEST_MEMFD_AUTO);
-append_vhost_opts(server, cmd_line, "");
+server->vu_ops->append_opts(server, cmd_line, "");
g_test_queue_destroy(vhost_user_test_cleanup, server);
@@ -682,11 +693,11 @@ static void *vhost_user_test_setup(GString *cmd_line,
void *arg)
static void *vhost_user_test_setup_memfd(GString *cmd_line, void *arg)
{
-TestServer *server = test_server_new("vhost-user-test");
+TestServer *server = test_server_new("vhost-user-test", arg);
test_server_listen(server);
append_mem_opts(server, cmd_line, 256, TEST_MEMFD_YES);
-append_vhost_opts(server, cmd_line, "");
+server->vu_ops->append_opts(server, cmd_line, "");
g_test_queue_destroy(vhost_user_test_cleanup, server);
@@ -720,7 +731,7 @@ static void test_migrate(void *obj, void *arg,
QGuestAllocator *alloc)
return;
}
-dest = test_server_new("dest");
+dest =
[PATCH v3 0/7] vhost-user-blk: fix the migration issue and enhance qtests
v2 -> v3: - update commit message for the "vhost: recheck dev state in the vhost_migration_log routine" commit - rename "started" field of the VhostUserBlk structure to "started_vu", so there will be no confustion with the VHOST started field - update vhost-user-test.c to always initialize nq local variable (spotted by patchew) v1 -> v2: - add comments to connected/started fields in the header file - move the "s->started" logic from the vhost_user_blk_disconnect routine to the vhost_user_blk_stop routine Reference e-mail threads: - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html If vhost-user daemon is used as a backend for the vhost device, then we should consider a possibility of disconnect at any moment. There was a general question here: should we consider it as an error or okay state for the vhost-user devices during migration process? I think the disconnect event for the vhost-user devices should not break the migration process, because: - the device will be in the stopped state, so it will not be changed during migration - if reconnect will be made the migration log will be reinitialized as part of reconnect/init process: #0 vhost_log_global_start (listener=0x563989cf7be0) at hw/virtio/vhost.c:920 #1 0x56398603d8bc in listener_add_address_space (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2664 #2 0x56398603dd30 in memory_listener_register (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2740 #3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8, opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER, busyloop_timeout=0) at hw/virtio/vhost.c:1385 #4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990) at hw/block/vhost-user-blk.c:315 #5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990, event=CHR_EVENT_OPENED) at hw/block/vhost-user-blk.c:379 The first patch in the patchset fixes this issue by setting vhost device to the stopped state in the disconnect handler and check it the vhost_migration_log() routine before returning from the function. qtest framework was updated to test vhost-user-blk functionality. The vhost-user-blk/vhost-user-blk-tests/migrate_reconnect test was added to reproduce the original issue found. Dima Stepanov (7): vhost: recheck dev state in the vhost_migration_log routine vhost: check queue state in the vhost_dev_set_log routine tests/qtest/vhost-user-test: prepare the tests for adding new dev class tests/qtest/libqos/virtio-blk: add support for vhost-user-blk tests/qtest/vhost-user-test: add support for the vhost-user-blk device tests/qtest/vhost-user-test: add migrate_reconnect test tests/qtest/vhost-user-test: enable the reconnect tests hw/block/vhost-user-blk.c | 19 ++- hw/virtio/vhost.c | 39 - include/hw/virtio/vhost-user-blk.h | 10 ++ tests/qtest/libqos/virtio-blk.c| 14 ++ tests/qtest/vhost-user-test.c | 290 +++-- 5 files changed, 323 insertions(+), 49 deletions(-) -- 2.7.4
[PATCH v3 4/7] tests/qtest/libqos/virtio-blk: add support for vhost-user-blk
Add support for the vhost-user-blk-pci device. This node can be used by
the vhost-user-blk tests. Tests for the vhost-user-blk device are added
in the following patches.
Signed-off-by: Dima Stepanov
---
tests/qtest/libqos/virtio-blk.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tests/qtest/libqos/virtio-blk.c b/tests/qtest/libqos/virtio-blk.c
index 5da0259..959c5dc 100644
--- a/tests/qtest/libqos/virtio-blk.c
+++ b/tests/qtest/libqos/virtio-blk.c
@@ -36,6 +36,9 @@ static void *qvirtio_blk_get_driver(QVirtioBlk *v_blk,
if (!g_strcmp0(interface, "virtio")) {
return v_blk->vdev;
}
+if (!g_strcmp0(interface, "vhost-user-blk")) {
+return v_blk;
+}
fprintf(stderr, "%s not present in virtio-blk-device\n", interface);
g_assert_not_reached();
@@ -120,6 +123,17 @@ static void virtio_blk_register_nodes(void)
qos_node_produces("virtio-blk-pci", "virtio-blk");
g_free(arg);
+
+/* vhost-user-blk-pci */
+arg = g_strdup_printf("id=drv0,chardev=chdev0,addr=%x.%x",
+PCI_SLOT, PCI_FN);
+opts.extra_device_opts = arg;
+add_qpci_address(, );
+qos_node_create_driver("vhost-user-blk-pci", virtio_blk_pci_create);
+qos_node_consumes("vhost-user-blk-pci", "pci-bus", );
+qos_node_produces("vhost-user-blk-pci", "vhost-user-blk");
+
+g_free(arg);
}
libqos_init(virtio_blk_register_nodes);
--
2.7.4
[PATCH v3 5/7] tests/qtest/vhost-user-test: add support for the vhost-user-blk device
Add vhost_user_ops structure for the vhost-user-blk device class. Add
the test_reconnect and test_migrate tests for this device.
Signed-off-by: Dima Stepanov
---
tests/qtest/vhost-user-test.c | 139 +-
1 file changed, 137 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 3df5322..a8af613 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -24,6 +24,7 @@
#include "libqos/libqos.h"
#include "libqos/pci-pc.h"
#include "libqos/virtio-pci.h"
+#include "libqos/virtio-blk.h"
#include "libqos/malloc-pc.h"
#include "hw/virtio/virtio-net.h"
@@ -31,6 +32,7 @@
#include "standard-headers/linux/vhost_types.h"
#include "standard-headers/linux/virtio_ids.h"
#include "standard-headers/linux/virtio_net.h"
+#include "standard-headers/linux/virtio_blk.h"
#ifdef CONFIG_LINUX
#include
@@ -43,6 +45,7 @@
" -numa node,memdev=mem"
#define QEMU_CMD_CHR" -chardev socket,id=%s,path=%s%s"
#define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce"
+#define QEMU_CMD_BLKCHR " -chardev socket,id=chdev0,path=%s%s"
#define HUGETLBFS_MAGIC 0x958458f6
@@ -55,6 +58,7 @@
#define VHOST_USER_PROTOCOL_F_MQ 0
#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
#define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6
+#define VHOST_USER_PROTOCOL_F_CONFIG 9
#define VHOST_LOG_PAGE 0x1000
@@ -78,6 +82,8 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_PROTOCOL_FEATURES = 16,
VHOST_USER_GET_QUEUE_NUM = 17,
VHOST_USER_SET_VRING_ENABLE = 18,
+VHOST_USER_GET_CONFIG = 24,
+VHOST_USER_SET_CONFIG = 25,
VHOST_USER_MAX
} VhostUserRequest;
@@ -99,6 +105,14 @@ typedef struct VhostUserLog {
uint64_t mmap_offset;
} VhostUserLog;
+#define VHOST_USER_MAX_CONFIG_SIZE 256
+typedef struct VhostUserConfig {
+uint32_t offset;
+uint32_t size;
+uint32_t flags;
+uint8_t region[VHOST_USER_MAX_CONFIG_SIZE];
+} VhostUserConfig;
+
typedef struct VhostUserMsg {
VhostUserRequest request;
@@ -114,6 +128,7 @@ typedef struct VhostUserMsg {
struct vhost_vring_addr addr;
VhostUserMemory memory;
VhostUserLog log;
+VhostUserConfig config;
} payload;
} QEMU_PACKED VhostUserMsg;
@@ -137,6 +152,7 @@ enum {
enum {
VHOST_USER_NET,
+VHOST_USER_BLK,
};
typedef struct TestServer {
@@ -166,12 +182,15 @@ struct vhost_user_ops {
int type;
void (*append_opts)(TestServer *s, GString *cmd_line,
const char *chr_opts);
+void (*driver_init)(void *obj, QGuestAllocator *alloc);
/* VHOST-USER commands. */
void (*set_features)(TestServer *s, CharBackend *chr,
VhostUserMsg *msg);
void (*get_protocol_features)(TestServer *s,
CharBackend *chr, VhostUserMsg *msg);
+void (*get_config)(TestServer *s, CharBackend *chr,
+VhostUserMsg *msg);
};
static const char *init_hugepagefs(void);
@@ -194,6 +213,14 @@ static void append_vhost_net_opts(TestServer *s, GString
*cmd_line,
chr_opts, s->chr_name);
}
+static void append_vhost_blk_opts(TestServer *s, GString *cmd_line,
+ const char *chr_opts)
+{
+g_string_append_printf(cmd_line, QEMU_CMD_BLKCHR,
+ s->socket_path,
+ chr_opts);
+}
+
static void append_mem_opts(TestServer *server, GString *cmd_line,
int size, enum test_memfd memfd)
{
@@ -425,6 +452,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
+case VHOST_USER_GET_CONFIG:
+if (s->vu_ops->get_config) {
+s->vu_ops->get_config(s, chr, );
+}
+break;
+
default:
break;
}
@@ -727,6 +760,9 @@ static void test_migrate(void *obj, void *arg,
QGuestAllocator *alloc)
guint8 *log;
guint64 size;
+if (s->vu_ops->driver_init) {
+s->vu_ops->driver_init(obj, alloc);
+}
if (!wait_for_fds(s)) {
return;
}
@@ -796,6 +832,24 @@ static void test_migrate(void *obj, void *arg,
QGuestAllocator *alloc)
g_string_free(dest_cmdline, true);
}
+static void vu_blk_driver_init(void *obj, QGuestAllocator *alloc)
+{
+QVirtioBlk *blk_if;
+QVirtioDevice *dev;
+QVirtQueue *vq;
+uint64_t features;
+
+blk_if = obj;
+dev = blk_if->vdev;
+features = qvirtio_get_features(dev);
+qvirtio_set_features(dev, features);
+
+vq = qvirtqueue_setup(dev, alloc, 0);
+g_assert(vq);
+
+qvirtio_set_driver_ok(dev);
+}
+
static void wait_for_rings_started(TestServer *s, size_t count)
{
gint64 end_time;
@@ -857,12 +911,21 @@ static void test_reconnect(void *obj, void *arg,
QGuestAllocator *alloc)
{
TestServer *s = arg;
GSource *src;
+int nq;
+
[PATCH v3 7/7] tests/qtest/vhost-user-test: enable the reconnect tests
For now a QTEST_VHOST_USER_FIXME environment variable is used to
separate reconnect tests for the vhost-user-net device. Looks like the
reconnect functionality is pretty stable, so this separation is
deprecated.
Remove it and enable these tests for the default run.
Signed-off-by: Dima Stepanov
---
tests/qtest/vhost-user-test.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 4b715d3..4b96312 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -1140,20 +1140,17 @@ static void register_vhost_user_test(void)
"virtio-net",
test_migrate, );
-/* keeps failing on build-system since Aug 15 2017 */
-if (getenv("QTEST_VHOST_USER_FIXME")) {
-opts.before = vhost_user_test_setup_reconnect;
-qos_add_test("vhost-user/reconnect", "virtio-net",
- test_reconnect, );
-
-opts.before = vhost_user_test_setup_connect_fail;
-qos_add_test("vhost-user/connect-fail", "virtio-net",
- test_vhost_user_started, );
-
-opts.before = vhost_user_test_setup_flags_mismatch;
-qos_add_test("vhost-user/flags-mismatch", "virtio-net",
- test_vhost_user_started, );
-}
+opts.before = vhost_user_test_setup_reconnect;
+qos_add_test("vhost-user/reconnect", "virtio-net",
+ test_reconnect, );
+
+opts.before = vhost_user_test_setup_connect_fail;
+qos_add_test("vhost-user/connect-fail", "virtio-net",
+ test_vhost_user_started, );
+
+opts.before = vhost_user_test_setup_flags_mismatch;
+qos_add_test("vhost-user/flags-mismatch", "virtio-net",
+ test_vhost_user_started, );
opts.before = vhost_user_test_setup_multiqueue;
opts.edge.extra_device_opts = "mq=on";
--
2.7.4
[PATCH v3 1/7] vhost: recheck dev state in the vhost_migration_log routine
vhost-user devices can get a disconnect in the middle of the VHOST-USER
handshake on the migration start. If disconnect event happened right
before sending next VHOST-USER command, then the vhost_dev_set_log()
call in the vhost_migration_log() function will return error. This error
will lead to the assert() and close the QEMU migration source process.
For the vhost-user devices the disconnect event should not break the
migration process, because:
- the device will be in the stopped state, so it will not be changed
during migration
- if reconnect will be made the migration log will be reinitialized as
part of reconnect/init process:
#0 vhost_log_global_start (listener=0x563989cf7be0)
at hw/virtio/vhost.c:920
#1 0x56398603d8bc in listener_add_address_space
(listener=0x563989cf7be0,
as=0x563986ea4340 )
at softmmu/memory.c:2664
#2 0x56398603dd30 in memory_listener_register (listener=0x563989cf7be0,
as=0x563986ea4340 )
at softmmu/memory.c:2740
#3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8,
opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER,
busyloop_timeout=0)
at hw/virtio/vhost.c:1385
#4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990)
at hw/block/vhost-user-blk.c:315
#5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990,
event=CHR_EVENT_OPENED)
at hw/block/vhost-user-blk.c:379
Update the vhost-user-blk device with the internal started_vu field which
will be used for initialization (vhost_user_blk_start) and clean up
(vhost_user_blk_stop). This additional flag in the VhostUserBlk structure
will be used to track whether the device really needs to be stopped and
cleaned up on a vhost-user level.
The disconnect event will set the overall VHOST device (not vhost-user) to
the stopped state, so it can be used by the general vhost_migration_log
routine.
Such approach could be propogated to the other vhost-user devices, but
better idea is just to make the same connect/disconnect code for all the
vhost-user devices.
This migration issue was slightly discussed earlier:
- https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html
- https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html
Signed-off-by: Dima Stepanov
---
hw/block/vhost-user-blk.c | 19 ---
hw/virtio/vhost.c | 27 ---
include/hw/virtio/vhost-user-blk.h | 10 ++
3 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 39aec42..a076b1e 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -150,6 +150,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
error_report("Error starting vhost: %d", -ret);
goto err_guest_notifiers;
}
+s->started_vu = true;
/* guest_notifier_mask/pending not used yet, so just unmask
* everything here. virtio-pci will do the right thing by
@@ -175,6 +176,11 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret;
+if (!s->started_vu) {
+return;
+}
+s->started_vu = false;
+
if (!k->set_guest_notifiers) {
return;
}
@@ -341,9 +347,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
}
s->connected = false;
-if (s->dev.started) {
-vhost_user_blk_stop(vdev);
-}
+vhost_user_blk_stop(vdev);
vhost_dev_cleanup(>dev);
}
@@ -399,6 +403,15 @@ static void vhost_user_blk_event(void *opaque,
QEMUChrEvent event)
NULL, NULL, false);
aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
}
+
+/*
+ * Move vhost device to the stopped state. The vhost-user device
+ * will be clean up and disconnected in BH. This can be useful in
+ * the vhost migration code. If disconnect was caught there is an
+ * option for the general vhost code to get the dev state without
+ * knowing its type (in this case vhost-user).
+ */
+s->dev.started = false;
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1a1384e..ffef7ab 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -861,21 +861,42 @@ static int vhost_migration_log(MemoryListener *listener,
bool enable)
dev->log_enabled = enable;
return 0;
}
+
+r = 0;
if (!enable) {
r = vhost_dev_set_log(dev, false);
if (r < 0) {
-return r;
+goto check_dev_state;
}
vhost_log_put(dev, false);
} else {
vhost_dev_log_resize(dev, vhost_get_log_size(dev));
r = vhost_dev_set_log(dev, true);
if (r < 0) {
-return r;
+goto check_dev_state;
Re: [PATCH v2 0/7] qemu: implementation of transient disk option
[FYI pkrempa is on PTO this week so he won't review this until the next one] On a Friday in 2020, Masayoshi Mizuma wrote: This patchset tries to implement transient option for qcow2 and raw format disk. This uses the snapshot cleanup codes: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html It gets user available to set to the domain xml file like as: Any changes which the Guest does to the disk is dropped when the Guest is shutdowned. There are some limitations for transient disk option so far: - Supported disk format is qcow2 and raw - blockdev capability is required for qemu - Following features are blocked with transient disk option - blockjobs - Migration - Disk hotplug Masayoshi Mizuma (7): qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL qemu: Introduce functions to handle transient disk qemu: Add transient disk handler to start and stop the guest qemu: Transient option gets avaiable for qcow2 and raw format disk qemu: Block blockjobs when transient disk option is enabled qemu: Block migration when transient disk option is enabled qemu: Block disk hotplug when transient disk option is enabled src/qemu/qemu_domain.c| 7 ++ src/qemu/qemu_hotplug.c | 6 ++ src/qemu/qemu_migration.c | 22 ++ src/qemu/qemu_process.c | 8 +++ src/qemu/qemu_snapshot.c | 139 +- src/qemu/qemu_snapshot.h | 8 +++ src/qemu/qemu_validate.c | 9 ++- src/util/virstoragefile.h | 2 + 8 files changed, 196 insertions(+), 5 deletions(-) -- 2.27.0 signature.asc Description: PGP signature
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote:
> On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov wrote:
> >
> > If the vhost-user-blk daemon provides only one virtqueue, but device was
> > added with several queues, then QEMU will send more VHOST-USER command
> > than expected by daemon side. The vhost_virtqueue_start() routine
> > handles such case by checking the return value from the
> > virtio_queue_get_desc_addr() function call. Add the same check to the
> > vhost_dev_set_log() routine.
> >
> > Signed-off-by: Dima Stepanov
> > ---
> > hw/virtio/vhost.c | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index ffef7ab..a33ffd4 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev
> > *dev,
> > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > {
> > int r, i, idx;
> > +hwaddr addr;
> > +
> > r = vhost_dev_set_features(dev, enable_log);
> > if (r < 0) {
> > goto err_features;
> > }
> > for (i = 0; i < dev->nvqs; ++i) {
> > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > +addr = virtio_queue_get_desc_addr(dev->vdev, idx);
> > +if (!addr) {
> > +/*
> > + * The queue might not be ready for start. If this
> > + * is the case there is no reason to continue the process.
> > + * The similar logic is used by the vhost_virtqueue_start()
> > + * routine.
> > + */
>
> Shouldn’t we goto err_vq here to reset the logging state of any vqs
> which have already been set?
As i understand it, no we shouldn't reset the state of other queues. In
general it is pretty valid case. Let's assume that the backend
vhost-user device supports only two queues. But for instance, the QEMU
command line is using value 4 to define number of virtqueues of such
device. In this case only 2 queues will be initializaed.
I've tried to reflect it in the comment section, that the
vhost_virtqueue_start() routine has been alread made the same:
"if a queue isn't ready for start, just return 0 without any error"
So i made the same here.
I've found this issue, while testing migration with the default
vhost-user-blk daemon. It fails with assert or sigsegv (don't remember),
because it receives NULL for the queues it doesn't have. In general
the daemon should not fall, because of unexpected VHOST_USER
communication, but also there is no reason for QEMU to send additional
packets.
>
> > +break;
> > +}
> > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > enable_log);
> > if (r < 0) {
> > --
> > 2.7.4
> >
> >
Re: [PATCH] block: always link with zlib
On 28/08/2020 19.32, Paolo Bonzini wrote:
> The qcow2 driver needs the zlib dependency. While emulators
> provided it through the migration code, this is not true of
> the tools. Move the dependency from the qcow1 rule directly
> into block_ss so that it is included unconditionally.
>
> Fixes build with --disable-qcow1.
>
> Reported-by: Thomas Huth
> Cc: qemu-block@nongnu.org
> Signed-off-by: Paolo Bonzini
> ---
> block/meson.build | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/meson.build b/block/meson.build
> index 4dbbfe60b4..a3e56b7cd1 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -40,9 +40,9 @@ block_ss.add(files(
>'vmdk.c',
>'vpc.c',
>'write-threshold.c',
> -), zstd)
> +), zstd, zlib)
>
> -block_ss.add(when: [zlib, 'CONFIG_QCOW1'], if_true: files('qcow.c'))
> +block_ss.add(when: 'CONFIG_QCOW1', if_true: files('qcow.c'))
> block_ss.add(when: 'CONFIG_VDI', if_true: files('vdi.c'))
> block_ss.add(when: 'CONFIG_CLOOP', if_true: files('cloop.c'))
> block_ss.add(when: 'CONFIG_BOCHS', if_true: files('bochs.c'))
Reviewed-by: Thomas Huth
Re: [PATCH v2 1/7] vhost: recheck dev state in the vhost_migration_log routine
On Thu, Aug 27, 2020 at 09:44:22PM -0400, Raphael Norwitz wrote:
> Generally I’m happy with this. Some comments:
>
> On Mon, Aug 24, 2020 at 4:40 AM Dima Stepanov wrote:
> >
> > vhost-user devices can get a disconnect in the middle of the VHOST-USER
> > handshake on the migration start. If disconnect event happened right
> > before sending next VHOST-USER command, then the vhost_dev_set_log()
> > call in the vhost_migration_log() function will return error. This error
> > will lead to the assert() and close the QEMU migration source process.
> > For the vhost-user devices the disconnect event should not break the
> > migration process, because:
> > - the device will be in the stopped state, so it will not be changed
> > during migration
> > - if reconnect will be made the migration log will be reinitialized as
> > part of reconnect/init process:
> > #0 vhost_log_global_start (listener=0x563989cf7be0)
> > at hw/virtio/vhost.c:920
> > #1 0x56398603d8bc in listener_add_address_space
> > (listener=0x563989cf7be0,
> > as=0x563986ea4340 )
> > at softmmu/memory.c:2664
> > #2 0x56398603dd30 in memory_listener_register
> > (listener=0x563989cf7be0,
> > as=0x563986ea4340 )
> > at softmmu/memory.c:2740
> > #3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8,
> > opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER,
> > busyloop_timeout=0)
> > at hw/virtio/vhost.c:1385
> > #4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990)
> > at hw/block/vhost-user-blk.c:315
> > #5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990,
> > event=CHR_EVENT_OPENED)
> > at hw/block/vhost-user-blk.c:379
> > Update the vhost-user-blk device with the internal started field which
> > will be used for initialization and clean up. The disconnect event will
> > set the overall VHOST device to the stopped state, so it can be used by
> > the vhost_migration_log routine.
> > Such approach could be propogated to the other vhost-user devices, but
> > better idea is just to make the same connect/disconnect code for all the
> > vhost-user devices.
> >
> > This migration issue was slightly discussed earlier:
> > - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html
> > - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html
> >
>
> What you’re doing here on a structural level is adding an additional
> flag “started” to VhostUserBlk to track whether the device really
> needs to be stopped and cleaned up on a vhost level on a disconnect.
> Can you make that more clear in the commit message as you have in the
> comments?
Sounds reasonable, will update the commit message.
>
>
> > Signed-off-by: Dima Stepanov
> > ---
> > hw/block/vhost-user-blk.c | 19 ---
> > hw/virtio/vhost.c | 27 ---
> > include/hw/virtio/vhost-user-blk.h | 10 ++
> > 3 files changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index a00b854..5573e89 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -150,6 +150,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> > error_report("Error starting vhost: %d", -ret);
> > goto err_guest_notifiers;
> > }
> > +s->started = true;
> >
> > /* guest_notifier_mask/pending not used yet, so just unmask
> > * everything here. virtio-pci will do the right thing by
> > @@ -175,6 +176,11 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> > VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > int ret;
> >
> > +if (!s->started) {
> > +return;
> > +}
> > +s->started = false;
> > +
> > if (!k->set_guest_notifiers) {
> > return;
> > }
> > @@ -341,9 +347,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> > }
> > s->connected = false;
> >
> > -if (s->dev.started) {
> > -vhost_user_blk_stop(vdev);
> > -}
> > +vhost_user_blk_stop(vdev);
> >
> > vhost_dev_cleanup(>dev);
> > }
> > @@ -399,6 +403,15 @@ static void vhost_user_blk_event(void *opaque,
> > QEMUChrEvent event)
> > NULL, NULL, false);
> > aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh,
> > opaque);
> > }
> > +
> > +/*
> > + * Move vhost device to the stopped state. The vhost-user device
> > + * will be clean up and disconnected in BH. This can be useful in
> > + * the vhost migration code. If disconnect was caught there is an
> > + * option for the general vhost code to get the dev state without
> > + * knowing its type (in this case vhost-user).
> > + */
> > +s->dev.started = false;
> > break;
> > case CHR_EVENT_BREAK:
> > case CHR_EVENT_MUX_IN:
> >
Re: [PATCH v7 1/8] Introduce yank feature
Lukas Straub writes:
> On Thu, 27 Aug 2020 14:37:00 +0200
> Markus Armbruster wrote:
>
>> I apologize for not reviewing this much earlier.
>>
>> Lukas Straub writes:
>>
>> > The yank feature allows to recover from hanging qemu by "yanking"
>> > at various parts. Other qemu systems can register themselves and
>> > multiple yank functions. Then all yank functions for selected
>> > instances can be called by the 'yank' out-of-band qmp command.
>> > Available instances can be queried by a 'query-yank' oob command.
>> >
>> > Signed-off-by: Lukas Straub
>> > Acked-by: Stefan Hajnoczi
[...]
>> > diff --git a/qapi/misc.json b/qapi/misc.json
>> > index 9d32820dc1..0d6a8f20b7 100644
>> > --- a/qapi/misc.json
>> > +++ b/qapi/misc.json
>> > @@ -1615,3 +1615,48 @@
>> > ##
>> > { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
>> >
>> > +##
>> > +# @YankInstances:
>> > +#
>> > +# @instances: List of yank instances.
>> > +#
>> > +# Yank instances are named after the following schema:
>> > +# "blockdev:", "chardev:" and "migration"
>> > +#
>> > +# Since: 5.1
>> > +##
>> > +{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }
>>
>> I'm afraid this is a problematic QMP interface.
>>
>> By making YankInstances a struct, you keep the door open to adding more
>> members, which is good.
>>
>> But by making its 'instances' member a ['str'], you close the door to
>> using anything but a single string for the individual instances. Not so
>> good.
>>
>> The single string encodes information which QMP client will need to
>> parse from the string. We frown on that in QMP. Use QAPI complex types
>> capabilities for structured data.
>>
>> Could you use something like this instead?
>>
>> { 'enum': 'YankInstanceType',
>> 'data': { 'block-node', 'chardev', 'migration' } }
>>
>> { 'struct': 'YankInstanceBlockNode',
>> 'data': { 'node-name': 'str' } }
>>
>> { 'struct': 'YankInstanceChardev',
>> 'data' { 'label': 'str' } }
>>
>> { 'union': 'YankInstance',
>> 'base': { 'type': 'YankInstanceType' },
>> 'discriminator': 'type',
>> 'data': {
>> 'block-node': 'YankInstanceBlockNode',
>> 'chardev': 'YankInstanceChardev' } }
>>
>> { 'command': 'yank',
>> 'data': { 'instances': ['YankInstance'] },
>> 'allow-oob': true }
>>
>> If you're confident nothing will ever be added to YankInstanceBlockNode
>> and YankInstanceChardev, you could use str instead.
>
> As Daniel said, this has already been discussed.
I'll look up that discussion.
[...]
>> The two QMP commands permit out-of-band execution ('allow-oob': true).
>> OOB is easy to get wrong, but I figure you have a legitimate use case.
>> Let's review the restrictions documented in
>> docs/devel/qapi-code-gen.txt:
>>
>> An OOB-capable command handler must satisfy the following conditions:
>>
>> - It terminates quickly.
>> - It does not invoke system calls that may block.
>> - It does not access guest RAM that may block when userfaultfd is
>> enabled for postcopy live migration.
>> - It takes only "fast" locks, i.e. all critical sections protected by
>> any lock it takes also satisfy the conditions for OOB command
>> handler code.
>>
>> Since the command handlers take , the restrictions apply to the
>> other critical sections protected by as well. I believe these are
>> all okay: they do nothing but allocate, initialize and free memory.
>>
>> The restrictions also apply to the YankFn callbacks, but you documented
>> that. Okay.
>>
>> The one such callback included in this patch is
>> yank_generic_iochannel(), which is a thin wrapper around
>> qio_channel_shutdown(), which in turn runs the io_shutdown method.
>> Thus, the restructions also apply to all the io_shutdown methods.
>> That's not documented.
>>
>> Daniel, should it be documented?
>>
> This is already done in patch 6.
Patch 6 adds "This function is thread-safe" to its contract. The
restrictions on OOB-capable handler code are much more severe than
ordinary thread safety. For instance, blocking system calls outside
critical sections are thread safe, but not permitted in OOB-capable
handler code. The contract needs to be more specific.
> Thank you for you review.
Better late than never... you're welcome!
Re: [PATCH v2 (BROKEN) 0/6] migration: bring improved savevm/loadvm/delvm to QMP
Kevin Wolf writes: > Am 28.08.2020 um 08:20 hat Markus Armbruster geschrieben: >> Kevin Wolf writes: >> >> > Am 27.08.2020 um 13:06 hat Markus Armbruster geschrieben: >> >> Daniel P. Berrangé writes: >> >> >> >> > On Wed, Aug 26, 2020 at 07:28:24PM +0100, Daniel P. Berrangé wrote: >> >> >> On Wed, Aug 26, 2020 at 05:52:06PM +0200, Markus Armbruster wrote: >> >> >> > Open questions: >> >> >> > >> >> >> > * Do we want the QMP command to delete existing snapshots with >> >> >> > conflicting tag / ID, like HMP savevm does? Or do we want it to >> >> >> > fail >> >> >> > the transaction? >> >> >> >> >> >> The intent is for the QMP commands to operate exclusively on >> >> >> 'tags', and never consider "ID". >> >> > >> >> > I forgot that even HMP ignores "ID" now and works exclusively in terms >> >> > of tags since: >> >> > >> >> > >> >> > commit 6ca080453ea403959ccde661030ca16264acc181 >> >> > Author: Daniel Henrique Barboza >> >> > Date: Wed Nov 7 11:09:58 2018 -0200 >> >> > >> >> > block/snapshot.c: eliminate use of ID input in snapshot operations >> >> >> >> Almost a year after I sent the memo I quoted. It's an incompatible >> >> change, but nobody complained, and I'm glad we got this issue out of the >> >> way. >> > >> > FWIW, I would have ignored any complaint about incompatible changes in >> > HMP. It's not supposed to be a stable API, but UI. >> >> The iffy part is actually the loss of ability to access snapshots that >> lack a name. Complaints about that would have been valid, I think. >> Fortunately, there have been none. > > 'loadvm ""' should do the trick for these. As long as you have at most one. >The same way as you have to > use with 'savevm' to create them in non-prehistoric versions of QEMU. > We stopped creating snapshots with empty names by default in 0.14, so > they are probably not very relevant any more. (Versioned machine types > go back "only" to 1.0, so good luck loading a snapshot from an older > version. And I wouldn't bet money either on a 1.0 snapshot still working > with that machine type...) No argument.
[PATCH v2 09/10] block/file-posix: fix a possible undefined behavior
local_err is not initialized to NULL, it will cause a assert error as below:
qemu/util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
Fixes: c6447510690
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Stefano Garzarella
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Aarushi Mehta
Cc: qemu-block@nongnu.org
---
- V2: no changes in v2.
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..697a7d9eea 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2113,7 +2113,7 @@ static void raw_aio_attach_aio_context(BlockDriverState
*bs,
#endif
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-Error *local_err;
+Error *local_err = NULL;
if (!aio_setup_linux_io_uring(new_context, _err)) {
error_reportf_err(local_err, "Unable to use linux io_uring, "
"falling back to thread pool: ");
--
2.18.2
[PATCH v2 08/10] blockdev: Fix a memleak in drive_backup_prepare()
'local_err' seems forgot to propagate in error path, it'll cause
a memleak. Fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Kevin Wolf
Reviewed-by: Li Qiang
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Markus Armbruster
Cc: qemu-block@nongnu.org
---
- V2: no changes in v2.
---
blockdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..842ac289c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1801,6 +1801,7 @@ static void drive_backup_prepare(BlkActionState *common,
Error **errp)
if (set_backing_hd) {
bdrv_set_backing_hd(target_bs, source, _err);
if (local_err) {
+error_propagate(errp, local_err);
goto unref;
}
}
--
2.18.2
