Re: [PATCH] qcow2: flush qcow2 l2 meta for new allocated clusters
On 8/7/2020 4:13 PM, Kevin Wolf wrote: Am 07.08.2020 um 09:42 hat Ying Fang geschrieben: On 8/6/2020 5:13 PM, Kevin Wolf wrote: Am 05.08.2020 um 04:38 hat Ying Fang geschrieben: From: fangying When qemu or qemu-nbd process uses a qcow2 image and configured with 'cache = none', it will write to the qcow2 image with a cache to cache L2 tables, however the process will not use L2 tables without explicitly calling the flush command or closing the mirror flash into the disk. Which may cause the disk data inconsistent with the written data for a long time. If an abnormal process exit occurs here, the issued written data will be lost. Therefore, in order to keep data consistency we need to flush the changes to the L2 entry to the disk in time for the newly allocated cluster. Signed-off-by: Ying Fang If you want to have data safely written to the disk after each write request, you need to use cache=writethrough/directsync (in other words, aliases that are equivalent to setting -device ...,write-cache=off). Note that this will have a major impact on write performance. cache=none means bypassing the kernel page cache (O_DIRECT), but not flushing after each write request. Well, IIUC, cache=none does not guarantee data safety and we should not expect that. Then this patch can be ignored. Indeed, cache=none is a writeback cache mode with all of the consequences. In practice, this is normally good enough because the guest OS will send flush requests when needed (e.g. because a guest application called fsync()), but if the guest doesn't do this, it may suffer data loss. This behaviour is comparable to a volatile disk cache on real hard disks and is a good default, but sometimes you need a writethrough cache mode at the cost of a performance penalty. The late reply, thanks for your detailed explanation on the 'cache' option, having more understanding for it now. Kevin .
Re: [RFC PATCH v2 7/7] util/vfio-helpers: Allow opening device requesting for multiple IRQs
On Thu, 13 Aug 2020 19:29:57 +0200
Philippe Mathieu-Daudé wrote:
> Now that our helper is ready for handling multiple IRQs, let
> qemu_vfio_open_pci() take an 'irq_count' argument.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
As with patch 2/ tying IRQ setup with the opening of a device seems
wrong. Get the device open, then create an interface to configure the
interrupt. Thanks,
Alex
> include/qemu/vfio-helpers.h | 2 +-
> block/nvme.c| 5 -
> util/vfio-helpers.c | 4 ++--
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 5c2d8ee5b3..4773b116df 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -16,7 +16,7 @@
> typedef struct QEMUVFIOState QEMUVFIOState;
>
> QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> - Error **errp);
> + unsigned irq_count, Error **errp);
> void qemu_vfio_close(QEMUVFIOState *s);
> int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>bool temporary, uint64_t *iova_list);
> diff --git a/block/nvme.c b/block/nvme.c
> index a5ef571492..2d7aac3903 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -106,6 +106,9 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) !=
> 0x1000);
> #define INDEX_ADMIN 0
> #define INDEX_IO(n) (1 + n)
>
> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> +#define MSIX_IRQ_COUNT 1
> +
> struct BDRVNVMeState {
> AioContext *aio_context;
> QEMUVFIOState *vfio;
> @@ -712,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char
> *device, int namespace,
> }
>
> s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
> - errp);
> + MSIX_IRQ_COUNT, errp);
> if (!s->vfio) {
> ret = -EINVAL;
> goto out;
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 7a934d1a1b..36fafef0d3 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -450,12 +450,12 @@ static void qemu_vfio_open_common(QEMUVFIOState *s)
> * Open a PCI device, e.g. ":00:01.0".
> */
> QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> - Error **errp)
> + unsigned irq_count, Error **errp)
> {
> int r;
> QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>
> -r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
> +r = qemu_vfio_init_pci(s, device, irq_type, irq_count, errp);
> if (r) {
> g_free(s);
> return NULL;
Re: [RFC PATCH v2 4/7] util/vfio-helpers: Check the device allow up to 'irq_count' IRQs
On Thu, 13 Aug 2020 19:29:54 +0200
Philippe Mathieu-Daudé wrote:
> As we want to use more than one single IRQ, add a check that
> the device accept our request to use multiple IRQs.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> util/vfio-helpers.c | 6 ++
> util/trace-events | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index bad60076f3..b81d4c70c2 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -335,6 +335,12 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const
> char *device,
> ret = -errno;
> goto fail;
> }
> +trace_qemu_vfio_init_pci(device_info.num_irqs);
> +if (device_info.num_irqs < irq_count) {
> +error_setg(errp, "Invalid device IRQ count");
> +ret = -EINVAL;
> +goto fail;
> +}
This is confusing the number of IRQ indexes (ie. IRQ types -
INTx/MSI/MSIx plus virtual interrupts like error reporting and device
request) with the number of sub-indexes available for a given type
again. You actually need to look at VFIO_DEVICE_GET_IRQ_INFO for the
specified irq_type to see if it supports irq_count sub-indexes.
Maybe think of interrupts as a 2-dimensional array, we have:
INDEX \ SUBINDEX
\ 0 1 2 3 4 ... N
==
INTx [0]|
MSI [1]|
MSI-X [2]|
... [M]|
VFIO_DEVICE_GET_INFO only tells us essentially the last INDEX that the
device supports. In order to learn about the number of SUBINDEXes, or
vectors, if any, that each INDEX provides, we need to look at
VFIO_DEVICE_GET_IRQ_INFO. When we're wanting to probe support for some
number of concurrent device interrupt vectors, we need to look at the
vfio_irq_info.count value for the desired index, ie. the extent of the
entries in the row associated with our column index type. Thanks,
Alex
> s->irq_type = irq_type;
> s->irq_count = irq_count;
>
> diff --git a/util/trace-events b/util/trace-events
> index 0ce42822eb..2e8be3 100644
> --- a/util/trace-events
> +++ b/util/trace-events
> @@ -83,3 +83,4 @@ qemu_vfio_new_mapping(void *s, void *host, size_t size, int
> index, uint64_t iova
> qemu_vfio_do_mapping(void *s, void *host, size_t size, uint64_t iova) "s %p
> host %p size %zu iova 0x%"PRIx64
> qemu_vfio_dma_map(void *s, void *host, size_t size, bool temporary, uint64_t
> *iova) "s %p host %p size %zu temporary %d iova %p"
> qemu_vfio_dma_unmap(void *s, void *host) "s %p host %p"
> +qemu_vfio_init_pci(uint32_t count) "device interrupt count: %"PRIu32
Re: [RFC PATCH v2 6/7] util/vfio-helpers: Allow to set EventNotifier to particular IRQ
On Thu, 13 Aug 2020 19:29:56 +0200
Philippe Mathieu-Daudé wrote:
> Let qemu_vfio_pci_init_irq() take an 'index' argument, so we can
> set the EventNotifier to a specific IRQ.
> Add a safety check. Since our helper is limited to one single IRQ
> we are safe.
>
> Our only user is the NVMe block driver, update it (also safe because
> it only uses the first IRQ).
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qemu/vfio-helpers.h | 2 +-
> block/nvme.c| 2 +-
> util/vfio-helpers.c | 11 +--
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 728f40922b..5c2d8ee5b3 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -28,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
> void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
> uint64_t offset, uint64_t size);
> int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> - Error **errp);
> + int irq_index, Error **errp);
>
> #endif
> diff --git a/block/nvme.c b/block/nvme.c
> index 21b0770c02..a5ef571492 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char
> *device, int namespace,
> }
> }
>
> -ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, errp);
> +ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, INDEX_ADMIN,
> errp);
> if (ret) {
> goto out;
> }
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 5781e4f066..7a934d1a1b 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -180,13 +180,20 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int
> index, void *bar,
> * Initialize device IRQ with @irq_type and and register an event notifier.
> */
> int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> - Error **errp)
> + int irq_index, Error **errp)
> {
> int r;
> struct vfio_irq_set *irq_set;
> size_t irq_set_size;
> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>
> +if (irq_index >= s->irq_count) {
> +error_setg(errp,
> + "Illegal interrupt %d (device initialized for %zu in
> total)",
> + irq_index, s->irq_count);
> +return -EINVAL;
> +}
> +
> irq_info.index = s->irq_type;
> if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, _info)) {
> error_setg_errno(errp, errno, "Failed to get device interrupt info");
> @@ -196,7 +203,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s,
> EventNotifier *e,
> error_setg(errp, "Device interrupt doesn't support eventfd");
> return -EINVAL;
> }
> -s->eventfd[0] = event_notifier_get_fd(e);
> +s->eventfd[irq_index] = event_notifier_get_fd(e);
This can't work. For each fd in the array provided the kernel is going
to try to get that fd and configure it as an eventfd. For each call
until we set all eventfd index {0..irq_count}, this SET_IRQS ioctl will
fail. I would probably make that pre-configure function I referred to
earlier and create a single spurious interrupt eventfd and configure
all of the vectors to signal that one eventfd. You could then have
this per vector callback swap the eventfd with the caller provided one
for the given vector.
NB, I don't know if you're going to run into trouble with this scheme
with the fact that devices can behave differently based on the number
of vectors they have enabled. You're creating an interface for a
driver, so presumably that driver knows, for example, that as soon as
vector N is enabled, signaling for event foo moves from vector 0 to
vector N. Thanks,
Alex
>
> irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
> irq_set = g_malloc0(irq_set_size);
Re: [RFC PATCH v2 2/7] util/vfio-helpers: Move IRQ 'type' from pci_init_irq() to open_pci()
On Thu, 13 Aug 2020 19:29:52 +0200
Philippe Mathieu-Daudé wrote:
> Once opened, we will used the same IRQ type for all our event
> notifiers, so pass the argument when we open the PCI device,
> store the IRQ type in the driver state, and directly use the
> value saved in the state each time we call qemu_vfio_pci_init_irq.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
This feels quite a bit strange to me, a PCI device can operate in one
of several interrupt modes, or without interrupts at all. Why would we
force a user of this interface to define the interrupt type they'll use
in advance and then not even verify if the device supports that type?
A driver might want to fall back to a different interrupt type if the
one they want is not supported. If we want to abstract this from the
driver then we should at least have an interface separate from the
initial open function that tells us to preconfigure some specified
number of vectors. We could then have a preference policy that would
attempt to use MSI-X, followed by MSI, followed by INTx (assuming
request is for a single vector), based on what the device supports.
Then a driver could fallback to fewer interrupts if the device does not
support, or the host system cannot provide, the desired number of
interrupts. Thanks,
Alex
> include/qemu/vfio-helpers.h | 5 +++--
> block/nvme.c| 6 +++---
> util/vfio-helpers.c | 13 +
> 3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 1f057c2b9e..728f40922b 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -15,7 +15,8 @@
>
> typedef struct QEMUVFIOState QEMUVFIOState;
>
> -QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp);
> +QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
> + Error **errp);
> void qemu_vfio_close(QEMUVFIOState *s);
> int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>bool temporary, uint64_t *iova_list);
> @@ -27,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
> void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
> uint64_t offset, uint64_t size);
> int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> - int irq_type, Error **errp);
> + Error **errp);
>
> #endif
> diff --git a/block/nvme.c b/block/nvme.c
> index a61e86a83e..21b0770c02 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -711,7 +711,8 @@ static int nvme_init(BlockDriverState *bs, const char
> *device, int namespace,
> return ret;
> }
>
> -s->vfio = qemu_vfio_open_pci(device, errp);
> +s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
> + errp);
> if (!s->vfio) {
> ret = -EINVAL;
> goto out;
> @@ -784,8 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char
> *device, int namespace,
> }
> }
>
> -ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier,
> - VFIO_PCI_MSIX_IRQ_INDEX, errp);
> +ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, errp);
> if (ret) {
> goto out;
> }
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 9cb9b553a5..f1196e43dc 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -43,6 +43,8 @@ typedef struct {
> struct QEMUVFIOState {
> QemuMutex lock;
>
> +int irq_type; /* vfio index */
> +
> /* These fields are protected by BQL */
> int container;
> int group;
> @@ -176,14 +178,14 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int
> index, void *bar,
> * Initialize device IRQ with @irq_type and and register an event notifier.
> */
> int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> - int irq_type, Error **errp)
> + Error **errp)
> {
> int r;
> struct vfio_irq_set *irq_set;
> size_t irq_set_size;
> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>
> -irq_info.index = irq_type;
> +irq_info.index = s->irq_type;
> if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, _info)) {
> error_setg_errno(errp, errno, "Failed to get device interrupt info");
> return -errno;
> @@ -237,6 +239,7 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
> void *buf, int size, int
> }
>
> static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
> + int irq_type,
>Error **errp)
> {
> int ret;
> @@ -331,6 +334,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const
> char *device,
> ret = -errno;
> goto fail;
> }
> +s->irq_type = irq_type;
>
>
[RFC PATCH v2 5/7] util/vfio-helpers: Support multiple eventfd
When using multiple IRQs, we'll assign an eventfd to each IRQ.
Be ready by holding an array of eventfd file descriptors in the
instance state, so when we assign new IRQs we will still use the
previous eventfds for the already assigned IRQs.
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index b81d4c70c2..5781e4f066 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -45,6 +45,7 @@ struct QEMUVFIOState {
int irq_type; /* vfio index */
size_t irq_count; /* vfio subindex (vector) */
+int32_t *eventfd;
/* These fields are protected by BQL */
int container;
@@ -195,6 +196,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
error_setg(errp, "Device interrupt doesn't support eventfd");
return -EINVAL;
}
+s->eventfd[0] = event_notifier_get_fd(e);
irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
irq_set = g_malloc0(irq_set_size);
@@ -207,8 +209,8 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
.start = 0,
.count = s->irq_count,
};
+memcpy(_set->data, >eventfd, s->irq_count * sizeof(int32_t));
-*(int32_t *)_set->data = event_notifier_get_fd(e);
r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
g_free(irq_set);
if (r) {
@@ -343,6 +345,10 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
}
s->irq_type = irq_type;
s->irq_count = irq_count;
+s->eventfd = g_new(int32_t, irq_count);
+for (i = 0; i < irq_count; i++) {
+s->eventfd[i] = -1;
+}
if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
error_setg(errp, "Invalid device regions");
@@ -379,6 +385,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
}
return 0;
fail:
+g_free(s->eventfd);
close(s->group);
fail_container:
close(s->container);
@@ -730,6 +737,7 @@ void qemu_vfio_close(QEMUVFIOState *s)
}
ram_block_notifier_remove(>ram_notifier);
qemu_vfio_reset(s);
+g_free(s->eventfd);
close(s->device);
close(s->group);
close(s->container);
--
2.21.3
[RFC PATCH v2 7/7] util/vfio-helpers: Allow opening device requesting for multiple IRQs
Now that our helper is ready for handling multiple IRQs, let
qemu_vfio_open_pci() take an 'irq_count' argument.
Signed-off-by: Philippe Mathieu-Daudé
---
include/qemu/vfio-helpers.h | 2 +-
block/nvme.c| 5 -
util/vfio-helpers.c | 4 ++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 5c2d8ee5b3..4773b116df 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -16,7 +16,7 @@
typedef struct QEMUVFIOState QEMUVFIOState;
QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
- Error **errp);
+ unsigned irq_count, Error **errp);
void qemu_vfio_close(QEMUVFIOState *s);
int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
bool temporary, uint64_t *iova_list);
diff --git a/block/nvme.c b/block/nvme.c
index a5ef571492..2d7aac3903 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -106,6 +106,9 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
#define INDEX_ADMIN 0
#define INDEX_IO(n) (1 + n)
+/* This driver shares a single MSIX IRQ for the admin and I/O queues */
+#define MSIX_IRQ_COUNT 1
+
struct BDRVNVMeState {
AioContext *aio_context;
QEMUVFIOState *vfio;
@@ -712,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
}
s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
- errp);
+ MSIX_IRQ_COUNT, errp);
if (!s->vfio) {
ret = -EINVAL;
goto out;
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 7a934d1a1b..36fafef0d3 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -450,12 +450,12 @@ static void qemu_vfio_open_common(QEMUVFIOState *s)
* Open a PCI device, e.g. ":00:01.0".
*/
QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
- Error **errp)
+ unsigned irq_count, Error **errp)
{
int r;
QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
-r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
+r = qemu_vfio_init_pci(s, device, irq_type, irq_count, errp);
if (r) {
g_free(s);
return NULL;
--
2.21.3
[RFC PATCH v2 0/7] util/vfio-helpers: Add support for multiple IRQs
This series intends to setup the VFIO helper to allow binding notifiers on different IRQs. Only MSIX IRQ type considered so far. Stefan suggested me to publish earlier to discuss. If not too bad feedbacks I'll add some documentation in "qemu/vfio-helpers.h" before reposting. (NVMe block driver series will follow). Based-on: <20200812185014.18267-1-phi...@redhat.com> "block/nvme: Various cleanups required to use multiple queues" https://www.mail-archive.com/qemu-devel@nongnu.org/msg729395.html Supersedes: <20200811172845.16698-1-phi...@redhat.com> Philippe Mathieu-Daudé (7): util/vfio-helpers: Store eventfd using int32_t type util/vfio-helpers: Move IRQ 'type' from pci_init_irq() to open_pci() util/vfio-helpers: Introduce 'irq_count' variable util/vfio-helpers: Check the device allow up to 'irq_count' IRQs util/vfio-helpers: Support multiple eventfd util/vfio-helpers: Allow to set EventNotifier to particular IRQ util/vfio-helpers: Allow opening device requesting for multiple IRQs include/qemu/vfio-helpers.h | 5 +++-- block/nvme.c| 9 +--- util/vfio-helpers.c | 42 ++--- util/trace-events | 1 + 4 files changed, 45 insertions(+), 12 deletions(-) -- 2.21.3
[RFC PATCH v2 2/7] util/vfio-helpers: Move IRQ 'type' from pci_init_irq() to open_pci()
Once opened, we will used the same IRQ type for all our event
notifiers, so pass the argument when we open the PCI device,
store the IRQ type in the driver state, and directly use the
value saved in the state each time we call qemu_vfio_pci_init_irq.
Signed-off-by: Philippe Mathieu-Daudé
---
include/qemu/vfio-helpers.h | 5 +++--
block/nvme.c| 6 +++---
util/vfio-helpers.c | 13 +
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 1f057c2b9e..728f40922b 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -15,7 +15,8 @@
typedef struct QEMUVFIOState QEMUVFIOState;
-QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp);
+QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
+ Error **errp);
void qemu_vfio_close(QEMUVFIOState *s);
int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
bool temporary, uint64_t *iova_list);
@@ -27,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
uint64_t offset, uint64_t size);
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
- int irq_type, Error **errp);
+ Error **errp);
#endif
diff --git a/block/nvme.c b/block/nvme.c
index a61e86a83e..21b0770c02 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -711,7 +711,8 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
return ret;
}
-s->vfio = qemu_vfio_open_pci(device, errp);
+s->vfio = qemu_vfio_open_pci(device, VFIO_PCI_MSIX_IRQ_INDEX,
+ errp);
if (!s->vfio) {
ret = -EINVAL;
goto out;
@@ -784,8 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
}
}
-ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier,
- VFIO_PCI_MSIX_IRQ_INDEX, errp);
+ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, errp);
if (ret) {
goto out;
}
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 9cb9b553a5..f1196e43dc 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -43,6 +43,8 @@ typedef struct {
struct QEMUVFIOState {
QemuMutex lock;
+int irq_type; /* vfio index */
+
/* These fields are protected by BQL */
int container;
int group;
@@ -176,14 +178,14 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index,
void *bar,
* Initialize device IRQ with @irq_type and and register an event notifier.
*/
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
- int irq_type, Error **errp)
+ Error **errp)
{
int r;
struct vfio_irq_set *irq_set;
size_t irq_set_size;
struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
-irq_info.index = irq_type;
+irq_info.index = s->irq_type;
if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, _info)) {
error_setg_errno(errp, errno, "Failed to get device interrupt info");
return -errno;
@@ -237,6 +239,7 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
}
static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
+ int irq_type,
Error **errp)
{
int ret;
@@ -331,6 +334,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
ret = -errno;
goto fail;
}
+s->irq_type = irq_type;
if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
error_setg(errp, "Invalid device regions");
@@ -423,12 +427,13 @@ static void qemu_vfio_open_common(QEMUVFIOState *s)
/**
* Open a PCI device, e.g. ":00:01.0".
*/
-QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
+QEMUVFIOState *qemu_vfio_open_pci(const char *device, int irq_type,
+ Error **errp)
{
int r;
QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
-r = qemu_vfio_init_pci(s, device, errp);
+r = qemu_vfio_init_pci(s, device, irq_type, errp);
if (r) {
g_free(s);
return NULL;
--
2.21.3
[RFC PATCH v2 3/7] util/vfio-helpers: Introduce 'irq_count' variable
Currently this helper is restricted to a single VFIO (MSIX) IRQ.
As we will slowly make it support multiple IRQs, introduce the
'irq_count' variable which contains the total number of IRQs we
initialized the device with.
Set the variable in qemu_vfio_init_pci().
Hardcode the current single IRQ used.
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index f1196e43dc..bad60076f3 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -44,6 +44,7 @@ struct QEMUVFIOState {
QemuMutex lock;
int irq_type; /* vfio index */
+size_t irq_count; /* vfio subindex (vector) */
/* These fields are protected by BQL */
int container;
@@ -195,7 +196,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
return -EINVAL;
}
-irq_set_size = sizeof(*irq_set) + sizeof(int32_t);
+irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
irq_set = g_malloc0(irq_set_size);
/* Get to a known IRQ state */
@@ -204,7 +205,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
.flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER,
.index = irq_info.index,
.start = 0,
-.count = 1,
+.count = s->irq_count,
};
*(int32_t *)_set->data = event_notifier_get_fd(e);
@@ -239,7 +240,7 @@ static int qemu_vfio_pci_write_config(QEMUVFIOState *s,
void *buf, int size, int
}
static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
- int irq_type,
+ int irq_type, unsigned irq_count,
Error **errp)
{
int ret;
@@ -335,6 +336,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
goto fail;
}
s->irq_type = irq_type;
+s->irq_count = irq_count;
if (device_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX) {
error_setg(errp, "Invalid device regions");
@@ -433,7 +435,7 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, int
irq_type,
int r;
QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
-r = qemu_vfio_init_pci(s, device, irq_type, errp);
+r = qemu_vfio_init_pci(s, device, irq_type, 1, errp);
if (r) {
g_free(s);
return NULL;
--
2.21.3
[RFC PATCH v2 6/7] util/vfio-helpers: Allow to set EventNotifier to particular IRQ
Let qemu_vfio_pci_init_irq() take an 'index' argument, so we can
set the EventNotifier to a specific IRQ.
Add a safety check. Since our helper is limited to one single IRQ
we are safe.
Our only user is the NVMe block driver, update it (also safe because
it only uses the first IRQ).
Signed-off-by: Philippe Mathieu-Daudé
---
include/qemu/vfio-helpers.h | 2 +-
block/nvme.c| 2 +-
util/vfio-helpers.c | 11 +--
3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 728f40922b..5c2d8ee5b3 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -28,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
uint64_t offset, uint64_t size);
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
- Error **errp);
+ int irq_index, Error **errp);
#endif
diff --git a/block/nvme.c b/block/nvme.c
index 21b0770c02..a5ef571492 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char
*device, int namespace,
}
}
-ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, errp);
+ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier, INDEX_ADMIN, errp);
if (ret) {
goto out;
}
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 5781e4f066..7a934d1a1b 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -180,13 +180,20 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index,
void *bar,
* Initialize device IRQ with @irq_type and and register an event notifier.
*/
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
- Error **errp)
+ int irq_index, Error **errp)
{
int r;
struct vfio_irq_set *irq_set;
size_t irq_set_size;
struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
+if (irq_index >= s->irq_count) {
+error_setg(errp,
+ "Illegal interrupt %d (device initialized for %zu in
total)",
+ irq_index, s->irq_count);
+return -EINVAL;
+}
+
irq_info.index = s->irq_type;
if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, _info)) {
error_setg_errno(errp, errno, "Failed to get device interrupt info");
@@ -196,7 +203,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
error_setg(errp, "Device interrupt doesn't support eventfd");
return -EINVAL;
}
-s->eventfd[0] = event_notifier_get_fd(e);
+s->eventfd[irq_index] = event_notifier_get_fd(e);
irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
irq_set = g_malloc0(irq_set_size);
--
2.21.3
[RFC PATCH v2 4/7] util/vfio-helpers: Check the device allow up to 'irq_count' IRQs
As we want to use more than one single IRQ, add a check that
the device accept our request to use multiple IRQs.
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 6 ++
util/trace-events | 1 +
2 files changed, 7 insertions(+)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index bad60076f3..b81d4c70c2 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -335,6 +335,12 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char
*device,
ret = -errno;
goto fail;
}
+trace_qemu_vfio_init_pci(device_info.num_irqs);
+if (device_info.num_irqs < irq_count) {
+error_setg(errp, "Invalid device IRQ count");
+ret = -EINVAL;
+goto fail;
+}
s->irq_type = irq_type;
s->irq_count = irq_count;
diff --git a/util/trace-events b/util/trace-events
index 0ce42822eb..2e8be3 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -83,3 +83,4 @@ qemu_vfio_new_mapping(void *s, void *host, size_t size, int
index, uint64_t iova
qemu_vfio_do_mapping(void *s, void *host, size_t size, uint64_t iova) "s %p
host %p size %zu iova 0x%"PRIx64
qemu_vfio_dma_map(void *s, void *host, size_t size, bool temporary, uint64_t
*iova) "s %p host %p size %zu temporary %d iova %p"
qemu_vfio_dma_unmap(void *s, void *host) "s %p host %p"
+qemu_vfio_init_pci(uint32_t count) "device interrupt count: %"PRIu32
--
2.21.3
[RFC PATCH v2 1/7] util/vfio-helpers: Store eventfd using int32_t type
Per the documentation in linux-headers/linux/vfio.h:
VFIO_DEVICE_SET_IRQS
* DATA_EVENTFD binds the specified ACTION to the provided __s32 eventfd.
Replace the 'int' by an 'int32_t' to match the documentation.
Fixes: 418026ca43 ("util: Introduce vfio helpers")
Signed-off-by: Philippe Mathieu-Daudé
---
util/vfio-helpers.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index e399e330e2..9cb9b553a5 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -193,7 +193,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
return -EINVAL;
}
-irq_set_size = sizeof(*irq_set) + sizeof(int);
+irq_set_size = sizeof(*irq_set) + sizeof(int32_t);
irq_set = g_malloc0(irq_set_size);
/* Get to a known IRQ state */
@@ -205,7 +205,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier
*e,
.count = 1,
};
-*(int *)_set->data = event_notifier_get_fd(e);
+*(int32_t *)_set->data = event_notifier_get_fd(e);
r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set);
g_free(irq_set);
if (r) {
--
2.21.3
Re: What is bs->reqs_lock for?
13.08.2020 18:54, Paolo Bonzini wrote: On 13/08/20 16:57, Vladimir Sementsov-Ogievskiy wrote: Hi! Sorry my stupid question, but which kind of concurrent access bs->reqs_lock prevents? In my understanding the whole logic of request tracking for the bs is going in the coroutine, so, we don't have parallel access anyway? How can parallel access to bs->tracked_requests happen? Different iothreads can access the same BlockDriverState, and block/io.c is not protected by the AioContext lock (in fact almost nothing, or nothing, needs it in the I/O path). I thought bs is attached to one aio context and aio context attached to one iothread. And all normal request processing of the bs is done in this one iothread. And when we need to access bs externally, we do it in aio_context_acquire / aio_context_release, which protects from parallel access to BlockDriverState fields... But you say, that block/io.c is not protected by AioContext lock.. Does it mean that everything must be thread-safe in block/io.c and all block drivers? Are tracked_requests different from other fields? A lot of other BlockDriverState fields are not protected by any mutex.. For example: total_sectors, file, backing.. Could you give an example of parallel access to tracked_requests? -- Best regards, Vladimir
[RFC PATCH 21/22] block/export: Move blk to BlockExport
Every block export has a BlockBackend representing the disk that is
exported. It should live in BlockExport therefore.
Signed-off-by: Kevin Wolf
---
include/block/export.h | 3 +++
block/export/export.c | 3 +++
nbd/server.c | 44 ++
3 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 83f554b745..53b4163a3b 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -54,6 +54,9 @@ struct BlockExport {
*/
AioContext *ctx;
+/* The block device to export */
+BlockBackend *blk;
+
/* List entry for block_exports */
QLIST_ENTRY(BlockExport) next;
};
diff --git a/block/export/export.c b/block/export/export.c
index 1255f3fc80..3cd448ba72 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -89,6 +89,8 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
return NULL;
}
+assert(exp->blk != NULL);
+
QLIST_INSERT_HEAD(_exports, exp, next);
return exp;
}
@@ -105,6 +107,7 @@ void blk_exp_unref(BlockExport *exp)
if (--exp->refcount == 0) {
QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
+blk_unref(exp->blk);
g_free(exp->id);
g_free(exp);
aio_wait_kick();
diff --git a/nbd/server.c b/nbd/server.c
index 899d00782f..6ad78203c9 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -84,7 +84,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
-BlockBackend *blk;
char *name;
char *description;
uint64_t size;
@@ -643,7 +642,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
* whether this is OPT_INFO or OPT_GO. */
/* minimum - 1 for back-compat, or actual if client will obey it. */
if (client->opt == NBD_OPT_INFO || blocksize) {
-check_align = sizes[0] = blk_get_request_alignment(exp->blk);
+check_align = sizes[0] = blk_get_request_alignment(exp->common.blk);
} else {
sizes[0] = 1;
}
@@ -652,7 +651,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
* TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
sizes[1] = MAX(4096, sizes[0]);
/* maximum - At most 32M, but smaller as appropriate. */
-sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE);
+sizes[2] = MIN(blk_get_max_transfer(exp->common.blk), NBD_MAX_BUFFER_SIZE);
trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]);
sizes[0] = cpu_to_be32(sizes[0]);
sizes[1] = cpu_to_be32(sizes[1]);
@@ -684,7 +683,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
* tolerate all clients, regardless of alignments.
*/
if (client->opt == NBD_OPT_INFO && !blocksize &&
-blk_get_request_alignment(exp->blk) > 1) {
+blk_get_request_alignment(exp->common.blk) > 1) {
return nbd_negotiate_send_rep_err(client,
NBD_REP_ERR_BLOCK_SIZE_REQD,
errp,
@@ -1556,7 +1555,7 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState
*bs,
blk_set_allow_aio_context_change(blk, true);
QTAILQ_INIT(>clients);
-exp->blk = blk;
+exp->common.blk = blk;
exp->name = g_strdup(name);
assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
exp->description = g_strdup(desc);
@@ -1679,15 +1678,13 @@ static void nbd_export_delete(BlockExport *blk_exp)
g_free(exp->description);
exp->description = NULL;
-if (exp->blk) {
+if (exp->common.blk) {
if (exp->eject_notifier_blk) {
notifier_remove(>eject_notifier);
blk_unref(exp->eject_notifier_blk);
}
-blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
+blk_remove_aio_context_notifier(exp->common.blk, blk_aio_attached,
blk_aio_detach, exp);
-blk_unref(exp->blk);
-exp->blk = NULL;
}
if (exp->export_bitmap) {
@@ -1840,7 +1837,7 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient
*client,
while (progress < size) {
int64_t pnum;
-int status = bdrv_block_status_above(blk_bs(exp->blk), NULL,
+int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
offset + progress,
size - progress, , NULL,
NULL);
@@ -1872,7 +1869,8 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient
*client,
stl_be_p(, pnum);
ret = nbd_co_send_iov(client, iov, 1, errp);
} else {
-ret = blk_pread(exp->blk, offset + progress, data + progress,
pnum);
+ret = blk_pread(exp->common.blk, offset + progress,
+data +
[RFC PATCH 22/22] block/export: Add query-block-exports
This adds a simple QMP command to query the list of block exports.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 33 +
block/export/export.c | 23 +++
2 files changed, 56 insertions(+)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index a067de2ba3..0b184bbd7c 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -226,3 +226,36 @@
##
{ 'command': 'block-export-del',
'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
+
+##
+# @BlockExportInfo:
+#
+# Information about a single block export.
+#
+# @id: The unique identifier for the block export
+#
+# @type: This field is returned only for compatibility reasons, it should
+#not be used (always returns 'unknown')
+#
+# @node-name: The node name of the block node that is exported
+#
+# @shutting-down: True if the export is shutting down (e.g. after a
+# block-export-del command, but before the shutdown has
+# completed)
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockExportInfo',
+ 'data': { 'id': 'str',
+'type': 'BlockExportType',
+'node-name': 'str',
+'shutting-down': 'bool' } }
+
+##
+# @query-block-exports:
+#
+# Returns: A list of BlockExportInfo describing all block exports
+#
+# Since: 5.2
+##
+{ 'command': 'query-block-exports', 'returns': ['BlockExportInfo'] }
diff --git a/block/export/export.c b/block/export/export.c
index 3cd448ba72..71d17bd440 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -272,3 +272,26 @@ void qmp_nbd_server_remove(const char *name,
qmp_block_export_del(name, has_mode, mode, errp);
}
+
+BlockExportInfoList *qmp_query_block_exports(Error **errp)
+{
+BlockExportInfoList *head = NULL, **p_next =
+BlockExport *exp;
+
+QLIST_FOREACH(exp, _exports, next) {
+BlockExportInfoList *entry = g_new0(BlockExportInfoList, 1);
+BlockExportInfo *info = g_new(BlockExportInfo, 1);
+*info = (BlockExportInfo) {
+.id = g_strdup(exp->id),
+.type = exp->drv->type,
+.node_name = g_strdup(bdrv_get_node_name(blk_bs(exp->blk))),
+.shutting_down = !exp->user_owned,
+};
+
+entry->value = info;
+*p_next = entry;
+p_next = >next;
+}
+
+return head;
+}
--
2.25.4
[RFC PATCH 20/22] block/export: Add block-export-del
Implement a new QMP command block-export-del and make nbd-server-remove
a wrapper around it.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 30 +++
include/block/nbd.h| 1 -
block/export/export.c | 54 ++
block/monitor/block-hmp-cmds.c | 2 +-
blockdev-nbd.c | 28 --
nbd/server.c | 14 -
6 files changed, 79 insertions(+), 50 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 0d0db9ca1b..a067de2ba3 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -111,9 +111,9 @@
'data': 'NbdServerAddOptions', 'boxed': true }
##
-# @NbdServerRemoveMode:
+# @BlockExportRemoveMode:
#
-# Mode for removing an NBD export.
+# Mode for removing a block export.
#
# @safe: Remove export if there are no existing connections, fail otherwise.
#
@@ -129,16 +129,16 @@
#
# Since: 2.12
##
-{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
+{'enum': 'BlockExportRemoveMode', 'data': ['safe', 'hard']}
##
# @nbd-server-remove:
#
# Remove NBD export by name.
#
-# @name: Export name.
+# @name: Block export id.
#
-# @mode: Mode of command operation. See @NbdServerRemoveMode description.
+# @mode: Mode of command operation. See @BlockExportRemoveMode description.
#Default is 'safe'.
#
# Returns: error if
@@ -149,7 +149,7 @@
# Since: 2.12
##
{ 'command': 'nbd-server-remove',
- 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
+ 'data': {'name': 'str', '*mode': 'BlockExportRemoveMode'} }
##
# @nbd-server-stop:
@@ -208,3 +208,21 @@
##
{ 'command': 'block-export-add',
'data': 'BlockExportOptions', 'boxed': true }
+
+##
+# @block-export-del:
+#
+# Remove a block export.
+#
+# @id: Block export id.
+#
+# @mode: Mode of command operation. See @BlockExportRemoveMode description.
+#Default is 'safe'.
+#
+# Returns: Error if the export is not found or @mode is 'safe' and the export
+# is still in use (e.g. by existing client connections)
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-del',
+ 'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 91a9d4f96d..7982a63f96 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -335,7 +335,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState
*bs,
const char *bitmap, bool readonly, bool shared,
bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
-void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
diff --git a/block/export/export.c b/block/export/export.c
index f94a81258a..1255f3fc80 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -115,6 +115,15 @@ void blk_exp_request_shutdown(BlockExport *exp)
{
AioContext *aio_context = exp->ctx;
+/*
+ * If the user doesn't own the export any more, it is already shutting
+ * down. We must not call .request_shutdown and decrease the refcount a
+ * second time.
+ */
+if (!exp->user_owned) {
+return;
+}
+
aio_context_acquire(aio_context);
exp->drv->request_shutdown(exp);
aio_context_release(aio_context);
@@ -215,3 +224,48 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error
**errp)
fail:
qapi_free_BlockExportOptions(export_opts);
}
+
+void qmp_block_export_del(const char *id,
+ bool has_mode, BlockExportRemoveMode mode,
+ Error **errp)
+{
+ERRP_GUARD();
+BlockExport *exp;
+
+exp = blk_exp_find(id);
+if (exp == NULL) {
+error_setg(errp, "Export '%s' is not found", id);
+return;
+}
+if (!exp->user_owned) {
+error_setg(errp, "Export '%s' is already shutting down", id);
+return;
+}
+
+if (!has_mode) {
+mode = BLOCK_EXPORT_REMOVE_MODE_SAFE;
+}
+if (mode == BLOCK_EXPORT_REMOVE_MODE_SAFE && exp->refcount > 1) {
+error_setg(errp, "export '%s' still in use", exp->id);
+error_append_hint(errp, "Use mode='hard' to force client "
+ "disconnect\n");
+return;
+}
+
+blk_exp_request_shutdown(exp);
+}
+
+void qmp_nbd_server_remove(const char *name,
+ bool has_mode, BlockExportRemoveMode mode,
+ Error **errp)
+{
+BlockExport *exp;
+
+exp = blk_exp_find(name);
+if (exp && exp->drv->type != BLOCK_EXPORT_TYPE_NBD) {
+error_setg(errp, "Block export '%s' is not an NBD export", name);
+return;
+}
+
+qmp_block_export_del(name, has_mode, mode, errp);
+}
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 6c823234a9..10165252cf 100644
---
[RFC PATCH 19/22] block/export: Move strong user reference to block_exports
The reference owned by the user/monitor that is created when adding the
export and dropped when removing it was tied to the 'exports' list in
nbd/server.c. Every block export will have a user reference, so move it
to the block export level and tie it to the 'block_exports' list in
block/export/export.c instead. This is necessary for introducing a QMP
command for removing exports.
Note that exports are present in block_exports even after the user has
requested shutdown. This is different from NBD's exports where exports
are immediately removed on a shutdown request, even if they are still in
the process of shutting down. In order to avoid that the user still
interacts with an export that is shutting down (and possibly removes it
a second time), we need to remember if the user actually still owns it.
Signed-off-by: Kevin Wolf
---
include/block/export.h | 8
block/export/export.c | 4
blockdev-nbd.c | 5 -
nbd/server.c | 2 --
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 43229857b0..83f554b745 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -40,6 +40,14 @@ struct BlockExport {
*/
int refcount;
+/*
+ * True if one of the references in refcount belongs to the user. After the
+ * user has dropped their reference, they may not e.g. remove the same
+ * export a second time (which would decrease the refcount without having
+ * it incremented first).
+ */
+bool user_owned;
+
/*
* The AioContex whose lock needs to be held while calling
* BlockExportDriver callbacks.
diff --git a/block/export/export.c b/block/export/export.c
index 72f1fab975..f94a81258a 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -78,6 +78,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
*exp = (BlockExport) {
.drv= _exp_nbd,
.refcount = 1,
+.user_owned = true,
.id = g_strdup(export->id),
};
@@ -117,6 +118,9 @@ void blk_exp_request_shutdown(BlockExport *exp)
aio_context_acquire(aio_context);
exp->drv->request_shutdown(exp);
aio_context_release(aio_context);
+
+exp->user_owned = false;
+blk_exp_unref(exp);
}
static bool blk_exp_has_type(BlockExportType type)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1c7aa874ee..40013b7d64 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -231,11 +231,6 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions
*exp_args,
goto out;
}
-/* The list of named exports has a strong reference to this export now and
- * our only way of accessing it is through nbd_export_find(), so we can
drop
- * the strong reference that is @exp. */
-blk_exp_unref((BlockExport*) exp);
-
ret = 0;
out:
aio_context_release(aio_context);
diff --git a/nbd/server.c b/nbd/server.c
index 7e2976b81d..e3ac7f548b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,7 +1616,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState
*bs,
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
-blk_exp_ref(>common);
QTAILQ_INSERT_TAIL(, exp, next);
return 0;
@@ -1663,7 +1662,6 @@ static void nbd_export_request_shutdown(BlockExport
*blk_exp)
client_close(client, true);
}
if (exp->name) {
-blk_exp_unref(>common);
g_free(exp->name);
exp->name = NULL;
QTAILQ_REMOVE(, exp, next);
--
2.25.4
[RFC PATCH 06/22] qemu-nbd: Use raw block driver for --offset
Instead of implementing qemu-nbd --offset in the NBD code, just put a
raw block node with the requested offset on top of the user image and
rely on that doing the job.
This does not only simplify the nbd_export_new() interface and bring it
closer to the set of options that the nbd-server-add QMP command offers,
but in fact it also eliminates a potential source for bugs in the NBD
code which previously had to add the offset manually in all relevant
places.
Signed-off-by: Kevin Wolf
---
include/block/nbd.h | 4 ++--
blockdev-nbd.c | 9 +
nbd/server.c| 34 +-
qemu-nbd.c | 27 ---
4 files changed, 32 insertions(+), 42 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index c8c5cb6b61..3846d2bac8 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -329,8 +329,8 @@ typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
-NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
- uint64_t size, const char *name, const char *desc,
+NBDExport *nbd_export_new(BlockDriverState *bs,
+ const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
BlockBackend *on_eject_blk, Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index a1dc11bdd7..16cda3b052 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -154,7 +154,6 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
BlockDriverState *bs = NULL;
BlockBackend *on_eject_blk;
NBDExport *exp = NULL;
-int64_t len;
AioContext *aio_context;
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
@@ -192,12 +191,6 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
-len = bdrv_getlength(bs);
-if (len < 0) {
-error_setg_errno(errp, -len,
- "Failed to determine the NBD export's length");
-goto out;
-}
if (!arg->has_writable) {
arg->writable = false;
@@ -206,7 +199,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
arg->writable = false;
}
-exp = nbd_export_new(bs, 0, len, arg->name, arg->description, arg->bitmap,
+exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
NULL, false, on_eject_blk, errp);
if (!exp) {
diff --git a/nbd/server.c b/nbd/server.c
index 774325dbe5..92360d1f08 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -89,7 +89,6 @@ struct NBDExport {
BlockBackend *blk;
char *name;
char *description;
-uint64_t dev_offset;
uint64_t size;
uint16_t nbdflags;
QTAILQ_HEAD(, NBDClient) clients;
@@ -1507,8 +1506,8 @@ static void nbd_eject_notifier(Notifier *n, void *data)
aio_context_release(aio_context);
}
-NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
- uint64_t size, const char *name, const char *desc,
+NBDExport *nbd_export_new(BlockDriverState *bs,
+ const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
BlockBackend *on_eject_blk, Error **errp)
@@ -1516,9 +1515,17 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t
dev_offset,
AioContext *ctx;
BlockBackend *blk;
NBDExport *exp;
+int64_t size;
uint64_t perm;
int ret;
+size = bdrv_getlength(bs);
+if (size < 0) {
+error_setg_errno(errp, -size,
+ "Failed to determine the NBD export's length");
+return NULL;
+}
+
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
.drv = _exp_nbd,
@@ -1553,8 +1560,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t
dev_offset,
exp->refcount = 1;
QTAILQ_INIT(>clients);
exp->blk = blk;
-assert(dev_offset <= INT64_MAX);
-exp->dev_offset = dev_offset;
exp->name = g_strdup(name);
assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
exp->description = g_strdup(desc);
@@ -1569,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t
dev_offset,
exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
NBD_FLAG_SEND_FAST_ZERO);
}
-assert(size <= INT64_MAX - dev_offset);
+assert(size <= INT64_MAX);
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
if (bitmap) {
@@ -1928,8
[RFC PATCH 17/22] block/export: Add blk_exp_close_all(_type)
This adds a function to shut down all block exports, and another one to
shut down the block exports of a single type. The latter is used for now
when stopping the NBD server. As soon as we implement support for
multiple NBD servers, we'll need a per-server list of exports and it
will be replaced by a function using that.
As a side effect, the BlockExport layer has a list tracking all existing
exports now. closed_exports loses its only user and can go away.
Signed-off-by: Kevin Wolf
---
include/block/export.h | 8 +++
include/block/nbd.h| 2 --
block.c| 2 +-
block/export/export.c | 52 ++
blockdev-nbd.c | 2 +-
nbd/server.c | 34 ---
qemu-nbd.c | 2 +-
7 files changed, 68 insertions(+), 34 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 597fc58245..1698b68f09 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -15,6 +15,7 @@
#define BLOCK_EXPORT_H
#include "qapi/qapi-types-block-export.h"
+#include "qemu/queue.h"
typedef struct BlockExport BlockExport;
@@ -23,6 +24,7 @@ typedef struct BlockExportDriver {
size_t instance_size;
int (*create)(BlockExport *, BlockExportOptions *, Error **);
void (*delete)(BlockExport *);
+void (*request_shutdown)(BlockExport *);
} BlockExportDriver;
struct BlockExport {
@@ -40,6 +42,9 @@ struct BlockExport {
* BlockExportDriver callbacks.
*/
AioContext *ctx;
+
+/* List entry for block_exports */
+QLIST_ENTRY(BlockExport) next;
};
extern const BlockExportDriver blk_exp_nbd;
@@ -47,5 +52,8 @@ extern const BlockExportDriver blk_exp_nbd;
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
void blk_exp_ref(BlockExport *exp);
void blk_exp_unref(BlockExport *exp);
+void blk_exp_request_shutdown(BlockExport *exp);
+void blk_exp_close_all(void);
+void blk_exp_close_all_type(BlockExportType type);
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 602536feb2..91a9d4f96d 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -335,12 +335,10 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState
*bs,
const char *bitmap, bool readonly, bool shared,
bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
-void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
-void nbd_export_close_all(void);
void nbd_client_new(QIOChannelSocket *sioc,
QCryptoTLSCreds *tlscreds,
diff --git a/block.c b/block.c
index d9ac0e07eb..357c72846e 100644
--- a/block.c
+++ b/block.c
@@ -4424,7 +4424,7 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
assert(job_next(NULL) == NULL);
-nbd_export_close_all();
+blk_exp_close_all();
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
diff --git a/block/export/export.c b/block/export/export.c
index 9de108cbc1..675db9a8b9 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -24,6 +24,9 @@ static const BlockExportDriver* blk_exp_drivers[] = {
_exp_nbd,
};
+static QLIST_HEAD(, BlockExport) block_exports =
+QLIST_HEAD_INITIALIZER(block_exports);
+
static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
{
int i;
@@ -60,6 +63,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
return NULL;
}
+QLIST_INSERT_HEAD(_exports, exp, next);
return exp;
}
@@ -73,11 +77,59 @@ void blk_exp_unref(BlockExport *exp)
{
assert(exp->refcount > 0);
if (--exp->refcount == 0) {
+QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
g_free(exp);
+aio_wait_kick();
}
}
+void blk_exp_request_shutdown(BlockExport *exp)
+{
+AioContext *aio_context = exp->ctx;
+
+aio_context_acquire(aio_context);
+exp->drv->request_shutdown(exp);
+aio_context_release(aio_context);
+}
+
+static bool blk_exp_has_type(BlockExportType type)
+{
+BlockExport *exp;
+
+if (type == BLOCK_EXPORT_TYPE__MAX) {
+return !QLIST_EMPTY(_exports);
+}
+
+QLIST_FOREACH(exp, _exports, next) {
+if (exp->drv->type == type) {
+return true;
+}
+}
+
+return false;
+}
+
+/* type == BLOCK_EXPORT_TYPE__MAX for all types */
+void blk_exp_close_all_type(BlockExportType type)
+{
+BlockExport *exp, *next;
+
+QLIST_FOREACH_SAFE(exp, _exports, next, next) {
+if (type != BLOCK_EXPORT_TYPE__MAX && exp->drv->type != type) {
+continue;
+}
+blk_exp_request_shutdown(exp);
+}
+
+AIO_WAIT_WHILE(NULL,
[RFC PATCH 16/22] block/export: Allocate BlockExport in blk_exp_add()
Instead of letting the driver allocate and return the BlockExport
object, allocate it already in blk_exp_add() and pass it. This allows us
to initialise the generic part before calling into the driver so that
the driver can just use these values instead of having to parse the
options a second time.
For symmetry, move freeing the BlockExport to blk_exp_unref().
Signed-off-by: Kevin Wolf
---
include/block/export.h | 3 ++-
include/block/nbd.h| 11 ++-
block/export/export.c | 17 -
blockdev-nbd.c | 24 +---
nbd/server.c | 30 +-
5 files changed, 50 insertions(+), 35 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 5459f79469..597fc58245 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -20,7 +20,8 @@ typedef struct BlockExport BlockExport;
typedef struct BlockExportDriver {
BlockExportType type;
-BlockExport *(*create)(BlockExportOptions *, Error **);
+size_t instance_size;
+int (*create)(BlockExport *, BlockExportOptions *, Error **);
void (*delete)(BlockExport *);
} BlockExportDriver;
diff --git a/include/block/nbd.h b/include/block/nbd.h
index af8509ab70..602536feb2 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -328,11 +328,12 @@ int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
-BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
-NBDExport *nbd_export_new(BlockDriverState *bs,
- const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- bool writethrough, Error **errp);
+int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
+ Error **errp);
+int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
+ const char *name, const char *desc,
+ const char *bitmap, bool readonly, bool shared,
+ bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
diff --git a/block/export/export.c b/block/export/export.c
index ef86bf892b..9de108cbc1 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -39,6 +39,8 @@ static const BlockExportDriver
*blk_exp_find_driver(BlockExportType type)
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
+BlockExport *exp;
+int ret;
drv = blk_exp_find_driver(export->type);
if (!drv) {
@@ -46,7 +48,19 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
return NULL;
}
-return drv->create(export, errp);
+exp = g_malloc0(drv->instance_size);
+*exp = (BlockExport) {
+.drv= _exp_nbd,
+.refcount = 1,
+};
+
+ret = drv->create(exp, export, errp);
+if (ret < 0) {
+g_free(exp);
+return NULL;
+}
+
+return exp;
}
void blk_exp_ref(BlockExport *exp)
@@ -60,6 +74,7 @@ void blk_exp_unref(BlockExport *exp)
assert(exp->refcount > 0);
if (--exp->refcount == 0) {
exp->drv->delete(exp);
+g_free(exp);
}
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 5e97975c80..f97deba424 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -173,18 +173,19 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
+int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
+ Error **errp)
{
BlockExportOptionsNbd *arg = _args->u.nbd;
BlockDriverState *bs = NULL;
-NBDExport *exp = NULL;
AioContext *aio_context;
+int ret;
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
if (!nbd_server && !is_qemu_nbd) {
error_setg(errp, "NBD server not running");
-return NULL;
+return -EINVAL;
}
if (!arg->has_name) {
@@ -193,22 +194,22 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "export name '%s' too long", arg->name);
-return NULL;
+return -EINVAL;
}
if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "description '%s' too long", arg->description);
-return NULL;
+return -EINVAL;
}
if (nbd_export_find(arg->name)) {
error_setg(errp, "NBD server already has export named '%s'",
arg->name);
-return NULL;
+return -EEXIST;
}
bs = bdrv_lookup_bs(exp_args->device, exp_args->device, errp);
if (!bs)
[RFC PATCH 18/22] block/export: Add 'id' option to block-export-add
We'll need an id to identify block exports in monitor commands. This
adds one.
Note that this is different from the 'name' option in the NBD server,
which is the externally visible export name. While block export ids need
to be unique in the whole process, export names must be unique only for
the same server. Different export types or (potentially in the future)
multiple NBD servers can have the same export name externally, but still
need different block export ids internally.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 3 +++
include/block/export.h | 3 +++
block/export/export.c | 27 +++
qemu-nbd.c | 1 +
4 files changed, 34 insertions(+)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index d68f3bf87e..0d0db9ca1b 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -179,6 +179,8 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @id: A unique identifier for the block export (across all export types)
+#
# @device: The device name or node name of the node to be exported
#
# @writethrough: If true, caches are flushed after every write request to the
@@ -189,6 +191,7 @@
##
{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
+'id': 'str',
'device': 'str',
'*writethrough': 'bool' },
'discriminator': 'type',
diff --git a/include/block/export.h b/include/block/export.h
index 1698b68f09..43229857b0 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -30,6 +30,9 @@ typedef struct BlockExportDriver {
struct BlockExport {
const BlockExportDriver *drv;
+/* Unique identifier for the export */
+char *id;
+
/*
* Reference count for this block export. This includes strong references
* both from the owner (qemu-nbd or the monitor) and clients connected to
diff --git a/block/export/export.c b/block/export/export.c
index 675db9a8b9..72f1fab975 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -19,6 +19,7 @@
#include "block/nbd.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-block-export.h"
+#include "qemu/id.h"
static const BlockExportDriver* blk_exp_drivers[] = {
_exp_nbd,
@@ -27,6 +28,19 @@ static const BlockExportDriver* blk_exp_drivers[] = {
static QLIST_HEAD(, BlockExport) block_exports =
QLIST_HEAD_INITIALIZER(block_exports);
+static BlockExport *blk_exp_find(const char *id)
+{
+BlockExport *exp;
+
+QLIST_FOREACH(exp, _exports, next) {
+if (strcmp(id, exp->id) == 0) {
+return exp;
+}
+}
+
+return NULL;
+}
+
static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
{
int i;
@@ -45,6 +59,15 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
BlockExport *exp;
int ret;
+if (!id_wellformed(export->id)) {
+error_setg(errp, "Invalid block export id");
+return NULL;
+}
+if (blk_exp_find(export->id)) {
+error_setg(errp, "Block export id '%s' is already in use", export->id);
+return NULL;
+}
+
drv = blk_exp_find_driver(export->type);
if (!drv) {
error_setg(errp, "No driver found for the requested export type");
@@ -55,10 +78,12 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
*exp = (BlockExport) {
.drv= _exp_nbd,
.refcount = 1,
+.id = g_strdup(export->id),
};
ret = drv->create(exp, export, errp);
if (ret < 0) {
+g_free(exp->id);
g_free(exp);
return NULL;
}
@@ -79,6 +104,7 @@ void blk_exp_unref(BlockExport *exp)
if (--exp->refcount == 0) {
QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
+g_free(exp->id);
g_free(exp);
aio_wait_kick();
}
@@ -144,6 +170,7 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error
**errp)
BlockExportOptions *export_opts = g_new(BlockExportOptions, 1);
*export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
+.id = g_strdup(arg->name ?: arg->device),
.device = g_strdup(arg->device),
.u.nbd = {
.has_name = arg->has_name,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 939a08902a..c6fc0581c1 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1055,6 +1055,7 @@ int main(int argc, char **argv)
export_opts = g_new(BlockExportOptions, 1);
*export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
+.id = g_strdup("qemu-nbd-export"),
.device = g_strdup(bdrv_get_node_name(bs)),
.has_writethrough = true,
.writethrough = writethrough,
--
2.25.4
[RFC PATCH 12/22] nbd/server: Simplify export shutdown
Closing export is somewhat convoluted because nbd_export_close() and
nbd_export_put() call each other and the ways they actually end up being
nested is not necessarily obvious.
However, it is not really necessary to call nbd_export_close() from
nbd_export_put() when putting the last reference because it only does
three things:
1. Close all clients. We're going to refcount 0 and all clients hold a
reference, so we know there is no active client any more.
2. Close the user reference (represented by exp->name being non-NULL).
The same argument applies: If the export were still named, we would
still have a reference.
3. Freeing exp->description. This is really cleanup work to be done when
the export is finally freed. There is no reason to already clear it
while clients are still in the process of shutting down.
So after moving the cleanup of exp->description, the code can be
simplified so that only nbd_export_close() calls nbd_export_put(), but
never the other way around.
Signed-off-by: Kevin Wolf
---
nbd/server.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index eadc5b9804..4c594e6558 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1678,8 +1678,6 @@ void nbd_export_close(NBDExport *exp)
QTAILQ_REMOVE(, exp, next);
QTAILQ_INSERT_TAIL(_exports, exp, next);
}
-g_free(exp->description);
-exp->description = NULL;
nbd_export_put(exp);
}
@@ -1706,19 +1704,12 @@ void nbd_export_get(NBDExport *exp)
void nbd_export_put(NBDExport *exp)
{
assert(exp->refcount > 0);
-if (exp->refcount == 1) {
-nbd_export_close(exp);
-}
-
-/* nbd_export_close() may theoretically reduce refcount to 0. It may happen
- * if someone calls nbd_export_put() on named export not through
- * nbd_export_set_name() when refcount is 1. So, let's assert that
- * it is > 0.
- */
-assert(exp->refcount > 0);
if (--exp->refcount == 0) {
assert(exp->name == NULL);
-assert(exp->description == NULL);
+assert(QTAILQ_EMPTY(>clients));
+
+g_free(exp->description);
+exp->description = NULL;
if (exp->blk) {
if (exp->eject_notifier_blk) {
--
2.25.4
[RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport
Having a refcount makes sense for all types of block exports. It is also
a prerequisite for keeping a list of all exports at the BlockExport
level.
Signed-off-by: Kevin Wolf
---
include/block/export.h | 10 ++
include/block/nbd.h| 2 --
block/export/export.c | 14
blockdev-nbd.c | 2 +-
nbd/server.c | 72 +++---
5 files changed, 58 insertions(+), 42 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 5424bdc85d..f44290a4a2 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport;
typedef struct BlockExportDriver {
BlockExportType type;
BlockExport *(*create)(BlockExportOptions *, Error **);
+void (*delete)(BlockExport *);
} BlockExportDriver;
struct BlockExport {
const BlockExportDriver *drv;
+
+/*
+ * Reference count for this block export. This includes strong references
+ * both from the owner (qemu-nbd or the monitor) and clients connected to
+ * the export.
+ */
+int refcount;
};
extern const BlockExportDriver blk_exp_nbd;
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+void blk_exp_ref(BlockExport *exp);
+void blk_exp_unref(BlockExport *exp);
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 23030db3f1..af8509ab70 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
-void nbd_export_get(NBDExport *exp);
-void nbd_export_put(NBDExport *exp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
diff --git a/block/export/export.c b/block/export/export.c
index 12672228c7..1d5de564c7 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -49,6 +49,20 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
return drv->create(export, errp);
}
+void blk_exp_ref(BlockExport *exp)
+{
+assert(exp->refcount > 0);
+exp->refcount++;
+}
+
+void blk_exp_unref(BlockExport *exp)
+{
+assert(exp->refcount > 0);
+if (--exp->refcount == 0) {
+exp->drv->delete(exp);
+}
+}
+
void qmp_block_export_add(BlockExportOptions *export, Error **errp)
{
blk_exp_add(export, errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 8dd127af52..a8b7b785e7 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
/* The list of named exports has a strong reference to this export now and
* our only way of accessing it is through nbd_export_find(), so we can
drop
* the strong reference that is @exp. */
-nbd_export_put(exp);
+blk_exp_unref((BlockExport*) exp);
out:
aio_context_release(aio_context);
diff --git a/nbd/server.c b/nbd/server.c
index 4c594e6558..2bf30bb731 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -83,7 +83,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
-int refcount;
BlockBackend *blk;
char *name;
@@ -499,7 +498,7 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
}
QTAILQ_INSERT_TAIL(>exp->clients, client, next);
-nbd_export_get(client->exp);
+blk_exp_ref(>exp->common);
nbd_check_meta_export(client);
return 0;
@@ -707,7 +706,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
client->exp = exp;
client->check_align = check_align;
QTAILQ_INSERT_TAIL(>exp->clients, client, next);
-nbd_export_get(client->exp);
+blk_exp_ref(>exp->common);
nbd_check_meta_export(client);
rc = 1;
}
@@ -1406,7 +1405,7 @@ void nbd_client_put(NBDClient *client)
g_free(client->tlsauthz);
if (client->exp) {
QTAILQ_REMOVE(>exp->clients, client, next);
-nbd_export_put(client->exp);
+blk_exp_unref(>exp->common);
}
g_free(client);
}
@@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
-.drv = _exp_nbd,
+.drv= _exp_nbd,
+.refcount = 1,
};
/*
@@ -1566,7 +1566,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
blk_set_enable_write_cache(blk, !writethrough);
blk_set_allow_aio_context_change(blk, true);
-exp->refcount = 1;
QTAILQ_INIT(>clients);
exp->blk = blk;
exp->name = g_strdup(name);
@@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
+
[RFC PATCH 15/22] block/export: Move device to BlockExportOptions
Every block export needs a block node to export, so move the 'device'
option from BlockExportOptionsNbd to BlockExportOptions.
To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
to be wrapped by a new type NbdServerAddOptions that adds 'device' back
because nbd-server-add doesn't use the BlockExportOptions base type.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 27 +--
block/export/export.c | 26 --
block/monitor/block-hmp-cmds.c | 6 +++---
blockdev-nbd.c | 4 ++--
qemu-nbd.c | 2 +-
5 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 4ce163411f..d68f3bf87e 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -62,9 +62,8 @@
##
# @BlockExportOptionsNbd:
#
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
+# An NBD block export (options shared between nbd-server-add and the NBD branch
+# of block-export-add).
#
# @name: Export name. If unspecified, the @device parameter is used as the
#export name. (Since 2.12)
@@ -82,8 +81,21 @@
# Since: 5.0
##
{ 'struct': 'BlockExportOptionsNbd',
- 'data': {'device': 'str', '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
+ 'data': { '*name': 'str', '*description': 'str',
+'*writable': 'bool', '*bitmap': 'str' } }
+
+##
+# @NbdServerAddOptions:
+#
+# An NBD block export.
+#
+# @device: The device name or node name of the node to be exported
+#
+# Since: 5.0
+##
+{ 'struct': 'NbdServerAddOptions',
+ 'base': 'BlockExportOptionsNbd',
+ 'data': { 'device': 'str' } }
##
# @nbd-server-add:
@@ -96,7 +108,7 @@
# Since: 1.3.0
##
{ 'command': 'nbd-server-add',
- 'data': 'BlockExportOptionsNbd', 'boxed': true }
+ 'data': 'NbdServerAddOptions', 'boxed': true }
##
# @NbdServerRemoveMode:
@@ -167,6 +179,8 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @device: The device name or node name of the node to be exported
+#
# @writethrough: If true, caches are flushed after every write request to the
#export before completion is signalled. (since: 5.2;
#default: false)
@@ -175,6 +189,7 @@
##
{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
+'device': 'str',
'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
diff --git a/block/export/export.c b/block/export/export.c
index 1d5de564c7..ef86bf892b 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -68,15 +68,26 @@ void qmp_block_export_add(BlockExportOptions *export, Error
**errp)
blk_exp_add(export, errp);
}
-void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
{
BlockExport *export;
BlockDriverState *bs;
BlockBackend *on_eject_blk;
-BlockExportOptions export_opts = {
-.type = BLOCK_EXPORT_TYPE_NBD,
-.u.nbd = *arg,
+BlockExportOptions *export_opts = g_new(BlockExportOptions, 1);
+*export_opts = (BlockExportOptions) {
+.type = BLOCK_EXPORT_TYPE_NBD,
+.device = g_strdup(arg->device),
+.u.nbd = {
+.has_name = arg->has_name,
+.name = g_strdup(arg->name),
+.has_description= arg->has_description,
+.description= g_strdup(arg->description),
+.has_writable = arg->has_writable,
+.writable = arg->writable,
+.has_bitmap = arg->has_bitmap,
+.bitmap = g_strdup(arg->bitmap),
+},
};
/*
@@ -89,9 +100,9 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error
**errp)
arg->writable = false;
}
-export = blk_exp_add(_opts, errp);
+export = blk_exp_add(export_opts, errp);
if (!export) {
-return;
+goto fail;
}
/*
@@ -102,4 +113,7 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error
**errp)
if (on_eject_blk) {
nbd_export_set_on_eject_blk(export, on_eject_blk);
}
+
+fail:
+qapi_free_BlockExportOptions(export_opts);
}
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index a651954e16..6c823234a9 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -398,7 +398,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
Error *local_err = NULL;
BlockInfoList *block_list, *info;
SocketAddress *addr;
-BlockExportOptionsNbd export;
+NbdServerAddOptions export;
if (writable && !all) {
error_setg(_err, "-w only valid together with -a");
@@ -431,7 +431,7 @@ void
[RFC PATCH 10/22] nbd: Remove NBDExport.close callback
The export close callback is unused by the built-in NBD server. qemu-nbd
uses it only during shutdown to wait for the unrefed export to actually
go away. It can just use nbd_export_close_all() instead and do without
the callback.
This removes the close callback from nbd_export_new() and makes both
callers of it more similar.
Signed-off-by: Kevin Wolf
---
include/block/nbd.h | 3 +--
blockdev-nbd.c | 2 +-
nbd/server.c| 9 +
qemu-nbd.c | 14 --
4 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6fc1f05ef4..50e1a46075 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -332,8 +332,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp);
NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- Error **errp);
+ bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 17417c1b6b..d5b084acc2 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -218,7 +218,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
- NULL, exp_args->writethrough, errp);
+ exp_args->writethrough, errp);
if (!exp) {
goto out;
}
diff --git a/nbd/server.c b/nbd/server.c
index 0b84fd30e2..eadc5b9804 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -84,7 +84,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
int refcount;
-void (*close)(NBDExport *exp);
BlockBackend *blk;
char *name;
@@ -1520,8 +1519,7 @@ void nbd_export_set_on_eject_blk(BlockExport *exp,
BlockBackend *blk)
NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- Error **errp)
+ bool writethrough, Error **errp)
{
AioContext *ctx;
BlockBackend *blk;
@@ -1625,7 +1623,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
}
-exp->close = close;
exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -1723,10 +1720,6 @@ void nbd_export_put(NBDExport *exp)
assert(exp->name == NULL);
assert(exp->description == NULL);
-if (exp->close) {
-exp->close(exp);
-}
-
if (exp->blk) {
if (exp->eject_notifier_blk) {
notifier_remove(>eject_notifier);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e348d5d6d8..48aa8a9d46 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -70,7 +70,7 @@ static int verbose;
static char *srcpath;
static SocketAddress *saddr;
static int persistent = 0;
-static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
+static enum { RUNNING, TERMINATE, TERMINATED } state;
static int shared = 1;
static int nb_fds;
static QIONetListener *server;
@@ -331,12 +331,6 @@ static int nbd_can_accept(void)
return state == RUNNING && nb_fds < shared;
}
-static void nbd_export_closed(NBDExport *export)
-{
-assert(state == TERMINATING);
-state = TERMINATED;
-}
-
static void nbd_update_server_watch(void);
static void nbd_client_closed(NBDClient *client, bool negotiated)
@@ -1058,7 +1052,7 @@ int main(int argc, char **argv)
export = nbd_export_new(bs, export_name,
export_description, bitmap, readonly, shared > 1,
-nbd_export_closed, writethrough, _fatal);
+writethrough, _fatal);
if (device) {
#if HAVE_NBD_DEVICE
@@ -1098,10 +1092,10 @@ int main(int argc, char **argv)
do {
main_loop_wait(false);
if (state == TERMINATE) {
-state = TERMINATING;
-nbd_export_close(export);
nbd_export_put(export);
+nbd_export_close_all();
export = NULL;
+state = TERMINATED;
}
} while (state != TERMINATED);
--
2.25.4
[RFC PATCH 11/22] qemu-nbd: Use blk_exp_add() to create the export
With this change, NBD exports are only created through the BlockExport
interface any more. This allows us finally to move things from the NBD
layer to the BlockExport layer if they make sense for other export
types, too.
blk_exp_add() returns only a weak reference, so the explicit
nbd_export_put() goes away.
Signed-off-by: Kevin Wolf
---
include/block/export.h | 2 ++
include/block/nbd.h| 1 +
block/export/export.c | 2 +-
blockdev-nbd.c | 8 +++-
qemu-nbd.c | 28 ++--
5 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index b1d7325403..5424bdc85d 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -29,4 +29,6 @@ struct BlockExport {
extern const BlockExportDriver blk_exp_nbd;
+BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 50e1a46075..23030db3f1 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -350,6 +350,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
+void nbd_server_is_qemu_nbd(bool value);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
const char *tls_authz, uint32_t max_connections,
Error **errp);
diff --git a/block/export/export.c b/block/export/export.c
index 2d5f92861c..12672228c7 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -36,7 +36,7 @@ static const BlockExportDriver
*blk_exp_find_driver(BlockExportType type)
return NULL;
}
-static BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
+BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index d5b084acc2..8dd127af52 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -28,9 +28,15 @@ typedef struct NBDServerData {
} NBDServerData;
static NBDServerData *nbd_server;
+static bool is_qemu_nbd;
static void nbd_update_server_watch(NBDServerData *s);
+void nbd_server_is_qemu_nbd(bool value)
+{
+is_qemu_nbd = value;
+}
+
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
nbd_client_put(client);
@@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
-if (!nbd_server) {
+if (!nbd_server && !is_qemu_nbd) {
error_setg(errp, "NBD server not running");
return NULL;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 48aa8a9d46..d967b8fcb9 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -65,7 +65,6 @@
#define MBR_SIZE 512
-static NBDExport *export;
static int verbose;
static char *srcpath;
static SocketAddress *saddr;
@@ -579,6 +578,7 @@ int main(int argc, char **argv)
int old_stderr = -1;
unsigned socket_activation;
const char *pid_file_name = NULL;
+BlockExportOptions *export_opts;
/* The client thread uses SIGTERM to interrupt the server. A signal
* handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -1050,9 +1050,27 @@ int main(int argc, char **argv)
bs->detect_zeroes = detect_zeroes;
-export = nbd_export_new(bs, export_name,
-export_description, bitmap, readonly, shared > 1,
-writethrough, _fatal);
+nbd_server_is_qemu_nbd(true);
+
+export_opts = g_new(BlockExportOptions, 1);
+*export_opts = (BlockExportOptions) {
+.type = BLOCK_EXPORT_TYPE_NBD,
+.has_writethrough = true,
+.writethrough = writethrough,
+.u.nbd = {
+.device = g_strdup(bdrv_get_node_name(bs)),
+.has_name = true,
+.name = g_strdup(export_name),
+.has_description= !!export_description,
+.description= g_strdup(export_description),
+.has_writable = true,
+.writable = !readonly,
+.has_bitmap = !!bitmap,
+.bitmap = g_strdup(bitmap),
+},
+};
+blk_exp_add(export_opts, _fatal);
+qapi_free_BlockExportOptions(export_opts);
if (device) {
#if HAVE_NBD_DEVICE
@@ -1092,9 +1110,7 @@ int main(int argc, char **argv)
do {
main_loop_wait(false);
if (state == TERMINATE) {
-nbd_export_put(export);
nbd_export_close_all();
-export = NULL;
state = TERMINATED;
}
} while (state != TERMINATED);
--
2.25.4
[RFC PATCH 04/22] block/export: Add BlockExport infrastructure and block-export-add
We want to have a common set of commands for all types of block exports.
Currently, this is only NBD, but we're going to add more types.
This patch adds the basic BlockExport and BlockExportDriver structs and
a QMP command block-export-add that creates a new export based on the
given BlockExportOptions.
qmp_nbd_server_add() becomes a wrapper around qmp_block_export_add().
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 9 ++
include/block/export.h | 32 +
include/block/nbd.h| 3 +-
block/export/export.c | 57 ++
blockdev-nbd.c | 19 -
nbd/server.c | 15 +-
Makefile.objs | 6 ++--
block/Makefile.objs| 2 ++
block/export/Makefile.objs | 1 +
9 files changed, 132 insertions(+), 12 deletions(-)
create mode 100644 include/block/export.h
create mode 100644 block/export/export.c
create mode 100644 block/export/Makefile.objs
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 9332076a05..40369814b4 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -170,3 +170,12 @@
'nbd': 'BlockExportOptionsNbd'
} }
+##
+# @block-export-add:
+#
+# Creates a new block export.
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-add',
+ 'data': 'BlockExportOptions', 'boxed': true }
diff --git a/include/block/export.h b/include/block/export.h
new file mode 100644
index 00..b1d7325403
--- /dev/null
+++ b/include/block/export.h
@@ -0,0 +1,32 @@
+/*
+ * Declarations for block exports
+ *
+ * Copyright (c) 2012, 2020 Red Hat, Inc.
+ *
+ * Authors:
+ * Paolo Bonzini
+ * Kevin Wolf
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#ifndef BLOCK_EXPORT_H
+#define BLOCK_EXPORT_H
+
+#include "qapi/qapi-types-block-export.h"
+
+typedef struct BlockExport BlockExport;
+
+typedef struct BlockExportDriver {
+BlockExportType type;
+BlockExport *(*create)(BlockExportOptions *, Error **);
+} BlockExportDriver;
+
+struct BlockExport {
+const BlockExportDriver *drv;
+};
+
+extern const BlockExportDriver blk_exp_nbd;
+
+#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 262f6da2ce..c8c5cb6b61 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -20,7 +20,7 @@
#ifndef NBD_H
#define NBD_H
-#include "qapi/qapi-types-block-export.h"
+#include "block/export.h"
#include "io/channel-socket.h"
#include "crypto/tlscreds.h"
#include "qapi/error.h"
@@ -328,6 +328,7 @@ int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
+BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
diff --git a/block/export/export.c b/block/export/export.c
new file mode 100644
index 00..3d0dacb3f2
--- /dev/null
+++ b/block/export/export.c
@@ -0,0 +1,57 @@
+/*
+ * Common block export infrastructure
+ *
+ * Copyright (c) 2012, 2020 Red Hat, Inc.
+ *
+ * Authors:
+ * Paolo Bonzini
+ * Kevin Wolf
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/export.h"
+#include "block/nbd.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-block-export.h"
+
+static const BlockExportDriver* blk_exp_drivers[] = {
+_exp_nbd,
+};
+
+static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(blk_exp_drivers); i++) {
+if (blk_exp_drivers[i]->type == type) {
+return blk_exp_drivers[i];
+}
+}
+return NULL;
+}
+
+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+const BlockExportDriver *drv;
+
+drv = blk_exp_find_driver(export->type);
+if (!drv) {
+error_setg(errp, "No driver found for the requested export type");
+return;
+}
+
+drv->create(export, errp);
+}
+
+void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+{
+BlockExportOptions export = {
+.type = BLOCK_EXPORT_TYPE_NBD,
+.u.nbd = *arg,
+};
+qmp_block_export_add(, errp);
+}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 98ee1b6170..a1dc11bdd7 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -148,17 +148,20 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
{
+BlockExportOptionsNbd *arg = _args->u.nbd;
BlockDriverState *bs = NULL;
[RFC PATCH 14/22] block/export: Move AioContext from NBDExport to BlockExport
Signed-off-by: Kevin Wolf
---
include/block/export.h | 6 ++
nbd/server.c | 26 +-
2 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index f44290a4a2..5459f79469 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -33,6 +33,12 @@ struct BlockExport {
* the export.
*/
int refcount;
+
+/*
+ * The AioContex whose lock needs to be held while calling
+ * BlockExportDriver callbacks.
+ */
+AioContext *ctx;
};
extern const BlockExportDriver blk_exp_nbd;
diff --git a/nbd/server.c b/nbd/server.c
index 2bf30bb731..b735a68429 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -92,8 +92,6 @@ struct NBDExport {
QTAILQ_HEAD(, NBDClient) clients;
QTAILQ_ENTRY(NBDExport) next;
-AioContext *ctx;
-
BlockBackend *eject_notifier_blk;
Notifier eject_notifier;
@@ -1333,8 +1331,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client,
Error **errp)
}
/* Attach the channel to the same AioContext as the export */
-if (client->exp && client->exp->ctx) {
-qio_channel_attach_aio_context(client->ioc, client->exp->ctx);
+if (client->exp && client->exp->common.ctx) {
+qio_channel_attach_aio_context(client->ioc, client->exp->common.ctx);
}
assert(!client->optlen);
@@ -1466,7 +1464,7 @@ static void blk_aio_attached(AioContext *ctx, void
*opaque)
trace_nbd_blk_aio_attached(exp->name, ctx);
-exp->ctx = ctx;
+exp->common.ctx = ctx;
QTAILQ_FOREACH(client, >clients, next) {
qio_channel_attach_aio_context(client->ioc, ctx);
@@ -1484,13 +1482,13 @@ static void blk_aio_detach(void *opaque)
NBDExport *exp = opaque;
NBDClient *client;
-trace_nbd_blk_aio_detach(exp->name, exp->ctx);
+trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
QTAILQ_FOREACH(client, >clients, next) {
qio_channel_detach_aio_context(client->ioc);
}
-exp->ctx = NULL;
+exp->common.ctx = NULL;
}
static void nbd_eject_notifier(Notifier *n, void *data)
@@ -1498,7 +1496,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
NBDExport *exp = container_of(n, NBDExport, eject_notifier);
AioContext *aio_context;
-aio_context = exp->ctx;
+aio_context = exp->common.ctx;
aio_context_acquire(aio_context);
nbd_export_close(exp);
aio_context_release(aio_context);
@@ -1534,10 +1532,13 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
return NULL;
}
+ctx = bdrv_get_aio_context(bs);
+
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
.drv= _exp_nbd,
.refcount = 1,
+.ctx= ctx,
};
/*
@@ -1547,7 +1548,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
* ctx was acquired in the caller.
*/
assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
-ctx = bdrv_get_aio_context(bs);
+
bdrv_invalidate_cache(bs, NULL);
/* Don't allow resize while the NBD server is running, otherwise we don't
@@ -1622,7 +1623,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
}
-exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
blk_exp_ref(>common);
@@ -1653,7 +1653,7 @@ NBDExport *nbd_export_find(const char *name)
AioContext *
nbd_export_aio_context(NBDExport *exp)
{
-return exp->ctx;
+return exp->common.ctx;
}
void nbd_export_close(NBDExport *exp)
@@ -1738,7 +1738,7 @@ void nbd_export_close_all(void)
AioContext *aio_context;
QTAILQ_FOREACH_SAFE(exp, , next, next) {
-aio_context = exp->ctx;
+aio_context = exp->common.ctx;
aio_context_acquire(aio_context);
nbd_export_close(exp);
aio_context_release(aio_context);
@@ -2519,7 +2519,7 @@ static void nbd_client_receive_next_request(NBDClient
*client)
if (!client->recv_coroutine && client->nb_requests < MAX_NBD_REQUESTS) {
nbd_client_get(client);
client->recv_coroutine = qemu_coroutine_create(nbd_trip, client);
-aio_co_schedule(client->exp->ctx, client->recv_coroutine);
+aio_co_schedule(client->exp->common.ctx, client->recv_coroutine);
}
}
--
2.25.4
[RFC PATCH 09/22] nbd: Add writethrough to block-export-add
qemu-nbd allows use of writethrough cache modes, which mean that write
requests made through NBD will cause a flush before they complete.
Expose the same functionality in block-export-add.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 7 ++-
blockdev-nbd.c | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 1fdc55c53a..4ce163411f 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -167,10 +167,15 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @writethrough: If true, caches are flushed after every write request to the
+#export before completion is signalled. (since: 5.2;
+#default: false)
+#
# Since: 4.2
##
{ 'union': 'BlockExportOptions',
- 'base': { 'type': 'BlockExportType' },
+ 'base': { 'type': 'BlockExportType',
+'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
'nbd': 'BlockExportOptionsNbd'
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 28159a92b2..17417c1b6b 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -218,7 +218,7 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
- NULL, false, errp);
+ NULL, exp_args->writethrough, errp);
if (!exp) {
goto out;
}
--
2.25.4
[RFC PATCH 08/22] nbd: Add max-connections to nbd-server-start
This is a QMP equivalent of qemu-nbd's --share option, limiting the
maximum number of clients that can attach at the same time.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 10 --
include/block/nbd.h| 3 ++-
block/monitor/block-hmp-cmds.c | 2 +-
blockdev-nbd.c | 33 ++---
qemu-storage-daemon.c | 4 ++--
5 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 40369814b4..1fdc55c53a 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -14,6 +14,8 @@
# is only resolved at time of use, so can be deleted and
# recreated on the fly while the NBD server is active.
# If missing, it will default to denying access (since 4.0).
+# @max-connections: The maximum number of connections to allow at the same
+# time, 0 for unlimited. (since 5.2; default: 0)
#
# Keep this type consistent with the nbd-server-start arguments. The only
# intended difference is using SocketAddress instead of SocketAddressLegacy.
@@ -23,7 +25,8 @@
{ 'struct': 'NbdServerOptions',
'data': { 'addr': 'SocketAddress',
'*tls-creds': 'str',
-'*tls-authz': 'str'} }
+'*tls-authz': 'str',
+'*max-connections': 'uint32' } }
##
# @nbd-server-start:
@@ -40,6 +43,8 @@
# is only resolved at time of use, so can be deleted and
# recreated on the fly while the NBD server is active.
# If missing, it will default to denying access (since 4.0).
+# @max-connections: The maximum number of connections to allow at the same
+# time, 0 for unlimited. (since 5.2; default: 0)
#
# Returns: error if the server is already running.
#
@@ -51,7 +56,8 @@
{ 'command': 'nbd-server-start',
'data': { 'addr': 'SocketAddressLegacy',
'*tls-creds': 'str',
-'*tls-authz': 'str'} }
+'*tls-authz': 'str',
+'*max-connections': 'uint32' } }
##
# @BlockExportOptionsNbd:
diff --git a/include/block/nbd.h b/include/block/nbd.h
index ffca3be78f..6fc1f05ef4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -352,7 +352,8 @@ void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
- const char *tls_authz, Error **errp);
+ const char *tls_authz, uint32_t max_connections,
+ Error **errp);
void nbd_server_start_options(NbdServerOptions *arg, Error **errp);
/* nbd_read
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 56bc83ac97..a651954e16 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -411,7 +411,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
goto exit;
}
-nbd_server_start(addr, NULL, NULL, _err);
+nbd_server_start(addr, NULL, NULL, 0, _err);
qapi_free_SocketAddress(addr);
if (local_err != NULL) {
goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 019c37c0bc..28159a92b2 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -23,23 +23,41 @@ typedef struct NBDServerData {
QIONetListener *listener;
QCryptoTLSCreds *tlscreds;
char *tlsauthz;
+uint32_t max_connections;
+uint32_t connections;
} NBDServerData;
static NBDServerData *nbd_server;
+static void nbd_update_server_watch(NBDServerData *s);
+
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
nbd_client_put(client);
+assert(nbd_server->connections > 0);
+nbd_server->connections--;
+nbd_update_server_watch(nbd_server);
}
static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
gpointer opaque)
{
+nbd_server->connections++;
+nbd_update_server_watch(nbd_server);
+
qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
nbd_blockdev_client_closed);
}
+static void nbd_update_server_watch(NBDServerData *s)
+{
+if (!s->max_connections || s->connections < s->max_connections) {
+qio_net_listener_set_client_func(s->listener, nbd_accept, NULL, NULL);
+} else {
+qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
+}
+}
static void nbd_server_free(NBDServerData *server)
{
@@ -88,7 +106,8 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id,
Error **errp)
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
- const char *tls_authz, Error **errp)
+ const char *tls_authz, uint32_t max_connections,
+ Error **errp)
{
if (nbd_server) {
error_setg(errp, "NBD server already running");
@@ -96,6 +115,7 @@
[RFC PATCH 07/22] block/export: Remove magic from block-export-add
nbd-server-add tries to be convenient and adds two questionable
features that we don't want to share in block-export-add, even for NBD
exports:
1. When requesting a writable export of a read-only device, the export
is silently downgraded to read-only. This should be an error in the
context of block-export-add.
2. When using a BlockBackend name, unplugging the device from the guest
will automatically stop the NBD server, too. This may sometimes be
what you want, but it could also be very surprising. Let's keep
things explicit with block-export-add. If the user wants to stop the
export, they should tell us so.
Move these things into the nbd-server-add QMP command handler so that
they apply only there.
Signed-off-by: Kevin Wolf
---
include/block/nbd.h | 3 ++-
block/export/export.c | 44 ++-
blockdev-nbd.c| 10 --
nbd/server.c | 19 ---
qemu-nbd.c| 3 +--
5 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 3846d2bac8..ffca3be78f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -333,7 +333,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
- BlockBackend *on_eject_blk, Error **errp);
+ Error **errp);
+void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
void nbd_export_get(NBDExport *exp);
diff --git a/block/export/export.c b/block/export/export.c
index 3d0dacb3f2..2d5f92861c 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -13,6 +13,8 @@
#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
#include "block/export.h"
#include "block/nbd.h"
#include "qapi/error.h"
@@ -34,24 +36,56 @@ static const BlockExportDriver
*blk_exp_find_driver(BlockExportType type)
return NULL;
}
-void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+static BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
drv = blk_exp_find_driver(export->type);
if (!drv) {
error_setg(errp, "No driver found for the requested export type");
-return;
+return NULL;
}
-drv->create(export, errp);
+return drv->create(export, errp);
+}
+
+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+blk_exp_add(export, errp);
}
void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
{
-BlockExportOptions export = {
+BlockExport *export;
+BlockDriverState *bs;
+BlockBackend *on_eject_blk;
+
+BlockExportOptions export_opts = {
.type = BLOCK_EXPORT_TYPE_NBD,
.u.nbd = *arg,
};
-qmp_block_export_add(, errp);
+
+/*
+ * nbd-server-add doesn't complain when a read-only device should be
+ * exported as writable, but simply downgrades it. This is an error with
+ * block-export-add.
+ */
+bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
+if (bs && bdrv_is_read_only(bs)) {
+arg->writable = false;
+}
+
+export = blk_exp_add(_opts, errp);
+if (!export) {
+return;
+}
+
+/*
+ * nbd-server-add removes the export when the named BlockBackend used for
+ * @device goes away.
+ */
+on_eject_blk = blk_by_name(arg->device);
+if (on_eject_blk) {
+nbd_export_set_on_eject_blk(export, on_eject_blk);
+}
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 16cda3b052..019c37c0bc 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -152,7 +152,6 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
{
BlockExportOptionsNbd *arg = _args->u.nbd;
BlockDriverState *bs = NULL;
-BlockBackend *on_eject_blk;
NBDExport *exp = NULL;
AioContext *aio_context;
@@ -182,8 +181,6 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
return NULL;
}
-on_eject_blk = blk_by_name(arg->device);
-
bs = bdrv_lookup_bs(arg->device, arg->device, errp);
if (!bs) {
return NULL;
@@ -195,13 +192,14 @@ BlockExport *nbd_export_create(BlockExportOptions
*exp_args, Error **errp)
if (!arg->has_writable) {
arg->writable = false;
}
-if (bdrv_is_read_only(bs)) {
-arg->writable = false;
+if (bdrv_is_read_only(bs) && arg->writable) {
+error_setg(errp, "Cannot export read-only node as writable");
+goto out;
}
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
[RFC PATCH 01/22] nbd: Remove unused nbd_export_get_blockdev()
Signed-off-by: Kevin Wolf
---
include/block/nbd.h | 2 --
nbd/server.c| 5 -
2 files changed, 7 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9bc3bfaeec..0451683d03 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -338,8 +338,6 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode
mode, Error **errp);
void nbd_export_get(NBDExport *exp);
void nbd_export_put(NBDExport *exp);
-BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
-
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
void nbd_export_close_all(void);
diff --git a/nbd/server.c b/nbd/server.c
index 4752a6c8bc..bee2ef8bd1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1731,11 +1731,6 @@ void nbd_export_put(NBDExport *exp)
}
}
-BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
-{
-return exp->blk;
-}
-
void nbd_export_close_all(void)
{
NBDExport *exp, *next;
--
2.25.4
[RFC PATCH 00/22] block/export: Add infrastructure and QAPI for block exports
We are planning to add more block export types than just NBD in the near future (e.g. vhost-user-blk and FUSE). This series lays the ground for this with some generic block export infrastructure and QAPI interfaces that will allow managing all of them (for now add/remove/query). As a side effect, qemu-storage-daemon can now map --export directly to the block-export-add QMP command, similar to other command line options. The built-in NBD servers also gains new options that bring it at least a little closer to feature parity with qemu-nbd. This series is still RFC because the patches aren't very polished (though they also shouldn't be too bad), it's not completely clear if more features should be moved from NBD to the BlockExport layer in this series (or maybe even less, and the rest dealt with in a separate series) and qemu-iotests cases for the new interfaces are still missing. Kevin Wolf (22): nbd: Remove unused nbd_export_get_blockdev() qapi: Create block-export module qapi: Rename BlockExport to BlockExportOptions block/export: Add BlockExport infrastructure and block-export-add qemu-storage-daemon: Use qmp_block_export_add() qemu-nbd: Use raw block driver for --offset block/export: Remove magic from block-export-add nbd: Add max-connections to nbd-server-start nbd: Add writethrough to block-export-add nbd: Remove NBDExport.close callback qemu-nbd: Use blk_exp_add() to create the export nbd/server: Simplify export shutdown block/export: Move refcount from NBDExport to BlockExport block/export: Move AioContext from NBDExport to BlockExport block/export: Move device to BlockExportOptions block/export: Allocate BlockExport in blk_exp_add() block/export: Add blk_exp_close_all(_type) block/export: Add 'id' option to block-export-add block/export: Move strong user reference to block_exports block/export: Add block-export-del block/export: Move blk to BlockExport block/export: Add query-block-exports qapi/block-core.json | 166 --- qapi/block-export.json | 261 +++ qapi/qapi-schema.json| 1 + include/block/export.h | 73 +++ include/block/nbd.h | 25 +-- block.c | 2 +- block/export/export.c| 297 +++ block/monitor/block-hmp-cmds.c | 11 +- blockdev-nbd.c | 124 +-- nbd/server.c | 243 +- qemu-nbd.c | 67 +++--- qemu-storage-daemon.c| 25 +-- Makefile.objs| 6 +- block/Makefile.objs | 2 + block/export/Makefile.objs | 1 + qapi/Makefile.objs | 5 +- storage-daemon/qapi/qapi-schema.json | 1 + 17 files changed, 857 insertions(+), 453 deletions(-) create mode 100644 qapi/block-export.json create mode 100644 include/block/export.h create mode 100644 block/export/export.c create mode 100644 block/export/Makefile.objs -- 2.25.4
[RFC PATCH 05/22] qemu-storage-daemon: Use qmp_block_export_add()
No reason to duplicate the functionality locally, we can now just reuse
the QMP command block-export-add for --export.
Signed-off-by: Kevin Wolf
---
qemu-storage-daemon.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index ed26097254..b6f678d3ab 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -150,17 +150,6 @@ static void init_qmp_commands(void)
qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
}
-static void init_export(BlockExportOptions *export, Error **errp)
-{
-switch (export->type) {
-case BLOCK_EXPORT_TYPE_NBD:
-qmp_nbd_server_add(>u.nbd, errp);
-break;
-default:
-g_assert_not_reached();
-}
-}
-
static void process_options(int argc, char *argv[])
{
int c;
@@ -241,7 +230,7 @@ static void process_options(int argc, char *argv[])
visit_type_BlockExportOptions(v, NULL, , _fatal);
visit_free(v);
-init_export(export, _fatal);
+qmp_block_export_add(export, _fatal);
qapi_free_BlockExportOptions(export);
break;
}
--
2.25.4
[RFC PATCH 03/22] qapi: Rename BlockExport to BlockExportOptions
The name BlockExport will be used for the struct containing the runtime
state of block exports, so change the name of export creation options.
Signed-off-by: Kevin Wolf
---
qapi/block-export.json | 12 ++--
block/monitor/block-hmp-cmds.c | 6 +++---
blockdev-nbd.c | 2 +-
qemu-storage-daemon.c | 8
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 62f4938e83..9332076a05 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -54,7 +54,7 @@
'*tls-authz': 'str'} }
##
-# @BlockExportNbd:
+# @BlockExportOptionsNbd:
#
# An NBD block export.
#
@@ -75,7 +75,7 @@
#
# Since: 5.0
##
-{ 'struct': 'BlockExportNbd',
+{ 'struct': 'BlockExportOptionsNbd',
'data': {'device': 'str', '*name': 'str', '*description': 'str',
'*writable': 'bool', '*bitmap': 'str' } }
@@ -90,7 +90,7 @@
# Since: 1.3.0
##
{ 'command': 'nbd-server-add',
- 'data': 'BlockExportNbd', 'boxed': true }
+ 'data': 'BlockExportOptionsNbd', 'boxed': true }
##
# @NbdServerRemoveMode:
@@ -156,17 +156,17 @@
'data': [ 'nbd' ] }
##
-# @BlockExport:
+# @BlockExportOptions:
#
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
# Since: 4.2
##
-{ 'union': 'BlockExport',
+{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType' },
'discriminator': 'type',
'data': {
- 'nbd': 'BlockExportNbd'
+ 'nbd': 'BlockExportOptionsNbd'
} }
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index fb9d87ee89..56bc83ac97 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -398,7 +398,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
Error *local_err = NULL;
BlockInfoList *block_list, *info;
SocketAddress *addr;
-BlockExportNbd export;
+BlockExportOptionsNbd export;
if (writable && !all) {
error_setg(_err, "-w only valid together with -a");
@@ -431,7 +431,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
continue;
}
-export = (BlockExportNbd) {
+export = (BlockExportOptionsNbd) {
.device = info->value->device,
.has_writable = true,
.writable = writable,
@@ -458,7 +458,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
bool writable = qdict_get_try_bool(qdict, "writable", false);
Error *local_err = NULL;
-BlockExportNbd export = {
+BlockExportOptionsNbd export = {
.device = (char *) device,
.has_name = !!name,
.name = (char *) name,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 0f6b80c58f..98ee1b6170 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -148,7 +148,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-void qmp_nbd_server_add(BlockExportNbd *arg, Error **errp)
+void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
{
BlockDriverState *bs = NULL;
BlockBackend *on_eject_blk;
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index ed9d2afcf3..ed26097254 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -150,7 +150,7 @@ static void init_qmp_commands(void)
qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
}
-static void init_export(BlockExport *export, Error **errp)
+static void init_export(BlockExportOptions *export, Error **errp)
{
switch (export->type) {
case BLOCK_EXPORT_TYPE_NBD:
@@ -235,14 +235,14 @@ static void process_options(int argc, char *argv[])
case OPTION_EXPORT:
{
Visitor *v;
-BlockExport *export;
+BlockExportOptions *export;
v = qobject_input_visitor_new_str(optarg, "type",
_fatal);
-visit_type_BlockExport(v, NULL, , _fatal);
+visit_type_BlockExportOptions(v, NULL, , _fatal);
visit_free(v);
init_export(export, _fatal);
-qapi_free_BlockExport(export);
+qapi_free_BlockExportOptions(export);
break;
}
case OPTION_MONITOR:
--
2.25.4
[RFC PATCH 02/22] qapi: Create block-export module
Move all block export related types and commands from block-core to the
new QAPI module block-export.
Signed-off-by: Kevin Wolf
---
qapi/block-core.json | 166 --
qapi/block-export.json | 172 +++
qapi/qapi-schema.json| 1 +
include/block/nbd.h | 2 +-
block/monitor/block-hmp-cmds.c | 1 +
blockdev-nbd.c | 2 +-
qemu-storage-daemon.c| 2 +-
qapi/Makefile.objs | 5 +-
storage-daemon/qapi/qapi-schema.json | 1 +
9 files changed, 181 insertions(+), 171 deletions(-)
create mode 100644 qapi/block-export.json
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab7bf3c612..5c491d4cbd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5176,172 +5176,6 @@
'iothread': 'StrOrNull',
'*force': 'bool' } }
-##
-# @NbdServerOptions:
-#
-# @addr: Address on which to listen.
-# @tls-creds: ID of the TLS credentials object (since 2.6).
-# @tls-authz: ID of the QAuthZ authorization object used to validate
-# the client's x509 distinguished name. This object is
-# is only resolved at time of use, so can be deleted and
-# recreated on the fly while the NBD server is active.
-# If missing, it will default to denying access (since 4.0).
-#
-# Keep this type consistent with the nbd-server-start arguments. The only
-# intended difference is using SocketAddress instead of SocketAddressLegacy.
-#
-# Since: 4.2
-##
-{ 'struct': 'NbdServerOptions',
- 'data': { 'addr': 'SocketAddress',
-'*tls-creds': 'str',
-'*tls-authz': 'str'} }
-
-##
-# @nbd-server-start:
-#
-# Start an NBD server listening on the given host and port. Block
-# devices can then be exported using @nbd-server-add. The NBD
-# server will present them as named exports; for example, another
-# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
-#
-# @addr: Address on which to listen.
-# @tls-creds: ID of the TLS credentials object (since 2.6).
-# @tls-authz: ID of the QAuthZ authorization object used to validate
-# the client's x509 distinguished name. This object is
-# is only resolved at time of use, so can be deleted and
-# recreated on the fly while the NBD server is active.
-# If missing, it will default to denying access (since 4.0).
-#
-# Returns: error if the server is already running.
-#
-# Keep this type consistent with the NbdServerOptions type. The only intended
-# difference is using SocketAddressLegacy instead of SocketAddress.
-#
-# Since: 1.3.0
-##
-{ 'command': 'nbd-server-start',
- 'data': { 'addr': 'SocketAddressLegacy',
-'*tls-creds': 'str',
-'*tls-authz': 'str'} }
-
-##
-# @BlockExportNbd:
-#
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
-#
-# @name: Export name. If unspecified, the @device parameter is used as the
-#export name. (Since 2.12)
-#
-# @description: Free-form description of the export, up to 4096 bytes.
-# (Since 5.0)
-#
-# @writable: Whether clients should be able to write to the device via the
-#NBD connection (default false).
-#
-# @bitmap: Also export the dirty bitmap reachable from @device, so the
-# NBD client can use NBD_OPT_SET_META_CONTEXT with
-# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
-#
-# Since: 5.0
-##
-{ 'struct': 'BlockExportNbd',
- 'data': {'device': 'str', '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
-
-##
-# @nbd-server-add:
-#
-# Export a block node to QEMU's embedded NBD server.
-#
-# Returns: error if the server is not running, or export with the same name
-# already exists.
-#
-# Since: 1.3.0
-##
-{ 'command': 'nbd-server-add',
- 'data': 'BlockExportNbd', 'boxed': true }
-
-##
-# @NbdServerRemoveMode:
-#
-# Mode for removing an NBD export.
-#
-# @safe: Remove export if there are no existing connections, fail otherwise.
-#
-# @hard: Drop all connections immediately and remove export.
-#
-# Potential additional modes to be added in the future:
-#
-# hide: Just hide export from new clients, leave existing connections as is.
-# Remove export after all clients are disconnected.
-#
-# soft: Hide export from new clients, answer with ESHUTDOWN for all further
-# requests from existing clients.
-#
-# Since: 2.12
-##
-{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
-
-##
-# @nbd-server-remove:
-#
-# Remove NBD export by name.
-#
-# @name: Export name.
-#
-# @mode: Mode of command operation. See @NbdServerRemoveMode description.
-#Default is 'safe'.
-#
-# Returns: error if
-#- the server is not running
-#- export is not found
-#- mode is 'safe' and there are existing connections
-#
-# Since:
Re: What is bs->reqs_lock for?
On 13/08/20 16:57, Vladimir Sementsov-Ogievskiy wrote: > Hi! > > Sorry my stupid question, but which kind of concurrent access > bs->reqs_lock prevents? > > In my understanding the whole logic of request tracking for the bs is > going in the coroutine, so, we don't have parallel access anyway? How > can parallel access to bs->tracked_requests happen? Different iothreads can access the same BlockDriverState, and block/io.c is not protected by the AioContext lock (in fact almost nothing, or nothing, needs it in the I/O path). Paolo
What is bs->reqs_lock for?
Hi! Sorry my stupid question, but which kind of concurrent access bs->reqs_lock prevents? In my understanding the whole logic of request tracking for the bs is going in the coroutine, so, we don't have parallel access anyway? How can parallel access to bs->tracked_requests happen? -- Best regards, Vladimir
[PATCH v3] block: Raise an error when backing file parameter is an empty string
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
v3:
- Moved test case into 049 instead of taking up
298.
v2:
- Removed 4 spaces to resolve pylint warning
- Updated format to be 'iotests.imgfmt' instead
of hardcoding 'qcow2'
- Use temporary file instead of '/tmp/foo'
- Give a size parameter to qemu-img
- Run test for qcow2, qcow, qed and *not* raw
block.c| 4
tests/qemu-iotests/049 | 4
tests/qemu-iotests/049.out | 5 +
3 files changed, 13 insertions(+)
diff --git a/block.c b/block.c
index d9ac0e07eb..1f72275b87 100644
--- a/block.c
+++ b/block.c
@@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"same filename as the backing file");
goto out;
}
+if (backing_file[0] == '\0') {
+error_setg(errp, "Expected backing file name, got empty string");
+goto out;
+}
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/049 b/tests/qemu-iotests/049
index 051a1c79e0..82b1e6c202 100755
--- a/tests/qemu-iotests/049
+++ b/tests/qemu-iotests/049
@@ -119,6 +119,10 @@ test_qemu_img create -f $IMGFMT -o
compat=1.1,lazy_refcounts=on "$TEST_IMG" 64M
test_qemu_img create -f $IMGFMT -o compat=0.10,lazy_refcounts=off "$TEST_IMG"
64M
test_qemu_img create -f $IMGFMT -o compat=0.10,lazy_refcounts=on "$TEST_IMG"
64M
+echo "== Expect error when backing file name is empty string =="
+echo
+test_qemu_img create -f $IMGFMT -b '' $TEST_IMG 1M
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 4c21dc70a5..61ee90b10e 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -209,4 +209,9 @@ qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=on
TEST_DIR/t.qcow2 64M
Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536
compression_type=zlib size=67108864 compat=0.10 lazy_refcounts=on
refcount_bits=16
qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility
level 1.1 and above (use version=v3 or greater)
+== Expect error when backing file name is empty string ==
+
+qemu-img create -f qcow2 -b TEST_DIR/t.qcow2 1M
+qemu-img: TEST_DIR/t.qcow2: Expected backing file name, got empty string
+
*** done
--
2.25.4
Re: [PATCH for-5.2 v3 0/3] migration: Add block-bitmap-mapping parameter
On 12.08.20 16:38, Vladimir Sementsov-Ogievskiy wrote: > Now this doesn't apply, as code changed a lot after my series > "[PATCH v4 for-5.1 00/21] Fix error handling during bitmap postcopy" > merged first. Yeah, well. Yeah. > I feel my responsibility for the mess with these series, so if you want > I can try to rebase and post v4 of this one myself :) I feel like rebasing is simpler than reviewing, so I’ll be egoistic and choose the rebaser’s side. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH for-5.2 v3 1/3] migration: Add block-bitmap-mapping parameter
On 12.08.20 16:32, Eric Blake wrote:
> On 7/22/20 3:05 AM, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Max Reitz
>> ---
>> qapi/migration.json | 104 -
>> migration/migration.h | 3 +
>> migration/block-dirty-bitmap.c | 373 -
>> migration/migration.c | 30 +++
>> monitor/hmp-cmds.c | 30 +++
>> 5 files changed, 485 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..d7e953cd73 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -507,6 +507,44 @@
>> 'data': [ 'none', 'zlib',
>> { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>> +##
>> +# @BitmapMigrationBitmapAlias:
>> +#
>> +# @name: The name of the bitmap.
>> +#
>> +# @alias: An alias name for migration (for example the bitmap name on
>> +# the opposite site).
>> +#
>> +# Since: 5.1
>
> 5.2, now, but I can touch that up if it is the only problem raised.
Yes, somehow I missed this, and amended it to 5.2 elsewhere...
>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> +# aliases for the purpose of dirty bitmap migration. Such
>> +# aliases may for example be the corresponding names on the
>> +# opposite site.
>> +# The mapping must be one-to-one, and on the destination also
>> +# complete: On the source, bitmaps on nodes where either the
>> +# bitmap or the node is not mapped will be ignored. In
>> +# contrast, on the destination, receiving a bitmap (by alias)
>> +# from a node (by alias) when either alias is not mapped will
>> +# result in an error.
>
> Grammar is a bit odd and it feels repetitive. How about:
>
> The mapping must not provide more than one alias for a bitmap, nor reuse
> the same alias across multiple bitmaps in the same node.
This describes an injective function, as in, one-to-one.
> On the source,
> an omitted node or bitmap within a node will ignore those bitmaps. In
> contrast, on the destination, receiving a bitmap (by alias) from a node
> (by alias) when either alias is not mapped will result in an error.
This will need a rewrite anyway. Because of Vladimir’s patches you
merged in rc2, it looks like it makes more sense now to ignore unknown
aliases on the destination. So I’ll have to think up something new anyway.
>> +# Note that the destination does not know about bitmaps it
>> +# does not receive, so there is no limitation or requirement
>> +# regarding the number of bitmaps received, or how they are
>> +# named, or on which nodes they are placed.
>> +# By default (when this parameter has never been set), bitmap
>> +# names are mapped to themselves. Nodes are mapped to their
>> +# block device name if there is one, and to their node name
>> +# otherwise. (Since 5.2)
>
> Looks good.
>
>
>> @@ -781,6 +839,25 @@
>> # will consume more CPU.
>> # Defaults to 1. (Since 5.0)
>> #
>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> +# aliases for the purpose of dirty bitmap migration. Such
>> +# aliases may for example be the corresponding names on the
>> +# opposite site.
>
> Ah, the joys of duplicated text.
>
>> +++ b/migration/block-dirty-bitmap.c
>
>> @@ -128,7 +131,8 @@ typedef struct DirtyBitmapMigState {
>> typedef struct DirtyBitmapLoadState {
>> uint32_t flags;
>> - char node_name[256];
>> + char node_alias[256];
>> + char bitmap_alias[256];
>
> Do we properly check that alias names will never overflow?
Hm, well. There are assertions guarding against that, but they’re
assertions.
That’s an existing problem, actually. If you give a bitmap a name
longer than 255 bytes, you run into the same failed assertion.
>> +static GHashTable *construct_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> + bool name_to_alias,
>> + Error **errp)
>> +{
>> + GHashTable *alias_map;
>> +
>> + alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
>> + g_free,
>> free_alias_map_inner_node);
>> +
>> + for (; bbm; bbm = bbm->next) {
>> + const BitmapMigrationNodeAlias *bmna = bbm->value;
>> + const BitmapMigrationBitmapAliasList *bmbal;
>> + AliasMapInnerNode *amin;
>> +
Re: [PATCH v2] block: Raise an error when backing file parameter is an empty string
On 8/12/20 1:58 AM, Kevin Wolf wrote: This looks like a test case that would be better served by not using QMPTestCase, but just printing the qemu-img output and having the message compared against the reference output. In fact, there is already 049 for testing some qemu-img create options and we could just add a line there (or multiple lines to cover other backing file related error cases, too). Putting it there would both simplify the test code and keep 298 free for the other series. None of the above is really a reason to reject the patch. I guess this is more of a "are you sure? (y/n)" before I apply it. :-) Hi Kevin! Thanks for the review :-) I think it'd be best for my own edification to address your comments here instead of applying this now. I'll send a v3. Connor Kevin
Re: [PULL 0/9] Tracing patches
On Thu, Aug 13, 2020 at 03:47:16AM -0700, no-re...@patchew.org wrote: > TESTcheck-unit: tests/test-char > Unexpected error in object_property_try_add() at > /tmp/qemu-test/src/qom/object.c:1181: > attempt to add duplicate property 'serial-id' to object (type 'container') > ERROR test-char - too few tests run (expected 38, got 9) Pre-existing issue. This error is also affecting other patch series. Stefan signature.asc Description: PGP signature
Re: [PULL 0/9] Tracing patches
Patchew URL: https://patchew.org/QEMU/20200813052257.226142-1-stefa...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === TESTcheck-unit: tests/test-char Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181: attempt to add duplicate property 'serial-id' to object (type 'container') ERROR test-char - too few tests run (expected 38, got 9) make: *** [check-unit] Error 1 make: *** Waiting for unfinished jobs qemu-system-aarch64: -accel kvm: invalid accelerator kvm qemu-system-aarch64: falling back to tcg --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=5808c97c711e485db979e5289207613b', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-wo4v6nnp/src/docker-src.2020-08-13-06.32.42.31813:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=5808c97c711e485db979e5289207613b make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-wo4v6nnp/src' make: *** [docker-run-test-quick@centos7] Error 2 real14m32.450s user0m8.381s The full log is available at http://patchew.org/logs/20200813052257.226142-1-stefa...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PULL 0/9] Tracing patches
Patchew URL:
https://patchew.org/QEMU/20200813052257.226142-1-stefa...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200813052257.226142-1-stefa...@redhat.com
Subject: [PULL 0/9] Tracing patches
=== 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 ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
ab1e0e0 trace-events: Fix attribution of trace points to source
b4e6c94 trace-events: Delete unused trace points
967b100 scripts/cleanup-trace-events: Emit files in alphabetical order
a3b5483 scripts/cleanup-trace-events: Fix for vcpu property
c846bf7 softmmu: Add missing trace-events file
63d5982 net/colo: Match is-enabled probe to tracepoint
75eab42 build: Don't make object files for dtrace on macOS
28b2951 scripts/tracetool: Use void pointer for vcpu
dc44f9f scripts/tracetool: Fix dtrace generation for macOS
=== OUTPUT BEGIN ===
1/9 Checking commit dc44f9f395ff (scripts/tracetool: Fix dtrace generation for
macOS)
2/9 Checking commit 28b2951555cd (scripts/tracetool: Use void pointer for vcpu)
3/9 Checking commit 75eab42de18c (build: Don't make object files for dtrace on
macOS)
4/9 Checking commit 63d598213058 (net/colo: Match is-enabled probe to
tracepoint)
5/9 Checking commit c846bf7afef4 (softmmu: Add missing trace-events file)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#76:
new file mode 100644
total: 0 errors, 1 warnings, 111 lines checked
Patch 5/9 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/9 Checking commit a3b5483d4283 (scripts/cleanup-trace-events: Fix for vcpu
property)
ERROR: code indent should never use tabs
#33: FILE: scripts/cleanup-trace-events.pl:38:
+^Idefined $3 ? () : ('--max-depth', '1'),$
ERROR: code indent should never use tabs
#34: FILE: scripts/cleanup-trace-events.pl:39:
+^I$pat$
total: 2 errors, 0 warnings, 16 lines checked
Patch 6/9 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/9 Checking commit 967b100f8520 (scripts/cleanup-trace-events: Emit files in
alphabetical order)
8/9 Checking commit b4e6c94dad64 (trace-events: Delete unused trace points)
9/9 Checking commit ab1e0e0ff43d (trace-events: Fix attribution of trace points
to source)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200813052257.226142-1-stefa...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[PULL 9/9] trace-events: Fix attribution of trace points to source
From: Markus Armbruster Some trace points are attributed to the wrong source file. Happens when we neglect to update trace-events for code motion, or add events in the wrong place, or misspell the file name. Clean up with help of scripts/cleanup-trace-events.pl. Funnies requiring manual post-processing: * accel/tcg/cputlb.c trace points are in trace-events. * block.c and blockdev.c trace points are in block/trace-events. * hw/block/nvme.c uses the preprocessor to hide its trace point use from cleanup-trace-events.pl. * hw/tpm/tpm_spapr.c uses pseudo trace point tpm_spapr_show_buffer to guard debug code. * include/hw/xen/xen_common.h trace points are in hw/xen/trace-events. * linux-user/trace-events abbreviates a tedious list of filenames to */signal.c. * net/colo-compare and net/filter-rewriter.c use pseudo trace points colo_compare_miscompare and colo_filter_rewriter_debug to guard debug code. Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Message-id: 20200806141334.3646302-5-arm...@redhat.com Signed-off-by: Stefan Hajnoczi --- block/trace-events| 5 ++--- hw/block/trace-events | 2 +- hw/char/trace-events | 2 +- hw/display/trace-events | 4 +++- hw/hyperv/trace-events| 2 +- hw/mips/trace-events | 2 +- hw/misc/trace-events | 8 +--- hw/ppc/trace-events | 6 ++ hw/riscv/trace-events | 2 +- hw/rtc/trace-events | 2 +- hw/tpm/trace-events | 2 +- hw/usb/trace-events | 4 +++- hw/vfio/trace-events | 10 ++ hw/virtio/trace-events| 2 +- migration/trace-events| 36 +++- target/riscv/trace-events | 2 +- trace-events | 5 +++-- ui/trace-events | 6 +++--- util/trace-events | 4 +++- 19 files changed, 58 insertions(+), 48 deletions(-) diff --git a/block/trace-events b/block/trace-events index 778140d304..916c442277 100644 --- a/block/trace-events +++ b/block/trace-events @@ -58,12 +58,10 @@ qmp_block_job_finalize(void *job) "job %p" qmp_block_job_dismiss(void *job) "job %p" qmp_block_stream(void *bs) "bs %p" -# file-posix.c # file-win32.c file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d" -file_copy_file_range(void *bs, int src, int64_t src_off, int dst, int64_t dst_off, int64_t bytes, int flags, int64_t ret) "bs %p src_fd %d offset %"PRIu64" dst_fd %d offset %"PRIu64" bytes %"PRIu64" flags %d ret %"PRId64 -#io_uring.c +# io_uring.c luring_init_state(void *s, size_t size) "s %p size %zu" luring_cleanup_state(void *s) "%p freed" luring_io_plug(void *s) "LuringState %p plug" @@ -199,6 +197,7 @@ curl_setup_preadv(uint64_t bytes, uint64_t start, const char *range) "reading %" curl_close(void) "close" # file-posix.c +file_copy_file_range(void *bs, int src, int64_t src_off, int dst, int64_t dst_off, int64_t bytes, int flags, int64_t ret) "bs %p src_fd %d offset %"PRIu64" dst_fd %d offset %"PRIu64" bytes %"PRIu64" flags %d ret %"PRId64 file_FindEjectableOpticalMedia(const char *media) "Matching using %s" file_setup_cdrom(const char *partition) "Using %s as optical disc" file_hdev_is_sg(int type, int version) "SG device found: type=%d, version=%d" diff --git a/hw/block/trace-events b/hw/block/trace-events index 958fcc5508..857ac5645e 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -4,8 +4,8 @@ fdc_ioport_read(uint8_t reg, uint8_t value) "read reg 0x%02x val 0x%02x" fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x" -# pflash_cfi02.c # pflash_cfi01.c +# pflash_cfi02.c pflash_reset(void) "reset" pflash_timer_expired(uint8_t cmd) "command 0x%02x done" pflash_io_read(uint64_t offset, unsigned size, uint32_t value, uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" size:%u value:0x%04x cmd:0x%02x wcycle:%u" diff --git a/hw/char/trace-events b/hw/char/trace-events index d20eafd56f..2442a9f7d5 100644 --- a/hw/char/trace-events +++ b/hw/char/trace-events @@ -98,5 +98,5 @@ exynos_uart_rxsize(uint32_t channel, uint32_t size) "UART%d: Rx FIFO size: %d" exynos_uart_channel_error(uint32_t channel) "Wrong UART channel number: %d" exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d: Rx timeout stat=0x%x intsp=0x%x" -# hw/char/cadence_uart.c +# cadence_uart.c cadence_uart_baudrate(unsigned baudrate) "baudrate %u" diff --git a/hw/display/trace-events b/hw/display/trace-events index 970d6bac5d..957b8ba994 100644 --- a/hw/display/trace-events +++ b/hw/display/trace-events @@ -32,9 +32,11 @@ vmware_scratch_read(uint32_t index, uint32_t value) "index %d, value 0x%x" vmware_scratch_write(uint32_t index, uint32_t value) "index %d, value 0x%x" vmware_setmode(uint32_t w, uint32_t h, uint32_t bpp) "%dx%d @ %d bpp" +# virtio-gpu-base.c +virtio_gpu_features(bool virgl) "virgl %d" + # virtio-gpu-3d.c # virtio-gpu.c
[PULL 7/9] scripts/cleanup-trace-events: Emit files in alphabetical order
From: Markus Armbruster
Signed-off-by: Markus Armbruster
Message-id: 20200806141334.3646302-3-arm...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
scripts/cleanup-trace-events.pl | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/cleanup-trace-events.pl b/scripts/cleanup-trace-events.pl
index 6c3767bd6a..c40d2fcc50 100755
--- a/scripts/cleanup-trace-events.pl
+++ b/scripts/cleanup-trace-events.pl
@@ -15,12 +15,15 @@ use warnings;
use strict;
use File::Basename;
-my $buf = '';
+my @files = ();
+my $events = '';
my %seen = ();
sub out {
-print $buf;
-$buf = '';
+print sort @files;
+print $events;
+@files = ();
+$events = '';
%seen = ();
}
@@ -42,7 +45,7 @@ while () {
chomp $fname;
next if $seen{$fname} || $fname eq 'trace-events';
$seen{$fname} = 1;
-$buf = "# $fname\n" . $buf;
+push @files, "# $fname\n";
}
unless (close GREP) {
die "close git grep: $!"
@@ -55,7 +58,7 @@ while () {
} elsif (!/^#|^$/) {
warn "unintelligible line";
}
-$buf .= $_;
+$events .= $_;
}
out;
--
2.26.2
[PULL 6/9] scripts/cleanup-trace-events: Fix for vcpu property
From: Markus Armbruster
Commit a44cf524f8 "scripts/cleanup-trace-events: Update for current
practice" limited search to the input file's directory. That's wrong
for events with the vcpu property, because these can only be defined
in root directory.
Signed-off-by: Markus Armbruster
Message-id: 20200806141334.3646302-2-arm...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
scripts/cleanup-trace-events.pl | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/scripts/cleanup-trace-events.pl b/scripts/cleanup-trace-events.pl
index d4f0e4cab5..6c3767bd6a 100755
--- a/scripts/cleanup-trace-events.pl
+++ b/scripts/cleanup-trace-events.pl
@@ -31,10 +31,12 @@ open(IN, $in) or die "open $in: $!";
chdir($dir) or die "chdir $dir: $!";
while () {
-if (/^(disable |(tcg) |vcpu )*([a-z_0-9]+)\(/i) {
-my $pat = "trace_$3";
-$pat .= '_tcg' if (defined $2);
-open GREP, '-|', 'git', 'grep', '-lw', '--max-depth', '1', $pat
+if (/^(disable |(tcg) |(vcpu) )*([a-z_0-9]+)\(/i) {
+my $pat = "trace_$4";
+$pat .= '_tcg' if defined $2;
+open GREP, '-|', 'git', 'grep', '-lw',
+ defined $3 ? () : ('--max-depth', '1'),
+ $pat
or die "run git grep: $!";
while (my $fname = ) {
chomp $fname;
--
2.26.2
[PULL 5/9] softmmu: Add missing trace-events file
From: Philippe Mathieu-Daudé
Commit c7f419f584 moved softmmu-only files out of the root
directory, but forgot to move the trace events, which should
no longer be generated to "trace-root.h". Fix that by adding
softmmu/trace-events.
Signed-off-by: Philippe Mathieu-Daudé
Tested-by: Claudio Fontana
Reviewed-by: Claudio Fontana
Message-id: 20200805130221.24487-1-phi...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
Makefile.objs| 1 +
softmmu/balloon.c| 2 +-
softmmu/ioport.c | 2 +-
softmmu/memory.c | 2 +-
softmmu/vl.c | 2 +-
softmmu/trace-events | 28
trace-events | 27 ---
7 files changed, 33 insertions(+), 31 deletions(-)
create mode 100644 softmmu/trace-events
diff --git a/Makefile.objs b/Makefile.objs
index 982f15ba30..8f593b8f4a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -190,6 +190,7 @@ trace-events-subdirs += hw/gpio
trace-events-subdirs += hw/riscv
trace-events-subdirs += migration
trace-events-subdirs += net
+trace-events-subdirs += softmmu
trace-events-subdirs += ui
endif
trace-events-subdirs += hw/core
diff --git a/softmmu/balloon.c b/softmmu/balloon.c
index 354408c6ea..23452295cd 100644
--- a/softmmu/balloon.c
+++ b/softmmu/balloon.c
@@ -28,10 +28,10 @@
#include "qemu/atomic.h"
#include "sysemu/kvm.h"
#include "sysemu/balloon.h"
-#include "trace-root.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-misc.h"
#include "qapi/qmp/qerror.h"
+#include "trace.h"
static QEMUBalloonEvent *balloon_event_fn;
static QEMUBalloonStatus *balloon_stat_fn;
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index 04e360e79a..cb8adb0b93 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -28,9 +28,9 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "exec/ioport.h"
-#include "trace-root.h"
#include "exec/memory.h"
#include "exec/address-spaces.h"
+#include "trace.h"
typedef struct MemoryRegionPortioList {
MemoryRegion mr;
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..d030eb6f7c 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -24,7 +24,7 @@
#include "qemu/main-loop.h"
#include "qemu/qemu-print.h"
#include "qom/object.h"
-#include "trace-root.h"
+#include "trace.h"
#include "exec/memory-internal.h"
#include "exec/ram_addr.h"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4eb9d1f7fd..f7b103467c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -90,7 +90,7 @@
#include "disas/disas.h"
-#include "trace-root.h"
+#include "trace.h"
#include "trace/control.h"
#include "qemu/plugin.h"
#include "qemu/queue.h"
diff --git a/softmmu/trace-events b/softmmu/trace-events
new file mode 100644
index 00..b80ca042e1
--- /dev/null
+++ b/softmmu/trace-events
@@ -0,0 +1,28 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# balloon.c
+# Since requests are raised via monitor, not many tracepoints are needed.
+balloon_event(void *opaque, unsigned long addr) "opaque %p addr %lu"
+
+# ioport.c
+cpu_in(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value %u"
+cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value
%u"
+
+# memory.c
+memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value,
unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
+memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
+memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr,
uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64"
size %u"
+flatview_new(void *view, void *root) "%p (root %p)"
+flatview_destroy(void *view, void *root) "%p (root %p)"
+flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
+
+# vl.c
+vm_state_notify(int running, int reason, const char *reason_str) "running %d
reason %d (%s)"
+load_file(const char *name, const char *path) "name %s location %s"
+runstate_set(int current_state, const char *current_state_str, int new_state,
const char *new_state_str) "current_run_state %d (%s) new_state %d (%s)"
+system_wakeup_request(int reason) "reason=%d"
+qemu_system_shutdown_request(int reason) "reason=%d"
+qemu_system_powerdown_request(void) ""
diff --git a/trace-events b/trace-events
index 42107ebc69..5882c2d5fc 100644
--- a/trace-events
+++ b/trace-events
@@ -25,22 +25,6 @@
#
# The should be a sprintf()-compatible format string.
-#
[PULL 3/9] build: Don't make object files for dtrace on macOS
From: Roman Bolshakov dtrace on macOS uses unresolved symbols with a special prefix to define probes [1], only headers should be generated for USDT (dtrace(1)). But it doesn't support backwards compatible no-op -G flag [2] and implicit build rules fail. 1. https://markmail.org/message/6grq2ygr5nwdwsnb 2. https://markmail.org/message/5xrxt2w5m42nojkz Reviewed-by: Daniel P. Berrangé Reviewed-by: Stefan Hajnoczi Signed-off-by: Roman Bolshakov Message-id: 20200717093517.73397-4-r.bolsha...@yadro.com Cc: Cameron Esfahani Signed-off-by: Roman Bolshakov Signed-off-by: Stefan Hajnoczi --- Makefile.objs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile.objs b/Makefile.objs index d22b3b45d7..982f15ba30 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -211,5 +211,7 @@ trace-events-files = $(SRC_PATH)/trace-events $(trace-events-subdirs:%=$(SRC_PAT trace-obj-y = trace-root.o trace-obj-y += $(trace-events-subdirs:%=%/trace.o) trace-obj-$(CONFIG_TRACE_UST) += trace-ust-all.o +ifneq ($(CONFIG_DARWIN),y) trace-obj-$(CONFIG_TRACE_DTRACE) += trace-dtrace-root.o trace-obj-$(CONFIG_TRACE_DTRACE) += $(trace-events-subdirs:%=%/trace-dtrace.o) +endif -- 2.26.2
[PULL 1/9] scripts/tracetool: Fix dtrace generation for macOS
From: Roman Bolshakov
dtrace USDT is fully supported since OS X 10.6. There are a few
peculiarities compared to other dtrace flavors.
1. It doesn't accept empty files.
2. It doesn't recognize bool type but accepts C99 _Bool.
3. It converts int8_t * in probe points to char * in
header files and introduces [-Wpointer-sign] warning.
Cc: Cameron Esfahani
Signed-off-by: Roman Bolshakov
Reviewed-by: Stefan Hajnoczi
Message-id: 20200717093517.73397-2-r.bolsha...@yadro.com
Signed-off-by: Stefan Hajnoczi
---
scripts/tracetool/format/d.py | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
index 0afb5f3f47..353722f89c 100644
--- a/scripts/tracetool/format/d.py
+++ b/scripts/tracetool/format/d.py
@@ -13,6 +13,7 @@ __email__ = "stefa...@redhat.com"
from tracetool import out
+from sys import platform
# Reserved keywords from
@@ -34,7 +35,8 @@ def generate(events, backend, group):
# SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
# with an empty file. Avoid the warning.
-if not events:
+# But dtrace on macOS can't deal with empty files.
+if not events and platform != "darwin":
return
out('/* This file is autogenerated by tracetool, do not edit. */'
@@ -44,6 +46,17 @@ def generate(events, backend, group):
for e in events:
args = []
for type_, name in e.args:
+if platform == "darwin":
+# macOS dtrace accepts only C99 _Bool
+if type_ == 'bool':
+type_ = '_Bool'
+if type_ == 'bool *':
+type_ = '_Bool *'
+# It converts int8_t * in probe points to char * in header
+# files and introduces [-Wpointer-sign] warning.
+# Avoid it by changing probe type to signed char * beforehand.
+if type_ == 'int8_t *':
+type_ = 'signed char *'
if name in RESERVED_WORDS:
name += '_'
args.append(type_ + ' ' + name)
--
2.26.2
[PULL 4/9] net/colo: Match is-enabled probe to tracepoint
From: Roman Bolshakov
Build of QEMU with dtrace fails on macOS:
LINKx86_64-softmmu/qemu-system-x86_64
error: probe colo_compare_miscompare doesn't exist
error: Could not register probes
ld: error creating dtrace DOF section for architecture x86_64
The reason of the error is explained by Adam Leventhal [1]:
Note that is-enabled probes don't have the stability magic so I'm not
sure how things would work if only is-enabled probes were used.
net/colo code uses is-enabled probes to determine if other probes should
be used but colo_compare_miscompare itself is not used explicitly.
Linker doesn't include the symbol and build fails.
The issue can be resolved if is-enabled probe matches the actual trace
point that is used inside the test. Packet dump toggle is replaced with
a compile-time conditional definition.
1. http://markmail.org/message/6grq2ygr5nwdwsnb
Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
Cc: Philippe Mathieu-Daudé
Cc: Cameron Esfahani
Signed-off-by: Roman Bolshakov
Reviewed-by: Zhang Chen
Message-id: 20200717093517.73397-5-r.bolsha...@yadro.com
Signed-off-by: Stefan Hajnoczi
---
net/colo-compare.c| 42 ++
net/filter-rewriter.c | 10 --
net/trace-events | 2 --
3 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 2c20de1537..0b3215dee0 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
#define REGULAR_PACKET_CHECK_MS 3000
#define DEFAULT_TIME_OUT_MS 3000
+/* #define DEBUG_COLO_PACKETS */
+
static QemuMutex colo_compare_mutex;
static bool colo_compare_active;
static QemuMutex event_mtx;
@@ -328,7 +330,7 @@ static int colo_compare_packet_payload(Packet *ppkt,
uint16_t len)
{
-if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
@@ -493,12 +495,12 @@ sec:
g_queue_push_head(>primary_list, ppkt);
g_queue_push_head(>secondary_list, spkt);
-if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-qemu_hexdump((char *)ppkt->data, stderr,
-"colo-compare ppkt", ppkt->size);
-qemu_hexdump((char *)spkt->data, stderr,
-"colo-compare spkt", spkt->size);
-}
+#ifdef DEBUG_COLO_PACKETS
+qemu_hexdump((char *)ppkt->data, stderr,
+ "colo-compare ppkt", ppkt->size);
+qemu_hexdump((char *)spkt->data, stderr,
+ "colo-compare spkt", spkt->size);
+#endif
colo_compare_inconsistency_notify(s);
}
@@ -534,12 +536,12 @@ static int colo_packet_compare_udp(Packet *spkt, Packet
*ppkt)
ppkt->size - offset)) {
trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
-if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
- ppkt->size);
-qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
- spkt->size);
-}
+#ifdef DEBUG_COLO_PACKETS
+qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+#endif
return -1;
} else {
return 0;
@@ -577,12 +579,12 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet
*ppkt)
ppkt->size);
trace_colo_compare_icmp_miscompare("Secondary pkt size",
spkt->size);
-if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
-qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
- ppkt->size);
-qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
- spkt->size);
-}
+#ifdef DEBUG_COLO_PACKETS
+qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+#endif
return -1;
} else {
return 0;
@@ -598,7 +600,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet
*ppkt)
uint16_t offset = ppkt->vnet_hdr_len;
trace_colo_compare_main("compare other");
-if
[PULL 8/9] trace-events: Delete unused trace points
From: Markus Armbruster Tracked down with the help of scripts/cleanup-trace-events.pl. Signed-off-by: Markus Armbruster Message-id: 20200806141334.3646302-4-arm...@redhat.com Signed-off-by: Stefan Hajnoczi --- audio/trace-events | 3 --- block/trace-events | 3 --- hw/misc/trace-events| 1 - hw/ppc/trace-events | 4 hw/timer/trace-events | 1 - migration/trace-events | 1 - target/ppc/trace-events | 1 - 7 files changed, 14 deletions(-) diff --git a/audio/trace-events b/audio/trace-events index a1d1eccb8a..6aec535763 100644 --- a/audio/trace-events +++ b/audio/trace-events @@ -9,12 +9,9 @@ alsa_read_zero(long len) "Failed to read %ld frames (read zero)" alsa_xrun_out(void) "Recovering from playback xrun" alsa_xrun_in(void) "Recovering from capture xrun" alsa_resume_out(void) "Resuming suspended output stream" -alsa_resume_in(void) "Resuming suspended input stream" -alsa_no_frames(int state) "No frames available and ALSA state is %d" # ossaudio.c oss_version(int version) "OSS version = 0x%x" -oss_invalid_available_size(int size, int bufsize) "Invalid available size, size=%d bufsize=%d" # audio.c audio_timer_start(int interval) "interval %d ms" diff --git a/block/trace-events b/block/trace-events index 9158335061..778140d304 100644 --- a/block/trace-events +++ b/block/trace-events @@ -42,7 +42,6 @@ backup_do_cow_enter(void *job, int64_t start, int64_t offset, uint64_t bytes) "j backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret) "job %p offset %" PRId64 " bytes %" PRIu64 " ret %d" # block-copy.c -block_copy_skip(void *bcs, int64_t start) "bcs %p start %"PRId64 block_copy_skip_range(void *bcs, int64_t start, uint64_t bytes) "bcs %p start %"PRId64" bytes %"PRId64 block_copy_process(void *bcs, int64_t start) "bcs %p start %"PRId64 block_copy_copy_range_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d" @@ -200,8 +199,6 @@ curl_setup_preadv(uint64_t bytes, uint64_t start, const char *range) "reading %" curl_close(void) "close" # file-posix.c -file_xfs_write_zeroes(const char *error) "cannot write zero range (%s)" -file_xfs_discard(const char *error) "cannot punch hole (%s)" file_FindEjectableOpticalMedia(const char *media) "Matching using %s" file_setup_cdrom(const char *partition) "Using %s as optical disc" file_hdev_is_sg(int type, int version) "SG device found: type=%d, version=%d" diff --git a/hw/misc/trace-events b/hw/misc/trace-events index 066752aa90..f9a0ba1a93 100644 --- a/hw/misc/trace-events +++ b/hw/misc/trace-events @@ -44,7 +44,6 @@ ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = 0 ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= 0x%02x" # empty_slot.c -empty_slot_read(uint64_t addr, unsigned width, uint64_t value, unsigned size, const char *name) "rd addr:0x%04"PRIx64" data:0x%0*"PRIx64" size %u [%s]" empty_slot_write(uint64_t addr, unsigned width, uint64_t value, unsigned size, const char *name) "wr addr:0x%04"PRIx64" data:0x%0*"PRIx64" size %u [%s]" # slavio_misc.c diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events index 7c0be4102e..ca34244e2a 100644 --- a/hw/ppc/trace-events +++ b/hw/ppc/trace-events @@ -10,7 +10,6 @@ spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u" spapr_pci_msi_retry(unsigned config_addr, unsigned req_num, unsigned max_irqs) "Guest device at 0x%x asked %u, have only %u" # spapr.c -spapr_cas_failed(unsigned long n) "DT diff buffer is too small: %ld bytes" spapr_cas_continue(unsigned long n) "Copy changes to the guest: %ld bytes" # spapr_hcall.c @@ -77,9 +76,6 @@ spapr_vio_free_crq(uint32_t reg) "CRQ for dev 0x%" PRIx32 " freed" # ppc.c ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t seconds) "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64" (%"PRId64"s)" -# prep.c -prep_io_800_writeb(uint32_t addr, uint32_t val) "0x%08" PRIx32 " => 0x%02" PRIx32 -prep_io_800_readb(uint32_t addr, uint32_t retval) "0x%08" PRIx32 " <= 0x%02" PRIx32 # prep_systemio.c prep_systemio_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x" diff --git a/hw/timer/trace-events b/hw/timer/trace-events index 447b7c405b..1537c3e6ec 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -81,7 +81,6 @@ avr_timer16_read(uint8_t addr, uint8_t value) "timer16 read addr:%u value:%u" avr_timer16_read_ifr(uint8_t value) "timer16 read addr:ifr value:%u" avr_timer16_read_imsk(uint8_t value) "timer16 read addr:imsk value:%u" avr_timer16_write(uint8_t addr, uint8_t value) "timer16 write addr:%u value:%u" -avr_timer16_write_ifr(uint8_t value) "timer16 write addr:ifr value:%u" avr_timer16_write_imsk(uint8_t value) "timer16 write addr:imsk value:%u" avr_timer16_interrupt_count(uint8_t cnt) "count: %u" avr_timer16_interrupt_overflow(const char *reason) "overflow: %s" diff --git a/migration/trace-events b/migration/trace-events index
[PULL 2/9] scripts/tracetool: Use void pointer for vcpu
From: Roman Bolshakov
dtrace on macOS complains that CPUState * is used for a few probes:
dtrace: failed to compile script trace-dtrace-root.dtrace: line 130: syntax
error near "CPUState"
A comment in scripts/tracetool/__init__.py mentions that:
We only want to allow standard C types or fixed sized
integer types. We don't want QEMU specific types
as we can't assume trace backends can resolve all the
typedefs
Fixes: 3d211d9f4dbee ("trace: Add 'vcpu' event property to trace guest vCPU")
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Roman Bolshakov
Message-id: 20200717093517.73397-3-r.bolsha...@yadro.com
Cc: Cameron Esfahani
Signed-off-by: Roman Bolshakov
Signed-off-by: Stefan Hajnoczi
---
scripts/tracetool/vcpu.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/tracetool/vcpu.py b/scripts/tracetool/vcpu.py
index b54e4f4e7a..868b4cb04c 100644
--- a/scripts/tracetool/vcpu.py
+++ b/scripts/tracetool/vcpu.py
@@ -24,7 +24,7 @@ def transform_event(event):
assert "tcg-trans" not in event.properties
assert "tcg-exec" not in event.properties
-event.args = Arguments([("CPUState *", "__cpu"), event.args])
+event.args = Arguments([("void *", "__cpu"), event.args])
if "tcg" in event.properties:
fmt = "\"cpu=%p \""
event.fmt = [fmt + event.fmt[0],
--
2.26.2
[PULL 0/9] Tracing patches
The following changes since commit d0ed6a69d399ae193959225cdeaa9382746c91cc: Update version for v5.1.0 release (2020-08-11 17:07:03 +0100) are available in the Git repository at: https://github.com/stefanha/qemu.git tags/tracing-pull-request for you to fetch changes up to bd6c9e56aba2e1b9a307642c72375386afbcf1f2: trace-events: Fix attribution of trace points to source (2020-08-12 20:28:5= 4 +0100) Pull request macOS dtrace support and cleanups. Markus Armbruster (4): scripts/cleanup-trace-events: Fix for vcpu property scripts/cleanup-trace-events: Emit files in alphabetical order trace-events: Delete unused trace points trace-events: Fix attribution of trace points to source Philippe Mathieu-Daud=C3=A9 (1): softmmu: Add missing trace-events file Roman Bolshakov (4): scripts/tracetool: Fix dtrace generation for macOS scripts/tracetool: Use void pointer for vcpu build: Don't make object files for dtrace on macOS net/colo: Match is-enabled probe to tracepoint Makefile.objs | 3 +++ net/colo-compare.c | 42 + net/filter-rewriter.c | 10 ++-- softmmu/balloon.c | 2 +- softmmu/ioport.c| 2 +- softmmu/memory.c| 2 +- softmmu/vl.c| 2 +- audio/trace-events | 3 --- block/trace-events | 8 ++- hw/block/trace-events | 2 +- hw/char/trace-events| 2 +- hw/display/trace-events | 4 +++- hw/hyperv/trace-events | 2 +- hw/mips/trace-events| 2 +- hw/misc/trace-events| 9 +++ hw/ppc/trace-events | 10 ++-- hw/riscv/trace-events | 2 +- hw/rtc/trace-events | 2 +- hw/timer/trace-events | 1 - hw/tpm/trace-events | 2 +- hw/usb/trace-events | 4 +++- hw/vfio/trace-events| 10 hw/virtio/trace-events | 2 +- migration/trace-events | 37 +++-- net/trace-events| 2 -- scripts/cleanup-trace-events.pl | 23 +++--- scripts/tracetool/format/d.py | 15 +++- scripts/tracetool/vcpu.py | 2 +- softmmu/trace-events| 28 ++ target/ppc/trace-events | 1 - target/riscv/trace-events | 2 +- trace-events| 32 +++-- ui/trace-events | 6 ++--- util/trace-events | 4 +++- 34 files changed, 152 insertions(+), 128 deletions(-) create mode 100644 softmmu/trace-events --=20 2.26.2
