Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working
Hello Michael, Such changes are long overdue. However given the complexity of commands and arguments, maybe it'd be a good idea to write a code generator for the command line interface, This way you could also generate --help outputs, manpages, shell completions just from the command line spec we'd use to generate the argv parsing routines. On Wed, 7 Feb 2024 at 19:58, Michael Tokarev wrote: > > This is an incomplete first attempt only, there's a lot left to do. > > All the options in qemu-img is a complete mess, - no, inconsistent or > incomplete syntax in documentation, many undocumented options, option > names are used inconsistently and differently for different commands, > no long options exists for many short options, --help output is a huge > mess by its own, and the way how it all is generated is another story. > docs/tools/qemu-img.rst with qemu-img-opts.hx is yet another. > > I hoped to fix just an option or two, but it ended up in a large task, > and I need some help and discussion, hence the RFC. > > The idea is: > > - create more or less consistent set of options between different >subcommands > - provide long options which can be used without figuring out which >-T/-t, -f|-F|-O etc to use for which of the two images given > - have qemu-img --help provide just a list of subcommands > - have qemu-img COMMAND --help to describe just this subcommand > - get rid of qemu-img-opts.hx, instead finish documentation in >qemu-img.rst based on the actual options implemented in >qemu-img.c. > > I started converting subcommands one by one, providing long options > and --help output. And immediately faced some questions which needs > wider discussion. > > o We have --image-opts and --target-image-opts. Do we really need both? >In my view, --image-opts should be sort of global, turning *all* >filenames on this command line into complete image specifications, >there's no need to have separate image-opts and --target-image-opts. >Don't know what to do wrt compatibility though. It shouldn't be made >this way from the beginning. As a possible solution, introduce a new >option and deprecate current set. > > o For conversion (convert, dd, etc), we've source and destination, >so it's easy to distinguish using long options, like --source-format >--target-cache etc (-t/-T -f/-F is a mess here already). What to >do with compare? --format1 --format2 is ugly, maybe --a-format and >--b-format? Maybe we can get off with --source (a) and --target (b) >instead of filename1 & filename2? >(--cache in this context is global for both). > > o qemu-img convert. It's the most messy one, and it is not really >documented (nor in qemu-img.rst , eg there's nothing in there about >FILENAME2, -B is difficult to understand, etc). >At the very least, I'd say the options should be > --source-format, --source-cache etc > --target-format, --target-options > --target-image-opts - this shold go IMHO > > o check and commit - inconsistent cache flags? >In convert, cache is backwards (source/target)? > > At first, I tried to have more or less common option descriptions, > using various parameters, variables or #defines, but in different > commands the same options has slightly different wording, and in > some option names are different, so it looks like it's best to > have complete text in each subcommand. > > > Michael Tokarev (8): > qemu-img: pass current cmdname into command handlers > qemu-img: refresh options/--help for "create" subcommand > qemu-img: factor out parse_output_format() and use it in the code >(this one has been sent in a separate patch, here it is just for > completness) > qemu-img: refresh options/--help for "check" command > qemu-img: simplify --repair error message > qemu-img: refresh options/--help for "commit" command > qemu-img: refresh options/--help for "compare" command > qemu-img: refresh options/--help for "convert" command > > qemu-img.c | 352 ++--- > 1 file changed, 226 insertions(+), 126 deletions(-) > > -- > 2.39.2 > > -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation
IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
- AioContext **vq_aio_context, uint16_t num_queues)
+/**
+ * apply_iothread_vq_mapping:
+ * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
+ * @vq_aio_context: The array of AioContext pointers to fill in.
+ * @num_queues: The length of @vq_aio_context.
+ * @errp: If an error occurs, a pointer to the area to store the error.
+ *
+ * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
+ * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
+ *
+ * Returns: %true on success, %false on failure.
+ **/
+static bool apply_iothread_vq_mapping(
+IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
+AioContext **vq_aio_context,
+uint16_t num_queues,
+Error **errp)
{
IOThreadVirtQueueMappingList *node;
size_t num_iothreads = 0;
size_t cur_iothread = 0;
+if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
+ num_queues, errp)) {
+return false;
+}
+
for (node = iothread_vq_mapping_list; node; node = node->next) {
num_iothreads++;
}
@@ -1638,6 +1656,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList
*iothread_vq_mapping_list,
/* Explicit vq:IOThread assignment */
for (vq = node->value->vqs; vq; vq = vq->next) {
+assert(vq->value < num_queues);
vq_aio_context[vq->value] = ctx;
}
} else {
@@ -1650,6 +1669,8 @@ apply_vq_mapping(IOThreadVirtQueueMappingList
*iothread_vq_mapping_list,
cur_iothread++;
}
+
+return true;
}
/* Context: BQL held */
@@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock
*s, Error **errp)
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+if (conf->iothread && conf->iothread_vq_mapping_list) {
+if (conf->iothread) {
+error_setg(errp, "iothread and iothread-vq-mapping properties "
+ "cannot be set at the same time");
+return false;
+}
+}
+
if (conf->iothread || conf->iothread_vq_mapping_list) {
if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
error_setg(errp,
@@ -1685,8 +1714,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock
*s, Error **errp)
s->vq_aio_context = g_new(AioContext *, conf->num_queues);
if (conf->iothread_vq_mapping_list) {
-apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
- conf->num_queues);
+if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
+ s->vq_aio_context,
+ conf->num_queues,
+ errp)) {
+g_free(s->vq_aio_context);
+s->vq_aio_context = NULL;
+return false;
+}
} else if (conf->iothread) {
AioContext *ctx = iothread_get_aio_context(conf->iothread);
for (unsigned i = 0; i < conf->num_queues; i++) {
@@ -1996,19 +2031,6 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
-if (conf->iothread_vq_mapping_list) {
-if (conf->iothread) {
-error_setg(errp, "iothread and iothread-vq-mapping properties "
- "cannot be set at the same time");
-return;
-}
-
-if (!validate_iothread_vq_mapping_list(conf->iothread_vq_mapping_list,
- conf->num_queues, errp)) {
-return;
-}
-}
-
s->config_size = virtio_get_config_size(_blk_cfg_size_params,
s->host_features);
virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
--
2.43.0
virtio_block_ops and methods are moved around without changes in the
diff, is that on purpose? If no the patch and history would be less
noisy.
Regardless:
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
The aio_co_reschedule_self() API is designed to avoid the race
condition between scheduling the coroutine in another AioContext and
yielding.
The QMP dispatch code uses the open-coded version that appears
susceptible to the race condition at first glance:
aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
qemu_coroutine_yield();
The code is actually safe because the iohandler and qemu_aio_context
AioContext run under the Big QEMU Lock. Nevertheless, set a good example
and use aio_co_reschedule_self() so it's obvious that there is no race.
Suggested-by: Hanna Reitz
Signed-off-by: Stefan Hajnoczi
---
qapi/qmp-dispatch.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 176b549473..f3488afeef 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -212,8 +212,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* executing the command handler so that it can make progress if it
* involves an AIO_WAIT_WHILE().
*/
-aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
-qemu_coroutine_yield();
+aio_co_reschedule_self(qemu_get_aio_context());
}
monitor_set_cur(qemu_coroutine_self(), cur_mon);
@@ -227,9 +226,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* Move back to iohandler_ctx so that nested event loops for
* qemu_aio_context don't start new monitor commands.
*/
-aio_co_schedule(iohandler_get_aio_context(),
-qemu_coroutine_self());
-qemu_coroutine_yield();
+aio_co_reschedule_self(iohandler_get_aio_context());
}
} else {
/*
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
It is not possible to instantiate a virtio-blk device with 0 virtqueues.
The following check is located in ->realize():
if (!conf->num_queues) {
error_setg(errp, "num-queues property must be larger than 0");
return;
}
Later on we access s->vq_aio_context[0] under the assumption that there
is as least one virtqueue. Hanna Czenczek noted that
it would help to show that the array index is already valid.
Add an assertion to document that s->vq_aio_context[0] is always
safe...and catch future code changes that break this assumption.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
hw/block/virtio-blk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e8b37fd5f4..a0735a9bca 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
* Try to change the AioContext so that block jobs and other operations can
* co-locate their activity in the same AioContext. If it fails, nevermind.
*/
+assert(nvqs > 0); /* enforced during ->realize() */
r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
_err);
if (r < 0) {
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
Hanna Czenczek noted that the array index in
virtio_blk_dma_restart_cb() is not bounds-checked:
g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
...
while (rq) {
VirtIOBlockReq *next = rq->next;
uint16_t idx = virtio_get_queue_index(rq->vq);
rq->next = vq_rq[idx];
^^
The code is correct because both rq->vq and vq_rq[] depend on
num_queues, but this is indirect and not 100% obvious. Add an assertion.
This sentence could be useful as an inline comment too.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
hw/block/virtio-blk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a0735a9bca..f3193f4b75 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1209,6 +1209,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool
running,
VirtIOBlockReq *next = rq->next;
uint16_t idx = virtio_get_queue_index(rq->vq);
+assert(idx < num_queues);
rq->next = vq_rq[idx];
vq_rq[idx] = rq;
rq = next;
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 4/5] virtio-blk: declare VirtIOBlock::rq with a type
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
The VirtIOBlock::rq field has had the type void * since its introduction
in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
Natapov)").
Perhaps this was done to avoid the forward declaration of
VirtIOBlockReq.
Hanna Czenczek pointed out the missing type. Specify
the actual type because there is no need to use void * here.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/virtio-blk.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 833a9a344f..5c14110c4b 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -55,7 +55,7 @@ struct VirtIOBlock {
VirtIODevice parent_obj;
BlockBackend *blk;
QemuMutex rq_lock;
-void *rq; /* protected by rq_lock */
+struct VirtIOBlockReq *rq; /* protected by rq_lock */
VirtIOBlkConf conf;
unsigned short sector_mask;
bool original_wce;
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH] hw/hyperv: Include missing headers
On Mon, 29 Jan 2024 19:00, Philippe Mathieu-Daudé wrote:
Include missing headers in order to avoid when refactoring
unrelated headers:
hw/hyperv/hyperv.c:33:18: error: field ‘msg_page_mr’ has incomplete type
33 | MemoryRegion msg_page_mr;
| ^~~
hw/hyperv/hyperv.c: In function ‘synic_update’:
hw/hyperv/hyperv.c:64:13: error: implicit declaration of function
‘memory_region_del_subregion’ [-Werror=implicit-function-declaration]
64 | memory_region_del_subregion(get_system_memory(),
| ^~~
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_signal_event’:
hw/hyperv/hyperv.c:683:17: error: implicit declaration of function ‘ldq_phys’;
did you mean ‘ldub_phys’? [-Werror=implicit-function-declaration]
683 | param = ldq_phys(_space_memory, addr);
| ^~~~
| ldub_phys
hw/hyperv/hyperv.c:683:17: error: nested extern declaration of ‘ldq_phys’
[-Werror=nested-externs]
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_retreive_dbg_data’:
hw/hyperv/hyperv.c:792:24: error: ‘TARGET_PAGE_SIZE’ undeclared (first use in
this function); did you mean ‘TARGET_PAGE_BITS’?
792 | msg.u.recv.count = TARGET_PAGE_SIZE - sizeof(*debug_data_out);
|^~~~
|TARGET_PAGE_BITS
hw/hyperv/hyperv.c: In function ‘hyperv_syndbg_send’:
hw/hyperv/hyperv.c:885:16: error: ‘HV_SYNDBG_STATUS_INVALID’ undeclared (first
use in this function)
885 | return HV_SYNDBG_STATUS_INVALID;
|^~~~
Signed-off-by: Philippe Mathieu-Daudé
---
BTW who maintains this code?
$ ./scripts/get_maintainer.pl -f hw/hyperv/hyperv.c
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches! Use common sense.
---
hw/hyperv/hyperv.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 57b402b956..6c4a18dd0e 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -12,6 +12,7 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "exec/memory.h"
#include "sysemu/kvm.h"
#include "qemu/bitops.h"
#include "qemu/error-report.h"
@@ -21,6 +22,9 @@
#include "qemu/rcu_queue.h"
#include "hw/hyperv/hyperv.h"
#include "qom/object.h"
+#include "target/i386/kvm/hyperv-proto.h"
+#include "target/i386/cpu.h"
+#include "exec/cpu-all.h"
struct SynICState {
DeviceState parent_obj;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
[PATCH v3 1/2] hw/core/qdev.c: add qdev_get_human_name()
Add a simple method to return some kind of human readable identifier for
use in error messages.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Manos Pitsidianakis
---
include/hw/qdev-core.h | 14 ++
hw/core/qdev.c | 8
2 files changed, 22 insertions(+)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..66338f479f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,20 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..c68d0f7c51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,14 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+g_assert(dev != NULL);
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
--
γαῖα πυρί μιχθήτω
[PATCH v3 0/2] hw/block/block.c: improve confusing error
In cases where a device tries to read more bytes than the block device contains with the blk_check_size_and_read_all() function, the error is vague: "device requires X bytes, block backend provides Y bytes". This patch changes the errors of this function to include the block backend name, the device id and device type name where appropriate. Version 3: - Changed phrasing "%s device with id='%s'" to "%s device '%s'" since second parameter might be either device id or device path. (thanks Stefan Hajnoczi ) Version 2: - Assert dev is not NULL on qdev_get_human_name (thanks Phil Mathieu-Daudé ) Manos Pitsidianakis (2): hw/core/qdev.c: add qdev_get_human_name() hw/block/block.c: improve confusing blk_check_size_and_read_all() error include/hw/block/block.h | 4 ++-- include/hw/qdev-core.h | 14 ++ hw/block/block.c | 25 +++-- hw/block/m25p80.c| 3 ++- hw/block/pflash_cfi01.c | 4 ++-- hw/block/pflash_cfi02.c | 2 +- hw/core/qdev.c | 8 7 files changed, 44 insertions(+), 16 deletions(-) Range-diff against v2: 1: 5fb5879708 ! 1: 8b566bfced hw/core/qdev.c: add qdev_get_human_name() @@ Commit message Add a simple method to return some kind of human readable identifier for use in error messages. +Reviewed-by: Stefan Hajnoczi Signed-off-by: Manos Pitsidianakis ## include/hw/qdev-core.h ## 2: 8e7eb17fbd ! 2: 7260eadff2 hw/block/block.c: improve confusing blk_check_size_and_read_all() error @@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, - "block backend provides %" PRIu64 " bytes", - size, blk_len); +dev_id = qdev_get_human_name(dev); -+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu ++error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu + " bytes, %s block backend provides %" PRIu64 " bytes", + object_get_typename(OBJECT(dev)), dev_id, size, + blk_name(blk), blk_len); @@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, -error_setg_errno(errp, -ret, "can't read block backend"); +dev_id = qdev_get_human_name(dev); +error_setg_errno(errp, -ret, "can't read %s block backend" -+ "for %s device with id='%s'", ++ " for %s device '%s'", + blk_name(blk), object_get_typename(OBJECT(dev)), + dev_id); return false; base-commit: 11be70677c70fdccd452a3233653949b79e97908 -- γαῖα πυρί μιχθήτω
[PATCH v3 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
---
include/hw/block/block.h | 4 ++--
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 15fff66435..de3946a5f1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -88,8 +88,8 @@ static inline unsigned int get_physical_block_exp(BlockConf
*conf)
/* Backend access helpers */
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp);
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp);
/* Configuration helpers */
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ec4a675490 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
* BDRV_REQUEST_MAX_BYTES.
* On success, return true.
* On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
* This function not intended for actual block devices, which read on
* demand. It's for things like memory devices that (ab)use a block
* backend to provide persistence.
*/
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp)
{
int64_t blk_len;
int ret;
+g_autofree char *dev_id = NULL;
blk_len = blk_getlength(blk);
if (blk_len < 0) {
error_setg_errno(errp, -blk_len,
- "can't get size of block backend");
+ "can't get size of %s block backend", blk_name(blk));
return false;
}
if (blk_len != size) {
-error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
- "block backend provides %" PRIu64 " bytes",
- size, blk_len);
+dev_id = qdev_get_human_name(dev);
+error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
+ " bytes, %s block backend provides %" PRIu64 " bytes",
+ object_get_typename(OBJECT(dev)), dev_id, size,
+ blk_name(blk), blk_len);
return false;
}
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
*buf, hwaddr size,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
-error_setg_errno(errp, -ret, "can't read block backend");
+dev_id = qdev_get_human_name(dev);
+error_setg_errno(errp, -ret, "can't read %s block backend"
+ " for %s device '%s'",
+ blk_name(blk), object_get_typename(OBJECT(dev)),
+ dev_id);
return false;
}
return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+ s->storage, s->size, errp)) {
return;
}
} else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
- errp)) {
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+ total_len, errp)) {
vmstate_unregister_ram(>mem, DEVICE(pfl));
return;
[PATCH v3 2/6] hw/arm/z2: convert DPRINTF to trace events and guest errors
Tracing DPRINTFs to stderr might not be desired. A developer that relies
on trace events should be able to opt-in to each trace event and rely on
QEMU's log redirection, instead of stderr by default.
This commit converts DPRINTFs in this file that are used for tracing
into trace events. DPRINTFs that report guest errors are logged with
LOG_GUEST_ERROR.
Signed-off-by: Manos Pitsidianakis
---
hw/arm/trace-events | 7 +++
hw/arm/z2.c | 27 ---
2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 7c56943215..0ff41e6c78 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -58,3 +58,10 @@ smmuv3_inv_notifiers_iova(const char *name, uint16_t asid,
uint16_t vmid, uint64
# strongarm.c
strongarm_uart_update_parameters(const char *label, int speed, char parity,
int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
strongarm_ssp_read_underrun(void) "SSP rx underrun"
+
+# z2.c
+z2_lcd_reg_update(uint8_t cur, uint8_t i_0, uint8_t i_1, uint8_t i_2, uint32_t
value) "cur_reg = 0x%x, buf = [0x%x, 0x%x, 0x%x], value = 0x%x"
+z2_lcd_enable_disable_result(const char *result) "LCD %s"
+z2_aer915_send_too_long(int8_t msg) "message too long (%i bytes)"
+z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
+z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index a67fba2cfd..eb2ff8dbc8 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -27,13 +27,7 @@
#include "exec/address-spaces.h"
#include "qom/object.h"
#include "qapi/error.h"
-
-#ifdef DEBUG_Z2
-#define DPRINTF(fmt, ...) \
-printf(fmt, ## __VA_ARGS__)
-#else
-#define DPRINTF(fmt, ...)
-#endif
+#include "trace.h"
static const struct keymap map[0x100] = {
[0 ... 0xff] = { -1, -1 },
@@ -119,6 +113,8 @@ static uint32_t zipit_lcd_transfer(SSIPeripheral *dev,
uint32_t value)
{
ZipitLCD *z = ZIPIT_LCD(dev);
uint16_t val;
+
+trace_z2_lcd_reg_update(z->cur_reg, z->buf[0], z->buf[1], z->buf[2],
value);
if (z->selected) {
z->buf[z->pos] = value & 0xff;
z->pos++;
@@ -126,22 +122,19 @@ static uint32_t zipit_lcd_transfer(SSIPeripheral *dev,
uint32_t value)
if (z->pos == 3) {
switch (z->buf[0]) {
case 0x74:
-DPRINTF("%s: reg: 0x%.2x\n", __func__, z->buf[2]);
z->cur_reg = z->buf[2];
break;
case 0x76:
val = z->buf[1] << 8 | z->buf[2];
-DPRINTF("%s: value: 0x%.4x\n", __func__, val);
if (z->cur_reg == 0x22 && val == 0x) {
z->enabled = 1;
-printf("%s: LCD enabled\n", __func__);
+trace_z2_lcd_enable_disable_result("enabled");
} else if (z->cur_reg == 0x10 && val == 0x) {
z->enabled = 0;
-printf("%s: LCD disabled\n", __func__);
+trace_z2_lcd_enable_disable_result("disabled");
}
break;
default:
-DPRINTF("%s: unknown command!\n", __func__);
break;
}
z->pos = 0;
@@ -211,14 +204,12 @@ static int aer915_send(I2CSlave *i2c, uint8_t data)
s->buf[s->len] = data;
if (s->len++ > 2) {
-DPRINTF("%s: message too long (%i bytes)\n",
-__func__, s->len);
+trace_z2_aer915_send_too_long(s->len);
return 1;
}
if (s->len == 2) {
-DPRINTF("%s: reg %d value 0x%02x\n", __func__,
-s->buf[0], s->buf[1]);
+trace_z2_aer915_send(s->buf[0], s->buf[1]);
}
return 0;
@@ -228,14 +219,12 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event
event)
{
AER915State *s = AER915(i2c);
+trace_z2_aer915_event(s->len, event);
switch (event) {
case I2C_START_SEND:
s->len = 0;
break;
case I2C_START_RECV:
-if (s->len != 1) {
-DPRINTF("%s: short message!?\n", __func__);
-}
break;
case I2C_FINISH:
break;
--
γαῖα πυρί μιχθήτω
[PATCH v3 6/6] hw/xen: convert stderr prints to error/warn reports
According to the QEMU Coding Style document:
> Do not use printf(), fprintf() or monitor_printf(). Instead, use
> error_report() or error_vreport() from error-report.h. This ensures the
> error is reported in the right place (current monitor or stderr), and in
> a uniform format.
> Use error_printf() & friends to print additional information.
This commit changes fprintfs that report warnings and errors to the
appropriate report functions.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
---
hw/xen/xen-hvm-common.c | 12 ++--
hw/xen/xen-mapcache.c | 5 ++---
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 05a29c6f11..baa1adb9f2 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -20,8 +20,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
MemoryRegion *mr,
if (runstate_check(RUN_STATE_INMIGRATE)) {
/* RAM already populated in Xen */
-fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
-" bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
+warn_report("%s: do not alloc "RAM_ADDR_FMT
+" bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE",
__func__, size, ram_addr);
return;
}
@@ -552,9 +552,9 @@ static void cpu_handle_ioreq(void *opaque)
req->data = copy.data;
if (req->state != STATE_IOREQ_INPROCESS) {
-fprintf(stderr, "Badness in I/O request ... not in service?!: "
+warn_report("Badness in I/O request ... not in service?!: "
"%x, ptr: %x, port: %"PRIx64", "
-"data: %"PRIx64", count: %u, size: %u, type: %u\n",
+"data: %"PRIx64", count: %u, size: %u, type: %u",
req->state, req->data_is_ptr, req->addr,
req->data, req->count, req->size, req->type);
destroy_hvm_domain(false);
@@ -758,9 +758,9 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
va_list ap;
va_start(ap, fmt);
-vfprintf(stderr, fmt, ap);
+error_vreport(fmt, ap);
va_end(ap);
-fprintf(stderr, "Will destroy the domain.\n");
+error_report("Will destroy the domain.");
/* destroy the domain */
qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
}
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index 336c212376..4f956d048e 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -347,9 +347,8 @@ tryagain:
MapCacheRev *reventry = g_new0(MapCacheRev, 1);
entry->lock++;
if (entry->lock == 0) {
-fprintf(stderr,
-"mapcache entry lock overflow: "HWADDR_FMT_plx" -> %p\n",
-entry->paddr_index, entry->vaddr_base);
+error_report("mapcache entry lock overflow: "HWADDR_FMT_plx" ->
%p",
+ entry->paddr_index, entry->vaddr_base);
abort();
}
reventry->dma = dma;
--
γαῖα πυρί μιχθήτω
[PATCH v3 4/6] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
Tracing DPRINTFs to stderr might not be desired. A developer that relies
on tracepoints should be able to opt-in to each tracepoint and rely on
QEMU's log redirection, instead of stderr by default.
This commit converts DPRINTFs in this file that are used for tracing
into tracepoints.
Signed-off-by: Manos Pitsidianakis
---
hw/xen/trace-events | 11 +
hw/xen/xen-mapcache.c | 54 +++
2 files changed, 35 insertions(+), 30 deletions(-)
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 67a6c41926..a65dc0e55f 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -60,3 +60,14 @@ cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t
reg, uint32_t size, ui
xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
xen_map_cache_return(void* ptr) "%p"
+xen_map_cache_init(uint64_t nr_buckets, uint64_t size) "nr_buckets =
0x%"PRIx64" size 0x%"PRIx64
+xen_replace_cache_entry_dummy(uint64_t old_phys_addr, uint64_t new_phys_addr)
"Replacing a dummy mapcache entry for 0x%"PRIx64" with 0x%"PRIx64
+xen_invalidate_map_cache_entry_unlocked_not_found(void *p) "could not find %p"
+xen_invalidate_map_cache_entry_unlocked_found(uint64_t addr, void *p) "
0x%"PRIx64" -> %p is present"
+xen_invalidate_map_cache_entry_unlocked_miss(void *buffer) "Trying to unmap
address %p that is not in the mapcache"
+xen_replace_cache_entry_unlocked_could_not_update_entry(uint64_t
old_phys_addr) "Unable to update a mapcache entry for 0x%"PRIx64
+xen_ram_addr_from_mapcache_not_found(void *p) "could not find %p"
+xen_ram_addr_from_mapcache_found(uint64_t addr, void *p) " 0x%"PRIx64" -> %p
is present"
+xen_ram_addr_from_mapcache_not_in_cache(void *p) "Trying to find address %p
that is not in the mapcache"
+xen_replace_cache_entry_unlocked(uint64_t old_phys_addr) "Trying to update an
entry for 0x%"PRIx64" that is not in the mapcache"
+xen_invalidate_map_cache(uint64_t paddr_index, void *vaddr_req) "Locked DMA
mapping while invalidating mapcache 0x%"PRIx64" -> %p is present"
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index f7d974677d..336c212376 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -22,16 +22,6 @@
#include "trace.h"
-//#define MAPCACHE_DEBUG
-
-#ifdef MAPCACHE_DEBUG
-# define DPRINTF(fmt, ...) do { \
-fprintf(stderr, "xen_mapcache: " fmt, ## __VA_ARGS__); \
-} while (0)
-#else
-# define DPRINTF(fmt, ...) do { } while (0)
-#endif
-
#if HOST_LONG_BITS == 32
# define MCACHE_BUCKET_SHIFT 16
# define MCACHE_MAX_SIZE (1UL<<31) /* 2GB Cap */
@@ -145,8 +135,7 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void
*opaque)
size = mapcache->nr_buckets * sizeof (MapCacheEntry);
size = (size + XC_PAGE_SIZE - 1) & ~(XC_PAGE_SIZE - 1);
-DPRINTF("%s, nr_buckets = %lx size %lu\n", __func__,
-mapcache->nr_buckets, size);
+trace_xen_map_cache_init(mapcache->nr_buckets, size);
mapcache->entry = g_malloc0(size);
}
@@ -286,7 +275,9 @@ tryagain:
test_bits(address_offset >> XC_PAGE_SHIFT,
test_bit_size >> XC_PAGE_SHIFT,
mapcache->last_entry->valid_mapping)) {
-trace_xen_map_cache_return(mapcache->last_entry->vaddr_base +
address_offset);
+trace_xen_map_cache_return(
+mapcache->last_entry->vaddr_base + address_offset
+);
return mapcache->last_entry->vaddr_base + address_offset;
}
@@ -368,7 +359,9 @@ tryagain:
QTAILQ_INSERT_HEAD(>locked_entries, reventry, next);
}
-trace_xen_map_cache_return(mapcache->last_entry->vaddr_base +
address_offset);
+trace_xen_map_cache_return(
+mapcache->last_entry->vaddr_base + address_offset
+);
return mapcache->last_entry->vaddr_base + address_offset;
}
@@ -402,10 +395,10 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
}
}
if (!found) {
-fprintf(stderr, "%s, could not find %p\n", __func__, ptr);
+trace_xen_ram_addr_from_mapcache_not_found(ptr);
QTAILQ_FOREACH(reventry, >locked_entries, next) {
-DPRINTF(" "HWADDR_FMT_plx" -> %p is present\n",
reventry->paddr_index,
-reventry->vaddr_req);
+trace_xen_ram_addr_from_mapcache_found(reventry->paddr_index,
+ reventry->vaddr_req);
}
abort();
return 0;
@@ -416,7 +409,7 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
entry = entry->next;
}
if (!entry) {
[PATCH v3 3/6] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports
Tracing DPRINTFs to stderr might not be desired. A developer that relies
on trace events should be able to opt-in to each trace event and rely on
QEMU's log redirection, instead of stderr by default.
This commit converts DPRINTFs in this file that are used for tracing
into trace events. Errors or warnings are converted to error_report and
warn_report calls.
Signed-off-by: Manos Pitsidianakis
---
hw/arm/trace-events | 5 +
hw/arm/xen_arm.c| 23 +++
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 0ff41e6c78..fd0d92762e 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -65,3 +65,8 @@ z2_lcd_enable_disable_result(const char *result) "LCD %s"
z2_aer915_send_too_long(int8_t msg) "message too long (%i bytes)"
z2_aer915_send(uint8_t reg, uint8_t value) "reg %d value 0x%02x"
z2_aer915_event(int8_t event, int8_t len) "i2c event =0x%x len=%d bytes"
+
+# xen_arm.c
+xen_create_virtio_mmio_devices(int i, int irq, uint64_t base) "Created
virtio-mmio device %d: irq %d base 0x%"PRIx64
+xen_init_ram(uint64_t machine_ram_size) "Initialized xen ram with size
0x%"PRIx64
+xen_enable_tpm(uint64_t addr) "Connected tpmdev at address 0x%"PRIx64
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index a5631529d0..32776d94df 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -34,6 +34,7 @@
#include "hw/xen/xen-hvm-common.h"
#include "sysemu/tpm.h"
#include "hw/xen/arch_hvm.h"
+#include "trace.h"
#define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh")
OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
@@ -91,8 +92,9 @@ static void xen_create_virtio_mmio_devices(XenArmState *xam)
sysbus_create_simple("virtio-mmio", base, irq);
-DPRINTF("Created virtio-mmio device %d: irq %d base 0x%lx\n",
-i, GUEST_VIRTIO_MMIO_SPI_FIRST + i, base);
+trace_xen_create_virtio_mmio_devices(i,
+ GUEST_VIRTIO_MMIO_SPI_FIRST + i,
+ base);
}
}
@@ -101,6 +103,7 @@ static void xen_init_ram(MachineState *machine)
MemoryRegion *sysmem = get_system_memory();
ram_addr_t block_len, ram_size[GUEST_RAM_BANKS];
+trace_xen_init_ram(machine->ram_size);
if (machine->ram_size <= GUEST_RAM0_SIZE) {
ram_size[0] = machine->ram_size;
ram_size[1] = 0;
@@ -117,15 +120,10 @@ static void xen_init_ram(MachineState *machine)
memory_region_init_alias(_lo, NULL, "xen.ram.lo", _memory,
GUEST_RAM0_BASE, ram_size[0]);
memory_region_add_subregion(sysmem, GUEST_RAM0_BASE, _lo);
-DPRINTF("Initialized region xen.ram.lo: base 0x%llx size 0x%lx\n",
-GUEST_RAM0_BASE, ram_size[0]);
-
if (ram_size[1] > 0) {
memory_region_init_alias(_hi, NULL, "xen.ram.hi", _memory,
GUEST_RAM1_BASE, ram_size[1]);
memory_region_add_subregion(sysmem, GUEST_RAM1_BASE, _hi);
-DPRINTF("Initialized region xen.ram.hi: base 0x%llx size 0x%lx\n",
-GUEST_RAM1_BASE, ram_size[1]);
}
}
@@ -158,7 +156,7 @@ static void xen_enable_tpm(XenArmState *xam)
TPMBackend *be = qemu_find_tpm_be("tpm0");
if (be == NULL) {
-DPRINTF("Couldn't fine the backend for tpm0\n");
+error_report("Couldn't find tmp0 backend");
return;
}
dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
@@ -168,7 +166,7 @@ static void xen_enable_tpm(XenArmState *xam)
sysbus_realize_and_unref(busdev, _fatal);
sysbus_mmio_map(busdev, 0, xam->cfg.tpm_base_addr);
-DPRINTF("Connected tpmdev at address 0x%lx\n", xam->cfg.tpm_base_addr);
+trace_xen_enable_tpm(xam->cfg.tpm_base_addr);
}
#endif
@@ -179,8 +177,9 @@ static void xen_arm_init(MachineState *machine)
xam->state = g_new0(XenIOState, 1);
if (machine->ram_size == 0) {
-DPRINTF("ram_size not specified. QEMU machine started without IOREQ"
-"(no emulated devices including Virtio)\n");
+warn_report("%s non-zero ram size not specified. QEMU machine started"
+" without IOREQ (no emulated devices including virtio)",
+MACHINE_CLASS(object_get_class(OBJECT(machine)))->desc);
return;
}
@@ -194,7 +193,7 @@ static void xen_arm_init(MachineState *machine)
if (xam->cfg.tpm_base_addr) {
xen_enable_tpm(xam);
} else {
-DPRINTF("tpm-base-addr is not provided. TPM will not be enabled\n");
+warn_report("tpm-base-addr is not provided. TPM will not be enabled");
}
#endif
}
--
γαῖα πυρί μιχθήτω
[PATCH v3 5/6] hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
Tracing DPRINTFs to stderr might not be desired. A developer that relies
on tracepoints should be able to opt-in to each tracepoint and rely on
QEMU's log redirection, instead of stderr by default.
This commit converts DPRINTFs in this file that are used for tracing
into tracepoints.
Signed-off-by: Manos Pitsidianakis
---
hw/xen/trace-events | 10 +-
hw/xen/xen-hvm-common.c | 35 ++-
2 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index a65dc0e55f..d1b27f6c11 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -42,7 +42,7 @@ xs_node_vscanf(char *path, char *value) "%s %s"
xs_node_watch(char *path) "%s"
xs_node_unwatch(char *path) "%s"
-# xen-hvm.c
+# xen-hvm-common.c
xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: 0x%lx,
size 0x%lx"
xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty)
"0x%"PRIx64" size 0x%lx, log_dirty %i"
handle_ioreq(void *req, uint32_t type, uint32_t dir, uint32_t df, uint32_t
data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size)
"I/O=%p type=%d dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d
size=%d"
@@ -55,6 +55,14 @@ cpu_ioreq_move(void *req, uint32_t dir, uint32_t df,
uint32_t data_is_ptr, uint6
xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size,
uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size,
uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
+cpu_get_ioreq_from_shared_memory_req_not_ready(int state, int data_is_ptr,
uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O request not
ready: 0x%x, ptr: 0x%x, port: 0x%"PRIx64", data: 0x%"PRIx64", count: %u, size:
%u"
+xen_main_loop_prepare_init_cpu(int id, void *cpu) "cpu_by_vcpu_id[%d]=%p"
+xen_map_ioreq_server_shared_page(long unsigned int ioreq_pfn) "shared page at
pfn 0x%lx"
+xen_map_ioreq_server_buffered_io_page(long unsigned int ioreq_pfn) "buffered
io page at pfn 0x%lx"
+xen_map_ioreq_server_buffered_io_evtchn(int bufioreq_evtchn) "buffered io
evtchn is 0x%x"
+destroy_hvm_domain_cannot_acquire_handle(void) "Cannot acquire xenctrl handle"
+destroy_hvm_domain_failed_action(const char *action, int sts, char *errno_s)
"xc_domain_shutdown failed to issue %s, sts %d, %s"
+destroy_hvm_domain_action(int xen_domid, const char *action) "Issued domain %d
%s"
# xen-mapcache.c
xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 47e6cb1db3..05a29c6f11 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -169,11 +169,12 @@ static ioreq_t
*cpu_get_ioreq_from_shared_memory(XenIOState *state, int vcpu)
ioreq_t *req = xen_vcpu_ioreq(state->shared_page, vcpu);
if (req->state != STATE_IOREQ_READY) {
-DPRINTF("I/O request not ready: "
-"%x, ptr: %x, port: %"PRIx64", "
-"data: %"PRIx64", count: %u, size: %u\n",
-req->state, req->data_is_ptr, req->addr,
-req->data, req->count, req->size);
+trace_cpu_get_ioreq_from_shared_memory_req_not_ready(req->state,
+ req->data_is_ptr,
+ req->addr,
+ req->data,
+ req->count,
+ req->size);
return NULL;
}
@@ -601,10 +602,9 @@ static void xen_main_loop_prepare(XenIOState *state)
if (evtchn_fd != -1) {
CPUState *cpu_state;
-DPRINTF("%s: Init cpu_by_vcpu_id\n", __func__);
CPU_FOREACH(cpu_state) {
-DPRINTF("%s: cpu_by_vcpu_id[%d]=%p\n",
-__func__, cpu_state->cpu_index, cpu_state);
+trace_xen_main_loop_prepare_init_cpu(cpu_state->cpu_index,
+ cpu_state);
state->cpu_by_vcpu_id[cpu_state->cpu_index] = cpu_state;
}
qemu_set_fd_handler(evtchn_fd, cpu_handle_ioreq, NULL, state);
@@ -681,7 +681,7 @@ static int xen_map_ioreq_server(XenIOState *state)
}
if (state->shared_page == NULL) {
-DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
+trace_xen_map_ioreq
[PATCH v3 1/6] hw/arm/strongarm.c: convert DPRINTF to trace events and guest errors
Tracing DPRINTFs to stderr might not be desired. A developer that relies
on trace events should be able to opt-in to each trace event and rely on
QEMU's log redirection, instead of stderr by default.
This commit converts DPRINTFs in this file that are used for tracing
into trace events. DPRINTFs that report guest errors are logged with
LOG_GUEST_ERROR.#
Signed-off-by: Manos Pitsidianakis
---
hw/arm/strongarm.c | 82 -
hw/arm/trace-events | 3 ++
2 files changed, 55 insertions(+), 30 deletions(-)
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index 7fd99a0f14..823b4931b0 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -46,8 +46,7 @@
#include "qemu/log.h"
#include "qom/object.h"
#include "target/arm/cpu-qom.h"
-
-//#define DEBUG
+#include "trace.h"
/*
TODO
@@ -66,12 +65,6 @@
- Enhance UART with modem signals
*/
-#ifdef DEBUG
-# define DPRINTF(format, ...) printf(format , ## __VA_ARGS__)
-#else
-# define DPRINTF(format, ...) do { } while (0)
-#endif
-
static struct {
hwaddr io_base;
int irq;
@@ -151,8 +144,9 @@ static uint64_t strongarm_pic_mem_read(void *opaque, hwaddr
offset,
case ICPR:
return s->pending;
default:
-printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
-__func__, offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad register offset 0x"HWADDR_FMT_plx"\n",
+ __func__, offset);
return 0;
}
}
@@ -173,8 +167,9 @@ static void strongarm_pic_mem_write(void *opaque, hwaddr
offset,
s->int_idle = (value & 1) ? 0 : ~0;
break;
default:
-printf("%s: Bad register offset 0x" HWADDR_FMT_plx "\n",
-__func__, offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad register offset 0x"HWADDR_FMT_plx"\n",
+ __func__, offset);
break;
}
strongarm_pic_update(s);
@@ -333,7 +328,9 @@ static uint64_t strongarm_rtc_read(void *opaque, hwaddr
addr,
((qemu_clock_get_ms(rtc_clock) - s->last_hz) << 15) /
(1000 * ((s->rttr & 0x) + 1));
default:
-printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad rtc register read 0x"HWADDR_FMT_plx"\n",
+ __func__, addr);
return 0;
}
}
@@ -375,7 +372,9 @@ static void strongarm_rtc_write(void *opaque, hwaddr addr,
break;
default:
-printf("%s: Bad register 0x" HWADDR_FMT_plx "\n", __func__, addr);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad rtc register write 0x"HWADDR_FMT_plx"\n",
+ __func__, addr);
}
}
@@ -556,12 +555,12 @@ static uint64_t strongarm_gpio_read(void *opaque, hwaddr
offset,
case GPSR:/* GPIO Pin-Output Set registers */
qemu_log_mask(LOG_GUEST_ERROR,
- "strongarm GPIO: read from write only register GPSR\n");
+ "%s: read from write only register GPSR\n", __func__);
return 0;
case GPCR:/* GPIO Pin-Output Clear registers */
qemu_log_mask(LOG_GUEST_ERROR,
- "strongarm GPIO: read from write only register GPCR\n");
+ "%s: read from write only register GPCR\n", __func__);
return 0;
case GRER:/* GPIO Rising-Edge Detect Enable registers */
@@ -581,7 +580,9 @@ static uint64_t strongarm_gpio_read(void *opaque, hwaddr
offset,
return s->status;
default:
-printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad gpio read offset 0x"HWADDR_FMT_plx"\n",
+ __func__, offset);
}
return 0;
@@ -626,7 +627,9 @@ static void strongarm_gpio_write(void *opaque, hwaddr
offset,
break;
default:
-printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad write offset 0x"HWADDR_FMT_plx"\n",
+ __func__, offset);
}
}
@@ -782,7 +785,9 @@ static uint64_t strongarm_ppc_read(void *opaque, hwaddr
offset,
return s->ppfr | ~0x7f001;
default:
-printf("%s: Bad offset 0x" HWADDR_FMT_plx "\n", __func__, offset);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bad ppc read offset 0x"HWA
[PATCH v3 0/6] hw/{arm,xen} convert printfs to trace/reports
This series changes some printfs to use the trace event framework. Additionally, it converts some error/warning reporting fprintfs to error_report/warn_report. v2 -> v3 : - addressed Peter Maydells's review v1 -> v2 : - addressed Alex's review Manos Pitsidianakis (6): hw/arm/strongarm.c: convert DPRINTF to trace events and guest errors hw/arm/z2: convert DPRINTF to trace events and guest errors hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints hw/xen: convert stderr prints to error/warn reports hw/arm/strongarm.c | 82 ++--- hw/arm/trace-events | 15 hw/arm/xen_arm.c| 23 ++-- hw/arm/z2.c | 27 -- hw/xen/trace-events | 21 ++- hw/xen/xen-hvm-common.c | 47 +++ hw/xen/xen-mapcache.c | 59 + 7 files changed, 156 insertions(+), 118 deletions(-) base-commit: 7a1dc45af581d2b643cdbf33c01fd96271616fbd -- γαῖα πυρί μιχθήτω
[PATCH v2 0/2] hw/block/block.c: improve confusing error
In cases where a device tries to read more bytes than the block device
contains with the blk_check_size_and_read_all() function, the error is
vague: "device requires X bytes, block backend provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Version 2:
- Assert dev is not NULL on qdev_get_human_name
(thanks Phil Mathieu-Daudé )
Manos Pitsidianakis (2):
hw/core/qdev.c: add qdev_get_human_name()
hw/block/block.c: improve confusing blk_check_size_and_read_all()
error
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
hw/core/qdev.c | 8
include/hw/block/block.h | 4 ++--
include/hw/qdev-core.h | 14 ++
7 files changed, 44 insertions(+), 16 deletions(-)
Range-diff against v1:
1: 15b15d6d4f ! 1: 5fb5879708 hw/core/qdev.c: add qdev_get_human_name()
@@ hw/core/qdev.c: Object *qdev_get_machine(void)
+char *qdev_get_human_name(DeviceState *dev)
+{
-+if (!dev) {
-+return g_strdup("");
-+}
++g_assert(dev != NULL);
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
@@ include/hw/qdev-core.h: const char *qdev_fw_name(DeviceState *dev);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
-+ * @dev: The device
++ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
-+ * else the object canonical path if not null. If @dev is NULL, it
returns an
-+ * allocated empty string.
++ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
2: e3701762ed ! 2: 8e7eb17fbd hw/block/block.c: improve confusing
blk_check_size_and_read_all() error
@@ Commit message
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
+Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
## hw/block/block.c ##
base-commit: 09be34717190c1620f0c6e5c8765b8da354aeb4b
--
γαῖα πυρί μιχθήτω
[PATCH v2 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
---
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
include/hw/block/block.h | 4 ++--
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..624389d62d 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
* BDRV_REQUEST_MAX_BYTES.
* On success, return true.
* On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
* This function not intended for actual block devices, which read on
* demand. It's for things like memory devices that (ab)use a block
* backend to provide persistence.
*/
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp)
{
int64_t blk_len;
int ret;
+g_autofree char *dev_id = NULL;
blk_len = blk_getlength(blk);
if (blk_len < 0) {
error_setg_errno(errp, -blk_len,
- "can't get size of block backend");
+ "can't get size of %s block backend", blk_name(blk));
return false;
}
if (blk_len != size) {
-error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
- "block backend provides %" PRIu64 " bytes",
- size, blk_len);
+dev_id = qdev_get_human_name(dev);
+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu
+ " bytes, %s block backend provides %" PRIu64 " bytes",
+ object_get_typename(OBJECT(dev)), dev_id, size,
+ blk_name(blk), blk_len);
return false;
}
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
*buf, hwaddr size,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
-error_setg_errno(errp, -ret, "can't read block backend");
+dev_id = qdev_get_human_name(dev);
+error_setg_errno(errp, -ret, "can't read %s block backend"
+ "for %s device with id='%s'",
+ blk_name(blk), object_get_typename(OBJECT(dev)),
+ dev_id);
return false;
}
return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+ s->storage, s->size, errp)) {
return;
}
} else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
- errp)) {
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+ total_len, errp)) {
vmstate_unregister_ram(>mem, DEVICE(pfl));
return;
}
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6fa56f14c0..2314142373 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -902,7 +902,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
pfl->chip_len, errp)) {
vmstate_unregister_ram(>orig_mem, DEVICE(
[PATCH v2 1/2] hw/core/qdev.c: add qdev_get_human_name()
Add a simple method to return some kind of human readable identifier for
use in error messages.
Signed-off-by: Manos Pitsidianakis
---
hw/core/qdev.c | 8
include/hw/qdev-core.h | 14 ++
2 files changed, 22 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..c68d0f7c51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,14 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+g_assert(dev != NULL);
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..66338f479f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,20 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
--
γαῖα πυρί μιχθήτω
Re: [PATCH 1/2] hw/core/qdev.c: add qdev_get_human_name()
On Tue, 23 Jan 2024 10:13, Philippe Mathieu-Daudé wrote:
Hi Manos,
On 23/1/24 09:09, Manos Pitsidianakis wrote:
Add a simple method to return some kind of human readable identifier for
use in error messages.
Signed-off-by: Manos Pitsidianakis
---
hw/core/qdev.c | 10 ++
include/hw/qdev-core.h | 15 +++
2 files changed, 25 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..499f191826 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,16 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+if (!dev) {
+return g_strdup("");
+}
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..a8c742b4a3 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,21 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path if not null. If @dev is NULL, it returns an
+ * allocated empty string.
In which case do we want to call this with NULL?
None I could think of, just future-proofing the NULL case.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
[PATCH 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Signed-off-by: Manos Pitsidianakis
---
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
include/hw/block/block.h | 4 ++--
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..624389d62d 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
* BDRV_REQUEST_MAX_BYTES.
* On success, return true.
* On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
* This function not intended for actual block devices, which read on
* demand. It's for things like memory devices that (ab)use a block
* backend to provide persistence.
*/
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp)
{
int64_t blk_len;
int ret;
+g_autofree char *dev_id = NULL;
blk_len = blk_getlength(blk);
if (blk_len < 0) {
error_setg_errno(errp, -blk_len,
- "can't get size of block backend");
+ "can't get size of %s block backend", blk_name(blk));
return false;
}
if (blk_len != size) {
-error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
- "block backend provides %" PRIu64 " bytes",
- size, blk_len);
+dev_id = qdev_get_human_name(dev);
+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu
+ " bytes, %s block backend provides %" PRIu64 " bytes",
+ object_get_typename(OBJECT(dev)), dev_id, size,
+ blk_name(blk), blk_len);
return false;
}
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
*buf, hwaddr size,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
-error_setg_errno(errp, -ret, "can't read block backend");
+dev_id = qdev_get_human_name(dev);
+error_setg_errno(errp, -ret, "can't read %s block backend"
+ "for %s device with id='%s'",
+ blk_name(blk), object_get_typename(OBJECT(dev)),
+ dev_id);
return false;
}
return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+ s->storage, s->size, errp)) {
return;
}
} else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
- errp)) {
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+ total_len, errp)) {
vmstate_unregister_ram(>mem, DEVICE(pfl));
return;
}
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6fa56f14c0..2314142373 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -902,7 +902,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
pfl->chip_len, errp)) {
vmstate_unregister_ram(>orig_mem, DEVICE(pfl));
return;
di
[PATCH 1/2] hw/core/qdev.c: add qdev_get_human_name()
Add a simple method to return some kind of human readable identifier for
use in error messages.
Signed-off-by: Manos Pitsidianakis
---
hw/core/qdev.c | 10 ++
include/hw/qdev-core.h | 15 +++
2 files changed, 25 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..499f191826 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,16 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+if (!dev) {
+return g_strdup("");
+}
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..a8c742b4a3 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,21 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path if not null. If @dev is NULL, it returns an
+ * allocated empty string.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
--
γαῖα πυρί μιχθήτω
[PATCH 0/2] hw/block/block.c: improve confusing error
In cases where a device tries to read more bytes than the block device contains with the blk_check_size_and_read_all() function, the error is vague: "device requires X bytes, block backend provides Y bytes". This patch changes the errors of this function to include the block backend name, the device id and device type name where appropriate. Manos Pitsidianakis (2): hw/core/qdev.c: add qdev_get_human_name() hw/block/block.c: improve confusing blk_check_size_and_read_all() error hw/block/block.c | 25 +++-- hw/block/m25p80.c| 3 ++- hw/block/pflash_cfi01.c | 4 ++-- hw/block/pflash_cfi02.c | 2 +- hw/core/qdev.c | 10 ++ include/hw/block/block.h | 4 ++-- include/hw/qdev-core.h | 15 +++ 7 files changed, 47 insertions(+), 16 deletions(-) base-commit: 09be34717190c1620f0c6e5c8765b8da354aeb4b -- γαῖα πυρί μιχθήτω
Re: [PATCH v5 5/6] hw/virtio: add vhost-user-snd and virtio-snd-pci devices
On Fri, 20 Oct 2023 12:02, "Michael S. Tsirkin" wrote: On Fri, Oct 20, 2023 at 09:16:03AM +0100, Alex Bennée wrote: Viresh Kumar writes: > On 19-10-23, 10:56, Alex Bennée wrote: >> From: Manos Pitsidianakis >> >> Tested with rust-vmm vhost-user-sound daemon: >> >> RUST_LOG=trace cargo run --bin vhost-user-sound -- --socket /tmp/snd.sock --backend null >> >> Invocation: >> >> qemu-system-x86_64 \ >> -qmp unix:./qmp-sock,server,wait=off \ >> -m 4096 \ >> -numa node,memdev=mem \ >> -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \ >> -D qemu.log \ >> -d guest_errors,trace:\*snd\*,trace:\*sound\*,trace:\*vhost\* \ >> -chardev socket,id=vsnd,path=/tmp/snd.sock \ >> -device vhost-user-snd-pci,chardev=vsnd,id=snd \ >> /path/to/disk >> >> [AJB: imported from https://github.com/epilys/qemu-virtio-snd/commit/54ae1cdd15fef2d88e9e387a175f099a38c636f4.patch] >> >> Signed-off-by: Alex Bennée > > Missing SOB from Manos ? oops, guess I need a respin then ;-) Just ask Manos to send his S.O.B in a reply. Signed-off-by: Manos Pitsidianakis
Re: [PATCH v8 3/5] vhost-user-scsi: support reconnect to backend
If the backend crashes and restarts, the device is broken. This patch adds reconnect for vhost-user-scsi. This patch also improves the error messages, and reports some silent errors. Tested with spdk backend. Signed-off-by: Li Feng --- Reviewed-by: Manos Pitsidianakis
Re: [PATCH 1/7] hw/virtio/virtio-pmem: Replace impossible check by assertion
On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé wrote:
The get_memory_region() handler is used when (un)plugging the
device, which can only occur *after* it is realized.
virtio_pmem_realize() ensure the instance can not be realized
without 'memdev'. Remove the superfluous check, replacing it
by an assertion.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/virtio/virtio-pmem.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index c3512c2dae..cc24812d2e 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -147,10 +147,7 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM
*pmem,
static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
Error **errp)
{
-if (!pmem->memdev) {
-error_setg(errp, "'%s' property must be set", VIRTIO_PMEM_MEMDEV_PROP);
-return NULL;
-}
+assert(pmem->memdev);
return >memdev->mr;
}
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro
On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé wrote:
Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/virtio-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
{
-VirtIOSCSICommon *vs = >parent_obj;
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
SCSIDevice *d;
int rc;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 3/7] hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro
On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé wrote: Access QOM parent with the proper QOM VIRTIO_DEVICE() macro. Signed-off-by: Philippe Mathieu-Daudé --- hw/display/virtio-gpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 93857ad523..51cb517999 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1132,7 +1132,7 @@ static void virtio_gpu_ctrl_bh(void *opaque) VirtIOGPU *g = opaque; VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g); -vgc->handle_ctrl(>parent_obj.parent_obj, g->ctrl_vq); +vgc->handle_ctrl(VIRTIO_DEVICE(g), g->ctrl_vq); } static void virtio_gpu_handle_cursor(VirtIODevice *vdev, VirtQueue *vq) -- 2.41.0 Reviewed-by: Manos Pitsidianakis
Re: [PATCH 2/7] hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros
On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé wrote:
Access QOM parent with the proper QOM [VIRTIO_]DEVICE() macros.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/vhost-user-blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..4b37e26120 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -405,7 +405,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent
event)
static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
{
-DeviceState *dev = >parent_obj.parent_obj;
+DeviceState *dev = DEVICE(s);
int ret;
s->connected = false;
@@ -423,7 +423,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s,
Error **errp)
assert(s->connected);
ret = vhost_dev_get_config(>dev, (uint8_t *)>blkcfg,
- s->parent_obj.config_len, errp);
+ VIRTIO_DEVICE(s)->config_len, errp);
if (ret < 0) {
qemu_chr_fe_disconnect(>chardev);
vhost_dev_cleanup(>dev);
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [RFC PATCH 00/78] Strict disable implicit fallthrough
On Mon, 16 Oct 2023, 18:04 Peter Maydell, wrote: > On Mon, 16 Oct 2023 at 15:58, Manos Pitsidianakis > wrote: > > > > Hello Peter, > > > > On Mon, 16 Oct 2023, 17:13 Peter Maydell, > wrote: > >> > >> On Fri, 13 Oct 2023 at 13:42, Markus Armbruster > wrote: > >> > > >> > Emmanouil Pitsidianakis writes: > >> > > >> > > Hello, > >> > > > >> > > This RFC is inspired by the kernel's move to > -Wimplicit-fallthrough=3 > >> > > back in 2019.[0] > >> > > We take one step (or two) further by increasing it to 5 which > rejects > >> > > fall through comments and requires an attribute statement. > >> > > > >> > > [0]: > >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > >> > > > >> > > The line differences are not many, but they spread all over > different > >> > > subsystems, architectures and devices. An attempt has been made to > split > >> > > them in cohesive patches to aid post-RFC review. Part of the RFC is > to > >> > > determine whether these patch divisions needs improvement. > >> > > > >> > > Main questions this RFC poses > >> > > = > >> > > > >> > > - Is this change desirable and net-positive. > >> > > >> > Unwanted fallthrough is an easy mistake to make, and > >> > -Wimplicit-fallthrough=N helps avoid it. The question is how far up > we > >> > need to push N. Right now we're at N=2. Has unwanted fallthrough > been > >> > a problem? > >> > >> Mmm, this is my opinion I think. We have a mechanism for > >> catching "forgot the 'break'" already (our =2 setting) and > >> a way to say "intentional" in a fairly natural way (add the > >> comment). Does pushing N up any further gain us anything > >> except a load of churn? > >> > >> Also, the compiler is not the only thing that processes our > >> code: Coverity also looks for "unexpected fallthrough" issues, > >> so if we wanted to switch away from our current practice we > >> should check whether what we're switching to is an idiom > >> that Coverity recognises. > > > > > > It is a code style change as the cover letter mentions, it's not related > to the static analysis itself. > > Yes, exactly. As a code style change it needs a fairly high level > of justification for the code churn, and the cover letter > doesn't really provide one... > As I state in the cover letter, I personally find that using one macro instead of a comment regex feels more consistent. But your view is valid as well! Let's consider the RFC retracted then. -- Manos >
Re: [RFC PATCH 00/78] Strict disable implicit fallthrough
Hello Peter, On Mon, 16 Oct 2023, 17:13 Peter Maydell, wrote: > On Fri, 13 Oct 2023 at 13:42, Markus Armbruster wrote: > > > > Emmanouil Pitsidianakis writes: > > > > > Hello, > > > > > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > > > back in 2019.[0] > > > We take one step (or two) further by increasing it to 5 which rejects > > > fall through comments and requires an attribute statement. > > > > > > [0]: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > > > > > The line differences are not many, but they spread all over different > > > subsystems, architectures and devices. An attempt has been made to > split > > > them in cohesive patches to aid post-RFC review. Part of the RFC is to > > > determine whether these patch divisions needs improvement. > > > > > > Main questions this RFC poses > > > = > > > > > > - Is this change desirable and net-positive. > > > > Unwanted fallthrough is an easy mistake to make, and > > -Wimplicit-fallthrough=N helps avoid it. The question is how far up we > > need to push N. Right now we're at N=2. Has unwanted fallthrough been > > a problem? > > Mmm, this is my opinion I think. We have a mechanism for > catching "forgot the 'break'" already (our =2 setting) and > a way to say "intentional" in a fairly natural way (add the > comment). Does pushing N up any further gain us anything > except a load of churn? > > Also, the compiler is not the only thing that processes our > code: Coverity also looks for "unexpected fallthrough" issues, > so if we wanted to switch away from our current practice we > should check whether what we're switching to is an idiom > that Coverity recognises. > It is a code style change as the cover letter mentions, it's not related to the static analysis itself. -- Manos >
Re: [RFC PATCH 00/78] Strict disable implicit fallthrough
On Fri, 13 Oct 2023 11:14, "Daniel P. Berrangé" wrote: On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote: Main questions this RFC poses = - Is this change desirable and net-positive. Yes, IMHO it is worth standardizing on use of the attribute. The allowed use of comments was a nice thing by the compiler for coping with pre-existing code, but using the attribute is best long term for a consistent style. - Should the `fallthrough;` pseudo-keyword be defined like in the Linux kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing QEMU_FALLTHROUGH macro. As a general rule, if glib provides functionality we aim o use that and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH. I agree. My reasoning was: - The reinvented wheel is only an attribute and not a big bunch of NIH code - The macro def in glib depends on the glib version you use - G_GNUC_FALLTHROUGH looks kind of abrasive to my eye, while `fallthrough` blends in with other switch keywords like break. - C23 standardises fallthrough. We might not ever support C23 but it's good to be consistent with standards and other, larger projects (linux kernel). I think these (except for myself finding G_GNUC_FALLTHROUGH ugly) make a strong case for not using the glib macro, personally. I'd be interested to know if there is a counterpoint to it: because I don't want this change to cause problems in the future. Manos
Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
Hello Markus,
On Fri, 13 Oct 2023 15:28, Markus Armbruster wrote:
The commit message needs to explain why.
Certainly.
This is wrong. docs/devel/style.rst:
Include directives
--
Order include directives as follows:
.. code-block:: c
#include "qemu/osdep.h" /* Always first... */
#include <...> /* then system headers... */
#include "..." /* and finally QEMU headers. */
Separate patch, please.
I know. spa headers use the `fallthrough` attribute and qemu/compiler.h
defines it as a macro, so it breaks compilation. If the spa headers go
after, we'd need to undef fallthrough before including them and
re-define or re-include qemu/compiler.h after. What do you think would
be best?
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 1109482a00..959982805d 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -1,215 +1,231 @@
[...]
#define QEMU_ALWAYS_INLINE
#endif
-/**
- * In most cases, normal "fallthrough" comments are good enough for
- * switch-case statements, but sometimes the compiler has problems
- * with those. In that case you can use QEMU_FALLTHROUGH instead.
+/*
+ * Add the pseudo keyword 'fallthrough' so case statement blocks
Pseudo-keyword? It's a macro.
C calls reserved words that you cannot redefine 'keywords'. Like
'break', 'continue', 'return'. Hence it's a pseudo-keyword. It's also a
macro. They are not mutually exclusive.
I did not write this, it was taken verbatim from the linux kernel
source, see: /include/linux/compiler_attributes.h
+ * must end with any of these keywords:
+ * break;
+ * fallthrough;
+ * continue;
+ * goto ;
+ * return [expression];
These are statements, not keywords.
I'm pretty sure it refers to {break, fallthrough, continue, goto,
return} by themselves.
+ *
+ * gcc:
https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
Not sure we need to point to the docs here. We have hundreds of
__attribute__ uses in the tree.
Again, copied from /include/linux/compiler_attributes.h
*/
-#if __has_attribute(fallthrough)
-# define QEMU_FALLTHROUGH __attribute__((fallthrough))
+
+/*
+ * glib_macros.h contains its own definition of fallthrough, so if we define
+ * the pseudokeyword here it will expand when the glib header checks for the
+ * attribute. glib headers must be #included after this header.
+ */
+#ifdef fallthrough
+#undef fallthrough
+#endif
Why do we need to roll our own macro then?
Glib uses a different name. The problem is when it checks for the
compiler attribute, which expands into our macro.
--
Manos
Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
On Fri, 13 Oct 2023 at 11:16, Daniel P. Berrangé wrote: > This patch (and all the others in the series) have a ridiculously > large context either side of the change. It makes this horrible > to review as it requires wading through pages of pre-existing code > trying to spot the change. > > Please send patches with the default git context lines setting. Many thanks Daniel, I had not noticed at all. Somehow that option slipped through... Will reroll the patch series.
Re: [PATCH 03/10] tests/virtio-scsi: Clean up global variable shadowing
On Mon, 09 Oct 2023 13:02, Philippe Mathieu-Daudé wrote:
Rename the (unused) 'allow' argument, following the pattern
s/allow/alloc
Otherwise,
Reviewed-By: Emmanouil Pitsidianakis
used by the other tests in this file. This fixes:
tests/qtest/virtio-scsi-test.c:159:61: error: declaration shadows a variable
in the global scope [-Werror,-Wshadow]
static void hotplug(void *obj, void *data, QGuestAllocator *alloc)
^
tests/qtest/virtio-scsi-test.c:37:25: note: previous declaration is here
static QGuestAllocator *alloc;
^
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/virtio-scsi-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index ceaa7f2415..db10d572d0 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -156,7 +156,7 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice
*dev)
return vs;
}
-static void hotplug(void *obj, void *data, QGuestAllocator *alloc)
+static void hotplug(void *obj, void *data, QGuestAllocator *t_alloc)
{
QTestState *qts = global_qtest;
--
2.41.0
Re: [PATCH v4 6/6] docs/system: add a basic enumeration of vhost-user devices
On Mon, 09 Oct 2023 12:59, Alex Bennée wrote: diff --git a/docs/system/devices/vhost-user.rst b/docs/system/devices/vhost-user.rst index a80e95a48a..0f9eec3f00 100644 --- a/docs/system/devices/vhost-user.rst +++ b/docs/system/devices/vhost-user.rst @@ -15,6 +15,47 @@ to the guest. The code is mostly boilerplate although each device has a ``chardev`` option which specifies the ID of the ``--chardev`` device that connects via a socket to the vhost-user *daemon*. +Each device will have an virtio-mmio and virtio-pci variant. See your +platform details for what sort of virtio bus to use. + +.. list-table:: vhost-user devices + :widths: 20 20 60 + :header-rows: 1 + + * - Device +- Type +- Notes + * - vhost-user-device +- Generic Development Device +- You must manually specify ``virtio-id`` and the correct ``num_vqs``. Intended for expert use. May be worth specifying they are `VHostUserBase` interface fields since it's not directly obvious if you come across this before reading the vhost-user-device code. + * - vhost-user-blk +- Block storage +- + * - vhost-user-fs +- File based storage driver +- See https://gitlab.com/virtio-fs/virtiofsd + * - vhost-user-scsi +- SCSI based storage +- See contrib/vhost-user/scsi + * - vhost-user-gpio +- Proxy gpio pins to host +- See https://github.com/rust-vmm/vhost-device + * - vhost-user-i2c +- Proxy i2c devices to host +- See https://github.com/rust-vmm/vhost-device + * - vhost-user-input +- Generic input driver +- See contrib/vhost-user-input + * - vhost-user-rng +- Entropy driver +- :ref:`vhost_user_rng` + * - vhost-user-gpu +- GPU driver +- + * - vhost-user-vsock +- Socket based communication +- See https://github.com/rust-vmm/vhost-device + There's also: - hw/virtio/vhost-user-scmi.c - hw/virtio/vhost-user-snd.c
Re: [PATCH v4 2/6] hw/virtio: derive vhost-user-rng from vhost-user-base
On Mon, 09 Oct 2023 12:59, Alex Bennée wrote:
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 51c3f97c2d..d0b963199c 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -18,8 +18,15 @@ if have_vhost
# fixme - this really should be generic
specific_virtio_ss.add(files('vhost-user.c'))
system_virtio_ss.add(files('vhost-user-base.c'))
+
+# MMIO Stubs
system_virtio_ss.add(files('vhost-user-device.c'))
+system_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true:
files('vhost-user-rng.c'))
+
+# PCI Stubs
system_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true:
files('vhost-user-device-pci.c'))
+system_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
+ if_true: files('vhost-user-rng-pci.c'))
Is there a reason why the target was moved to system_virtio_ss from
virtio_pci_ss?
endif
if have_vhost_vdpa
system_virtio_ss.add(files('vhost-vdpa.c'))
@@ -34,10 +41,8 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_FS',
if_true: files('vhost-user-
specific_virtio_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true:
files('virtio-pmem.c'))
specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true:
files('vhost-vsock.c'))
specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true:
files('vhost-user-vsock.c'))
-specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true:
files('virtio-rng.c'))
Was this accidental? It's not added anywhere else, only deleted.
@@ -57,7 +61,6 @@ virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_FS',
if_true: files('vhost-user-fs-pc
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
files('virtio-crypto-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT_HOST', if_true:
files('virtio-input-host-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true:
files('virtio-input-pci.c'))
-virtio_pci_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true:
files('virtio-rng-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true:
files('virtio-balloon-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true:
files('virtio-scsi-pci.c'))
Same here
Manos
Re: [PATCH v7 3/5] vhost-user-scsi: support reconnect to backend
Hello Li, I have some trivial style comments you could possibly address
in a next version:
On Sun, 08 Oct 2023 12:12, Li Feng wrote:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+int ret;
+
+ret = vhost_scsi_common_start(vsc, errp);
+s->started_vu = (ret < 0 ? false : true);
-+s->started_vu = (ret < 0 ? false : true);
++s->started_vu = !(ret < 0);
static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t
status)
{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
-+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(vdev);
+static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VHostUserSCSI *s = VHOST_USER_SCSI(vdev);
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+int ret = 0;
+
+if (s->connected) {
+return 0;
+}
+s->connected = true;
+
+vsc->dev.num_queues = vs->conf.num_queues;
+vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
+vsc->dev.vqs = s->vhost_vqs;
+vsc->dev.vq_index = 0;
+vsc->dev.backend_features = 0;
+
+ret = vhost_dev_init(>dev, >vhost_user, VHOST_BACKEND_TYPE_USER, 0,
+ errp);
+if (ret < 0) {
+return ret;
+}
+
+/* restore vhost state */
+if (virtio_device_started(vdev, vdev->status)) {
+ret = vhost_user_scsi_start(s, errp);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
-+if (virtio_device_started(vdev, vdev->status)) {
-+ret = vhost_user_scsi_start(s, errp);
-+if (ret < 0) {
-+return ret;
-+}
-+}
-+
-+return 0;
-+}
++if (virtio_device_started(vdev, vdev->status)) {
++ret = vhost_user_scsi_start(s, errp);
++}
++
++return ret;
++}
[skipping..]
+static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
+{
+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
-+DeviceState *dev = >parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(s);
diff --git a/include/hw/virtio/vhost-user-scsi.h
b/include/hw/virtio/vhost-user-scsi.h
index 521b08e559..b405ec952a 100644
--- a/include/hw/virtio/vhost-user-scsi.h
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -29,6 +29,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserSCSI, VHOST_USER_SCSI)
struct VHostUserSCSI {
VHostSCSICommon parent_obj;
VhostUserState vhost_user;
+bool connected;
+bool started_vu;
+
+struct vhost_virtqueue *vhost_vqs;
+bool connected;
+bool started_vu;
-+
+struct vhost_virtqueue *vhost_vqs;
See
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations
The definition should look like:
struct VHostUserSCSI {
VHostSCSICommon parent_obj;
/* Properties */
bool connected;
bool started_vu;
VhostUserState vhost_user;
struct vhost_virtqueue *vhost_vqs;
}
Re: [Qemu-block] [PATCH v2] throttle: fix a qemu crash problem when calling blk_delete
On Fri, Nov 03, 2017 at 02:26:33PM +, Stefan Hajnoczi wrote:
On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote:
On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote:
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember
*tgm)
>
> /* remove the current tgm from the list */
> QLIST_REMOVE(tgm, round_robin);
> -throttle_timers_destroy(>throttle_timers);
> +if (throttle_timers_are_initialized(>throttle_timers)) {
> +throttle_timers_destroy(>throttle_timers);
> +}
> qemu_mutex_unlock(>lock);
>
> throttle_group_unref(>ts);
I don't know what the rest of the people think, but I'm not completely
convinced that it's a good idea to have an active ThrottleState inside a
ThrottleGroupMember without timers.
Perhaps in blk_remove_bs() after detaching the AioContext from the BDS
we can attach the default one (what you would get with
blk_get_aio_context()).
On the other hand I think that Manos's series to remove the legacy
throttling code gets rid of BlockBackend.ThrottleGroupMember completely.
What is the status of Manos' series?
It would be good to merge it and then consider currently open throttling
bugs again.
I have been busy with schoolwork the past weeks, but I'm close to
finishing some changes Kevin recommended for this series. I hope they
will be ready soon.
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v13 2/6] qmp: Use ThrottleLimits structure
On Fri, Oct 13, 2017 at 09:26:17AM -0500, Eric Blake wrote:
[adding Markus, and block list]
On 10/13/2017 09:16 AM, Alberto Garcia wrote:
On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote:
This patch factors out code to use the ThrottleLimits
structure.
{ 'struct': 'BlockIOThrottle',
- 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
-'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
-'*bps_max': 'int', '*bps_rd_max': 'int',
-'*bps_wr_max': 'int', '*iops_max': 'int',
-'*iops_rd_max': 'int', '*iops_wr_max': 'int',
-'*bps_max_length': 'int', '*bps_rd_max_length': 'int',
-'*bps_wr_max_length': 'int', '*iops_max_length': 'int',
-'*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
-'*iops_size': 'int', '*group': 'str' } }
+ 'base': 'ThrottleLimits',
+ 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } }
So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and
after this patch they become bps-read and iops-write. This breaks the
API completely, as you can see if you run e.g. iotest 129:
AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError',
u'desc': u"Parameter 'iops_rd' is unexpected"}}"
I just checked previous versions of the series and I see that Manos
already warned you of this in v11:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html
On the bright side, ThrottleLimits is marked 'since 2.11' (added in
commit 432d889e), meaning it has not yet been released, so we CAN fix
the naming in ThrottleLimits to be compatible with BlockIOThrottle if we
want to share the type, as long as we get it done before the 2.11
release.
We decided to keep BlockIOThrottle separate from ThrottleLimits because
that would break the old I/O throttling API, just like is done in this
patch series. BlockIOThrottle is the one using old naming conventions
so I think it should be the one to go, if that has to be done.
But this all boils down to whether the legacy throttling API has to
break in 2.11 or not, which probably is the maintainer's decision.
It does mean tweaking Manos' code to use compatible names
everywhere, but that may be a wise course of action (we tend to favor
'-' in new API names unless there is a strong reason to use '_'; but
sharing code for maximum back-compat would be a reason to use '_').
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command
On Fri, Oct 06, 2017 at 02:59:55PM +0200, Max Reitz wrote:
On 2017-10-04 23:04, Manos Pitsidianakis wrote:
On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:
On 2017-10-04 19:05, Manos Pitsidianakis wrote:
On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
On 2017-08-15 09:45, Manos Pitsidianakis wrote:
block-insert-node and its pair command block-remove-node provide
runtime
insertion and removal of filter nodes.
block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and
creates
a new (parent, node) child with the same BdrvChildRole.
This is a different approach than x-blockdev-change which uses the
driver
methods bdrv_add_child() and bdrv_del_child(),
Why? :-)
Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.
I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that. If it doesn't, then just attach the child.
Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached). Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands. In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver. (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
Max
I think the two use cases are this:
a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.
For me both are the same: Adding/removing nodes into/from the graph.
The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children. Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).
Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Not really, because it doesn't add a new child. But it's still a good
point, because your block-insert-node command would make it obsolete,
actually.
blockdev-snapshot literally is a special-cased block-insert-node. And
blockdev-snapshot-sync then is qemu-img create + blockdev-add +
blockdev-snapshot.
Now that we're on this topic, quorum might be a good candidate for
*_reopen when and if it lands on QMP: Reconfiguring the children could
be done by reopening the BDS with new options.
Hmmm... I guess that would work, too. But intuitively, that seems a
bit heavy-weight to me
In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.
(I guess writing too much trying to get my point across, sorry...)
The gist is that for me it's not so much about "quorum" or "filter
nodes". It's about adding a new child to a node vs. replacing an
existing one.
These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?
Yes, I think it would be. :-)
So say we have these two trees in our graph:
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
[ Parent BDS -> Child BDS ]
is split into two trees
[ Parent BDS ] and
[ Child BDS ]
- Add a child, so we can merge
[ Parent BDS ] and
[ Filter BDS -> Child BDS ]
into
[ Parent BDS -> Filter BDS -> Child BDS ]
Yes, of course this would have to be done in one transaction.
Would it? If you want to put Filter BDS into the chain, you can just
create [ Filter BDS ] without any child, and then use a single
x-blockdev-change invocation to inject it.
The only thing that's necessary is that the filter BDS would have to be
able to handle having no child.
Yeah that was what I had in mind.
[...]
So after I've written all of this, I feel like the new insert-node and
remove-node commands are exactly what x-blockdev-change should do when
asked to replace a node. The only difference is that x-blockdev-change
would allow you to replace any node with anything, without the
constraints that block-insert-node and block-remove-node exact.
(And node replacement with x-blockdev-change would work by specifying
all three parameters.)
Not sure if that makes sense, I hope it does. :-)
Hm, I can't think of a way to fit that into x-blockdev-change *and* keep
the bdrv_add_child/bdrv_del_child functionality
Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command
On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:
On 2017-10-04 19:05, Manos Pitsidianakis wrote:
On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
On 2017-08-15 09:45, Manos Pitsidianakis wrote:
block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.
block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.
This is a different approach than x-blockdev-change which uses the
driver
methods bdrv_add_child() and bdrv_del_child(),
Why? :-)
Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.
I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that. If it doesn't, then just attach the child.
Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached). Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands. In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver. (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
Max
I think the two use cases are this:
a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.
For me both are the same: Adding/removing nodes into/from the graph.
The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children. Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).
Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Now that we're on this topic, quorum might be a good candidate for
*_reopen when and if it lands on QMP: Reconfiguring the children could
be done by reopening the BDS with new options.
In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.
(I guess writing too much trying to get my point across, sorry...)
The gist is that for me it's not so much about "quorum" or "filter
nodes". It's about adding a new child to a node vs. replacing an
existing one.
These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?
Yes, I think it would be. :-)
So say we have these two trees in our graph:
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
[ Parent BDS -> Child BDS ]
is split into two trees
[ Parent BDS ] and
[ Child BDS ]
- Add a child, so we can merge
[ Parent BDS ] and
[ Filter BDS -> Child BDS ]
into
[ Parent BDS -> Filter BDS -> Child BDS ]
Yes, of course this would have to be done in one transaction.
However, this is only possible with quorum because usually block drivers
don't support detaching children.
And here's what you can do with your commands (from what I can see):
- Replace a child (you call it insertion, but it really is just
replacement of an existing child with the constraint that both nodes
involved must have the same child):
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
to
[ Parent BDS -> Filter BDS -> Child BDS ]
- Replace a child (you call it removal, but it really is just
replacement of a child with its child):
[ Parent BDS -> Filter BDS -> Child BDS ]
to
[ Parent BDS -> Child BDS ]
This works on all BDSs because you don't change the number of children.
The interesting thing of course is that the "change" command can
actually add and remove children; where as the "insert" and "remove"
commands can only replace children. So that's already a bit funny (one
command does two things; two commands do one thing).
That is true, but the replacing is more in terms of inserting and
removing a node in a BDS chain.
And then of course you can simply modify x-blockdev-change so it can do
the same thing block-insert-node and block-remove-node can do: It just
needs another mode which can be used to replace a child (and its
description already states that it is supposed to be usable for that at
some point in the future).
So after I'
Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command
On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
On 2017-08-15 09:45, Manos Pitsidianakis wrote:
block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.
block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and
creates
a new (parent, node) child with the same BdrvChildRole.
This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),
Why? :-)
Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.
I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that. If it doesn't, then just attach the child.
Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached). Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands. In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver. (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
Max
I think the two use cases are this:
a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.
These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command
On Fri, Sep 29, 2017 at 07:52:35PM +0200, Kevin Wolf wrote:
Am 15.08.2017 um 09:45 hat Manos Pitsidianakis geschrieben:
block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.
block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.
This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4d6ba1baef..16e19cb648 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3947,3 +3947,63 @@
'data' : { 'parent': 'str',
'*child': 'str',
'*node': 'str' } }
+
+##
+# @block-insert-node:
+#
+# Insert a filter node between a specific edge in the block driver state graph.
+# @parent: the name of the parent node or device
+# @node:the name of the node to insert under parent
+# @child: the name of the child of both node and parent
+#
+# Example:
+# Insert and remove a throttle filter on top of a device chain, between the
+# device 'ide0-hd0' and node 'node-A'
+#
+# -> {'execute': 'object-add',
+# "arguments": {
+# "qom-type": "throttle-group",
+# "id": "group0",
+# "props" : { "limits": { "iops-total": 300 } }
+# }
+#}
+# <- { 'return': {} }
+# -> {'execute': 'blockdev-add',
+# 'arguments': {
+# 'driver': 'throttle',
+# 'node-name': 'throttle0',
+# 'throttle-group': 'group0',
+# 'file': 'node-A'
+# }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-insert-node',
+# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node':
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-remove-node',
+# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node':
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'blockdev-del',
+# 'arguments': { 'node-name': 'throttle0' }
+#}
+# <- { 'return': {} }
+#
+##
+{ 'command': 'block-insert-node',
+ 'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }
I would suggest a change to the meaning of @child: Instead of using the
node-name of the child BDS, I would use the name of the BdrvChild that
represents the link.
The reason for this is that the node-name could be ambiguous, if you
have two edges between the same two nodes.
The only use of the node-name of the child that I can remember was for
checking that the graph still looks like what the user expects. But I
think we came to the conclusion that there are no race conditions to
check for if we have manual block job deletion instead of automatic
completion which can involve surprise changes to the graph. So probably
we don't need the node-name even for this.
+##
+# @block-remove-node:
+#
+# Remove a filter node between two other nodes in the block driver state graph.
+# @parent: the name of the parent node or device
+# @node:the name of the node to remove from parent
+# @child: the name of the child of node which will go under parent
+##
+{ 'command': 'block-remove-node',
+ 'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }
Same thing here.
diff --git a/block.c b/block.c
index 81bd51b670..f874aabbfb 100644
--- a/block.c
+++ b/block.c
+/* insert 'node' as child bs of 'parent' node */
+if (check_node_edge(parent, child, errp)) {
+return;
+}
+parent_bs = bdrv_find_node(parent);
+c = bdrv_find_child(parent_bs, child);
+role = c->role;
+assert(role == _file || role == _backing);
+
+bdrv_ref(node_bs);
+
+bdrv_drained_begin(parent_bs);
+bdrv_unref_child(parent_bs, c);
+if (role == _file) {
+parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
+_file, errp);
+if (!parent_bs->file) {
+parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
+_file, _abort);
+goto out;
+}
+} else if (role == _backing) {
+parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
+ _backing, errp);
+if (!parent_bs->backing) {
+parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
+ "backing", _backing,
+ _abort);
+goto out;
+}
+}
I would prefer if we could find a solution to avoid requiring a specifi
Re: [Qemu-block] [PATCH v3 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks
On Tue, Sep 26, 2017 at 12:00:24PM +0100, Stefan Hajnoczi wrote:
On Sat, Sep 23, 2017 at 02:14:08PM +0300, Manos Pitsidianakis wrote:
This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new
bdrv_co_drain_end callback to match bdrv_drained_begin/end and
drained_begin/end of BdrvChild. This is needed because the throttle driver
(block/throttle.c) needs a way to mark the end of the drain in order to toggle
io_limits_disabled correctly.
Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c
v3:
fixed commit message typo in first patch [Fam]
rephrased doc comment based on mailing discussion
v2:
add doc for callbacks and change order of request polling for completion
[Stefan]
Manos Pitsidianakis (3):
block: add bdrv_co_drain_end callback
block: rename bdrv_co_drain to bdrv_co_drain_begin
block/throttle.c: add bdrv_co_drain_begin/end callbacks
include/block/block_int.h | 13 ++---
block/io.c| 48 +--
block/qed.c | 6 +++---
block/throttle.c | 18 ++
4 files changed, 65 insertions(+), 20 deletions(-)
Oops, this seems to cause a qemu-iotests failure. Please take a look:
$ ./check -qcow2 184
184 0s ... - output mismatch (see 184.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/184.out 2017-09-19
14:51:46.673854437 +0100
+++ 184.out.bad 2017-09-26 11:13:06.946610239 +0100
@@ -142,6 +142,9 @@
"guest": false
}
}
+./common.config: line 118: 9196 Segmentation fault (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
Hey Stefan,
This is fixed in
Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c
Which I sent before this series and is in Kevin's branch.
signature.asc
Description: PGP signature
[Qemu-block] [PATCH v3 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks
This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new bdrv_co_drain_end callback to match bdrv_drained_begin/end and drained_begin/end of BdrvChild. This is needed because the throttle driver (block/throttle.c) needs a way to mark the end of the drain in order to toggle io_limits_disabled correctly. Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr> "block/throttle-groups.c: allocate RestartData on the heap" Which fixes a coroutine crash in block/throttle-groups.c v3: fixed commit message typo in first patch [Fam] rephrased doc comment based on mailing discussion v2: add doc for callbacks and change order of request polling for completion [Stefan] Manos Pitsidianakis (3): block: add bdrv_co_drain_end callback block: rename bdrv_co_drain to bdrv_co_drain_begin block/throttle.c: add bdrv_co_drain_begin/end callbacks include/block/block_int.h | 13 ++--- block/io.c| 48 +-- block/qed.c | 6 +++--- block/throttle.c | 18 ++ 4 files changed, 65 insertions(+), 20 deletions(-) -- 2.11.0
[Qemu-block] [PATCH v3 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Fam Zheng <f...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
block/throttle.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
}
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+if (atomic_fetch_inc(>io_limits_disabled) == 0) {
+throttle_group_restart_tgm(tgm);
+}
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+assert(tgm->io_limits_disabled);
+atomic_dec(>io_limits_disabled);
+}
+
static BlockDriver bdrv_throttle = {
.format_name= "throttle",
.protocol_name = "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_abort = throttle_reopen_abort,
.bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+.bdrv_co_drain_begin= throttle_co_drain_begin,
+.bdrv_co_drain_end = throttle_co_drain_end,
+
.is_filter = true,
};
--
2.11.0
[Qemu-block] [PATCH v3 1/3] block: add bdrv_co_drain_end callback
BlockDriverState has a bdrv_co_drain() callback but no equivalent for
the end of the drain. The throttle driver (block/throttle.c) needs a way
to mark the end of the drain in order to toggle io_limits_disabled
correctly, thus bdrv_co_drain_end is needed.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 11 +--
block/io.c| 48 +--
2 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..9ebdeb6db0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -354,10 +354,17 @@ struct BlockDriver {
int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
/**
- * Drain and stop any internal sources of requests in the driver, and
- * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+ * bdrv_co_drain is called if implemented in the beginning of a
+ * drain operation to drain and stop any internal sources of requests in
+ * the driver.
+ * bdrv_co_drain_end is called if implemented at the end of the drain.
+ *
+ * They should be used by the driver to e.g. manage scheduled I/O
+ * requests, or toggle an internal state. After the end of the drain new
+ * requests will continue normally.
*/
void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
Coroutine *co;
BlockDriverState *bs;
bool done;
+bool begin;
} BdrvCoDrainData;
static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BdrvCoDrainData *data = opaque;
BlockDriverState *bs = data->bs;
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
/* Set data->done before reading bs->wakeup. */
atomic_mb_set(>done, true);
bdrv_wakeup(bs);
}
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
BDRV_POLL_WHILE(bs, !data.done);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
{
BdrvChild *child, *tmp;
bool waited;
-waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
-
/* Ensure any pending metadata writes are submitted to bs->file. */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
QLIST_FOREACH_SAFE(child, >children, next, tmp) {
BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
*/
bdrv_ref(bs);
}
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
if (in_main_loop) {
bdrv_unref(bs);
}
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
BlockDriverState *bs = data->bs;
bdrv_dec_in_flight(bs);
-bdrv_drained_begin(bs);
+if (data->begin) {
+bdrv_drained_begin(bs);
+} else {
+bdrv_drained_end(bs);
+}
+
data->done = true;
aio_co_wake(co);
}
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+bool begin)
{
BdrvCoDrainData data;
@@ -239,6 +252,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
.co = qemu_coroutine_self(),
.bs = bs,
.done = false,
+.begin = begin,
};
bdrv_inc_in_flight(bs);
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +267,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
void bdrv_drained_begin(BlockDriverState *bs)
{
if (qemu_in_coroutine()) {
-bdrv_co_yield_
[Qemu-block] [PATCH v3 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Fam Zheng <f...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 4 ++--
block/io.c| 4 ++--
block/qed.c | 6 +++---
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ebdeb6db0..51575d22b6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -354,7 +354,7 @@ struct BlockDriver {
int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
/**
- * bdrv_co_drain is called if implemented in the beginning of a
+ * bdrv_co_drain_begin is called if implemented in the beginning of a
* drain operation to drain and stop any internal sources of requests in
* the driver.
* bdrv_co_drain_end is called if implemented at the end of the drain.
@@ -363,7 +363,7 @@ struct BlockDriver {
* requests, or toggle an internal state. After the end of the drain new
* requests will continue normally.
*/
-void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
diff --git a/block/io.c b/block/io.c
index b0a10ad3ef..65e8094d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BlockDriverState *bs = data->bs;
if (data->begin) {
-bs->drv->bdrv_co_drain(bs);
+bs->drv->bdrv_co_drain_begin(bs);
} else {
bs->drv->bdrv_co_drain_end(bs);
}
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool
begin)
{
BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
assert(!s->allocating_write_reqs_plugged);
if (s->allocating_acb != NULL) {
/* Another allocating write came concurrently. This cannot happen
- * from bdrv_qed_co_drain, but it can happen when the timer runs.
+ * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
*/
qemu_co_mutex_unlock(>table_lock);
return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState
*bs,
}
}
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
{
BDRVQEDState *s = bs->opaque;
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
.bdrv_check = bdrv_qed_check,
.bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
.bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
-.bdrv_co_drain= bdrv_qed_co_drain,
+.bdrv_co_drain_begin = bdrv_qed_co_drain_begin,
};
static void bdrv_qed_init(void)
--
2.11.0
Re: [Qemu-block] [PATCH v2 1/3] block: add bdrv_co_drain_end callback
On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
s/bdrv_do_drain/bdrv_co_drain/
of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 6 ++
block/io.c| 48 +--
2 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..21950cfda3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,8 +356,14 @@ struct BlockDriver {
/**
* Drain and stop any internal sources of requests in the driver, and
* remain so until next I/O callback (e.g. bdrv_co_writev) is called.
This line needs update too, maybe:
/**
* bdrv_co_drain drains and stops any ... and remain so until
* bdrv_co_drain_end is called.
+ *
+ * The callbacks are called at the beginning and ending of the drain
+ * respectively. They should be implemented if the driver needs to e.g.
As implied by above change, should we explicitly require "should both be
implemented"? It may not be technically required, but I think is cleaner and
easier to reason about.
It might imply to someone that there's an
assert(drv->bdrv_co_drain_begin && drv->bdrv_co_drain_end) somewhere
unless you state they don't have to be implemented at the same time. How
about we be completely explicit:
bdrv_co_drain_begin is called if implemented in the beggining of a
drain operation to drain and stop any internal sources of requests in
the driver.
bdrv_co_drain_end is called if implemented at the end of the drain.
They should be used by the driver to e.g. manage scheduled I/O
requests, or toggle an internal state. After the end of the drain new
requests will continue normally.
I hope this is easier for a reader to understand!
+ * manage scheduled I/O requests, or toggle an internal state.
After an
s/After an/After a/
+ * bdrv_co_drain_end() invocation new requests will continue normally.
*/
void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
Coroutine *co;
BlockDriverState *bs;
bool done;
+bool begin;
} BdrvCoDrainData;
static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BdrvCoDrainData *data = opaque;
BlockDriverState *bs = data->bs;
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
/* Set data->done before reading bs->wakeup. */
atomic_mb_set(>done, true);
bdrv_wakeup(bs);
}
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
BDRV_POLL_WHILE(bs, !data.done);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
{
BdrvChild *child, *tmp;
bool waited;
-waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
-
/* Ensure any pending metadata writes are submitted to bs->file. */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
QLIST_FOREACH_SAFE(child, >children, next, tmp) {
BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
*/
bdrv_ref(bs);
}
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
if (in_main_loop) {
bdrv_unref(bs);
}
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
BlockDriverState *bs =
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks
On Thu, Sep 21, 2017 at 09:35:35PM +0800, Fam Zheng wrote: On Thu, 09/21 16:17, Manos Pitsidianakis wrote: This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new bdrv_co_drain_end callback to match bdrv_drained_begin/end and drained_begin/end of BdrvChild. This is needed because the throttle driver (block/throttle.c) needs a way to mark the end of the drain in order to toggle io_limits_disabled correctly. Is this a bug fix? I.e. do we need to Cc qemu-stable@? Fam No that's not needed, throttle is not in 2.10. The bug in this case is that the drain would wait for the throttled requests to be completed as they were scheduled by the I/O throttling instead of restarting the queue and scheduling them right away. signature.asc Description: PGP signature
[Qemu-block] [PATCH v2 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 2 +-
block/io.c| 4 ++--
block/qed.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 21950cfda3..205d088271 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -362,7 +362,7 @@ struct BlockDriver {
* manage scheduled I/O requests, or toggle an internal state. After an
* bdrv_co_drain_end() invocation new requests will continue normally.
*/
-void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
diff --git a/block/io.c b/block/io.c
index b0a10ad3ef..65e8094d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BlockDriverState *bs = data->bs;
if (data->begin) {
-bs->drv->bdrv_co_drain(bs);
+bs->drv->bdrv_co_drain_begin(bs);
} else {
bs->drv->bdrv_co_drain_end(bs);
}
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool
begin)
{
BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
assert(!s->allocating_write_reqs_plugged);
if (s->allocating_acb != NULL) {
/* Another allocating write came concurrently. This cannot happen
- * from bdrv_qed_co_drain, but it can happen when the timer runs.
+ * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
*/
qemu_co_mutex_unlock(>table_lock);
return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState
*bs,
}
}
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
{
BDRVQEDState *s = bs->opaque;
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
.bdrv_check = bdrv_qed_check,
.bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
.bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
-.bdrv_co_drain= bdrv_qed_co_drain,
+.bdrv_co_drain_begin = bdrv_qed_co_drain_begin,
};
static void bdrv_qed_init(void)
--
2.11.0
[Qemu-block] [PATCH v2 1/3] block: add bdrv_co_drain_end callback
BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 6 ++
block/io.c| 48 +--
2 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..21950cfda3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,8 +356,14 @@ struct BlockDriver {
/**
* Drain and stop any internal sources of requests in the driver, and
* remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+ *
+ * The callbacks are called at the beginning and ending of the drain
+ * respectively. They should be implemented if the driver needs to e.g.
+ * manage scheduled I/O requests, or toggle an internal state. After an
+ * bdrv_co_drain_end() invocation new requests will continue normally.
*/
void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
Coroutine *co;
BlockDriverState *bs;
bool done;
+bool begin;
} BdrvCoDrainData;
static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BdrvCoDrainData *data = opaque;
BlockDriverState *bs = data->bs;
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
/* Set data->done before reading bs->wakeup. */
atomic_mb_set(>done, true);
bdrv_wakeup(bs);
}
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
BDRV_POLL_WHILE(bs, !data.done);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
{
BdrvChild *child, *tmp;
bool waited;
-waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
-
/* Ensure any pending metadata writes are submitted to bs->file. */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
QLIST_FOREACH_SAFE(child, >children, next, tmp) {
BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
*/
bdrv_ref(bs);
}
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
if (in_main_loop) {
bdrv_unref(bs);
}
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
BlockDriverState *bs = data->bs;
bdrv_dec_in_flight(bs);
-bdrv_drained_begin(bs);
+if (data->begin) {
+bdrv_drained_begin(bs);
+} else {
+bdrv_drained_end(bs);
+}
+
data->done = true;
aio_co_wake(co);
}
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+bool begin)
{
BdrvCoDrainData data;
@@ -239,6 +252,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
.co = qemu_coroutine_self(),
.bs = bs,
.done = false,
+.begin = begin,
};
bdrv_inc_in_flight(bs);
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +267,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
void bdrv_drained_begin(BlockDriverState *bs)
{
if (qemu_in_coroutine()) {
-bdrv_co_yield_to_drain(bs);
+bdrv_co_yield_to_drain(bs, true);
return;
}
@@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
bdrv_parent_drained_begin(bs);
}
-
[Qemu-block] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
block/throttle.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
}
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+if (atomic_fetch_inc(>io_limits_disabled) == 0) {
+throttle_group_restart_tgm(tgm);
+}
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+assert(tgm->io_limits_disabled);
+atomic_dec(>io_limits_disabled);
+}
+
static BlockDriver bdrv_throttle = {
.format_name= "throttle",
.protocol_name = "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_abort = throttle_reopen_abort,
.bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+.bdrv_co_drain_begin= throttle_co_drain_begin,
+.bdrv_co_drain_end = throttle_co_drain_end,
+
.is_filter = true,
};
--
2.11.0
[Qemu-block] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks
This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new bdrv_co_drain_end callback to match bdrv_drained_begin/end and drained_begin/end of BdrvChild. This is needed because the throttle driver (block/throttle.c) needs a way to mark the end of the drain in order to toggle io_limits_disabled correctly. Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr> "block/throttle-groups.c: allocate RestartData on the heap" Which fixes a coroutine crash in block/throttle-groups.c v2: add doc for callbacks and change order of request polling for completion [Stefan] Manos Pitsidianakis (3): block: add bdrv_co_drain_end callback block: rename bdrv_co_drain to bdrv_co_drain_begin block/throttle.c: add bdrv_co_drain_begin/end callbacks include/block/block_int.h | 8 +++- block/io.c| 48 +-- block/qed.c | 6 +++--- block/throttle.c | 18 ++ 4 files changed, 62 insertions(+), 18 deletions(-) -- 2.11.0
Re: [Qemu-block] [PATCH 1/3] block: add bdrv_co_drain_end callback
On Wed, Sep 20, 2017 at 03:26:32PM +0100, Stefan Hajnoczi wrote: On Wed, Sep 20, 2017 at 01:23:09PM +0300, Manos Pitsidianakis wrote: @@ -188,7 +194,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs) waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0); /* Ensure any pending metadata writes are submitted to bs->file. */ -bdrv_drain_invoke(bs); +bdrv_drain_invoke(bs, begin); Do you need to move bdrv_drain_invoke(bs, begin) before BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0)? This will ensure that throttling is disabled and the TGM restarted before we wait for requests to complete. Hm yes. Before, the order was irrelevant because BlockBackend issued the drain first by restarting the tgm in blk_root_drained_begin. signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH 1/2] block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite
On Wed, Sep 20, 2017 at 02:24:21PM +0200, Kevin Wolf wrote: Am 20.09.2017 um 13:43 hat Manos Pitsidianakis geschrieben: blk_check_byte_request() is called from the blk_co_pwritev/blk_co_preadv to check if the request offset and request bytes parameters are valid for the given Blockbackend. Let's do that in blk_pread/blk_pwrite too. Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> I don't think this is necessary, blk_pread/pwrite() are only wrappers around blk_co_preadv/pwritev(), so we're already checking the parameters. Kevin Alright, this can be dropped then. Thanks! signature.asc Description: PGP signature
[Qemu-block] [PATCH 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 2 +-
block/io.c| 4 ++--
block/qed.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ea1326e3c7..fd10f53b43 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -357,7 +357,7 @@ struct BlockDriver {
* Drain and stop any internal sources of requests in the driver, and
* remain so until next I/O callback (e.g. bdrv_co_writev) is called.
*/
-void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
diff --git a/block/io.c b/block/io.c
index 465345289d..6b4b13b777 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BlockDriverState *bs = data->bs;
if (data->begin) {
-bs->drv->bdrv_co_drain(bs);
+bs->drv->bdrv_co_drain_begin(bs);
} else {
bs->drv->bdrv_co_drain_end(bs);
}
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool
begin)
{
BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
assert(!s->allocating_write_reqs_plugged);
if (s->allocating_acb != NULL) {
/* Another allocating write came concurrently. This cannot happen
- * from bdrv_qed_co_drain, but it can happen when the timer runs.
+ * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
*/
qemu_co_mutex_unlock(>table_lock);
return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState
*bs,
}
}
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
{
BDRVQEDState *s = bs->opaque;
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
.bdrv_check = bdrv_qed_check,
.bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
.bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
-.bdrv_co_drain= bdrv_qed_co_drain,
+.bdrv_co_drain_begin = bdrv_qed_co_drain_begin,
};
static void bdrv_qed_init(void)
--
2.11.0
Re: [Qemu-block] [PATCH v2] throttle-groups: update tg->any_timer_armed[] on detach
On Wed, Sep 20, 2017 at 11:17:40AM +0100, Stefan Hajnoczi wrote:
Clear tg->any_timer_armed[] when throttling timers are destroyed during
AioContext attach/detach. Failure to do so causes throttling to hang
because we believe the timer is already scheduled!
The following was broken at least since QEMU 2.10.0 with -drive
iops=100:
$ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000
(qemu) stop
(qemu) cont
...I/O is stuck...
Reported-by: Yongxue Hong <yh...@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
v2:
* Use timer_pending(tt->timers[i]) instead of token [Berto]
* Fix s/destroy/destroyed/ typo [Eric]
block/throttle-groups.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 6ba992c8d7..0a49f3c409 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -592,6 +592,17 @@ void throttle_group_attach_aio_context(ThrottleGroupMember
*tgm,
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
{
ThrottleTimers *tt = >throttle_timers;
+ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
+
+qemu_mutex_lock(>lock);
+if (timer_pending(tt->timers[0])) {
+tg->any_timer_armed[0] = false;
+}
+if (timer_pending(tt->timers[1])) {
+tg->any_timer_armed[1] = false;
+}
+qemu_mutex_unlock(>lock);
+
throttle_timers_detach_aio_context(tt);
tgm->aio_context = NULL;
}
--
2.13.5
signature.asc
Description: PGP signature
[Qemu-block] [PATCH 1/2] block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite
blk_check_byte_request() is called from the blk_co_pwritev/blk_co_preadv to
check if the request offset and request bytes parameters are valid for the
given Blockbackend. Let's do that in blk_pread/blk_pwrite too.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
block/block-backend.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 45d9101be3..110a52d5b1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1288,22 +1288,23 @@ BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk,
int64_t offset,
int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
{
-int ret = blk_prw(blk, offset, buf, count, blk_read_entry, 0);
+int ret = blk_check_byte_request(blk, offset, count);
if (ret < 0) {
return ret;
}
-return count;
+ret = blk_prw(blk, offset, buf, count, blk_read_entry, 0);
+return ret < 0 ? ret : count;
}
int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
BdrvRequestFlags flags)
{
-int ret = blk_prw(blk, offset, (void *) buf, count, blk_write_entry,
- flags);
+int ret = blk_check_byte_request(blk, offset, count);
if (ret < 0) {
return ret;
}
-return count;
+ret = blk_prw(blk, offset, (void *) buf, count, blk_write_entry, flags);
+return ret < 0 ? ret : count;
}
int64_t blk_getlength(BlockBackend *blk)
--
2.11.0
[Qemu-block] [PATCH 1/3] block: add bdrv_co_drain_end callback
BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 2 ++
block/io.c| 43 +++
2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..ea1326e3c7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -359,6 +359,8 @@ struct BlockDriver {
*/
void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
+
void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..465345289d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
Coroutine *co;
BlockDriverState *bs;
bool done;
+bool begin;
} BdrvCoDrainData;
static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void
*opaque)
BdrvCoDrainData *data = opaque;
BlockDriverState *bs = data->bs;
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
/* Set data->done before reading bs->wakeup. */
atomic_mb_set(>done, true);
bdrv_wakeup(bs);
}
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
@@ -180,7 +186,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
BDRV_POLL_WHILE(bs, !data.done);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
{
BdrvChild *child, *tmp;
bool waited;
@@ -188,7 +194,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
waited = BDRV_POLL_WHILE(bs, atomic_read(>in_flight) > 0);
/* Ensure any pending metadata writes are submitted to bs->file. */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
QLIST_FOREACH_SAFE(child, >children, next, tmp) {
BlockDriverState *bs = child->bs;
@@ -205,7 +211,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
*/
bdrv_ref(bs);
}
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
if (in_main_loop) {
bdrv_unref(bs);
}
@@ -221,12 +227,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
BlockDriverState *bs = data->bs;
bdrv_dec_in_flight(bs);
-bdrv_drained_begin(bs);
+if (data->begin) {
+bdrv_drained_begin(bs);
+} else {
+bdrv_drained_end(bs);
+}
+
data->done = true;
aio_co_wake(co);
}
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+bool begin)
{
BdrvCoDrainData data;
@@ -239,6 +251,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
.co = qemu_coroutine_self(),
.bs = bs,
.done = false,
+.begin = begin,
};
bdrv_inc_in_flight(bs);
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +266,7 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
void bdrv_drained_begin(BlockDriverState *bs)
{
if (qemu_in_coroutine()) {
-bdrv_co_yield_to_drain(bs);
+bdrv_co_yield_to_drain(bs, true);
return;
}
@@ -262,17 +275,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
bdrv_parent_drained_begin(bs);
}
-bdrv_drain_recurse(bs);
+bdrv_drain_recurse(bs, true);
}
void bdrv_drained_end(BlockDriverState *bs)
{
+if (qemu_in_coroutine()) {
+bdrv_co_yield_to_drain(bs, false);
+return;
+}
assert(bs->quiesce_counter > 0);
if (atomic_fetch_dec(>quiesce_counter) > 1) {
return;
}
bdrv_parent_drained_end(bs);
+bdrv_drain_recurse(bs, false);
aio_enable_external(bdrv_get_aio_context(bs));
}
@@ -
[Qemu-block] [PATCH 0/2] remove blk_pread_unthrottled()
Cleanups for minor stuff I noticed while looking around blk_root_drained_* Manos Pitsidianakis (2): block/block-backend.c: add blk_check_byte_request call to blk_pread/blk_pwrite block/block-backend.c: remove blk_pread_unthrottled() include/sysemu/block-backend.h | 2 -- block/block-backend.c | 27 ++- hw/block/hd-geometry.c | 7 +-- 3 files changed, 7 insertions(+), 29 deletions(-) -- 2.11.0
[Qemu-block] [PATCH 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
block/throttle.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
}
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+if (atomic_fetch_inc(>io_limits_disabled) == 0) {
+throttle_group_restart_tgm(tgm);
+}
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+assert(tgm->io_limits_disabled);
+atomic_dec(>io_limits_disabled);
+}
+
static BlockDriver bdrv_throttle = {
.format_name= "throttle",
.protocol_name = "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_abort = throttle_reopen_abort,
.bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
+.bdrv_co_drain_begin= throttle_co_drain_begin,
+.bdrv_co_drain_end = throttle_co_drain_end,
+
.is_filter = true,
};
--
2.11.0
Re: [Qemu-block] [PATCH v2] throttle-groups: update tg->any_timer_armed[] on detach
On Wed, Sep 20, 2017 at 01:08:52PM +0200, Alberto Garcia wrote:
On Wed 20 Sep 2017 12:17:40 PM CEST, Stefan Hajnoczi wrote:
@@ -592,6 +592,17 @@ void throttle_group_attach_aio_context(ThrottleGroupMember
*tgm,
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)
{
ThrottleTimers *tt = >throttle_timers;
+ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
+
+qemu_mutex_lock(>lock);
+if (timer_pending(tt->timers[0])) {
+tg->any_timer_armed[0] = false;
+}
+if (timer_pending(tt->timers[1])) {
+tg->any_timer_armed[1] = false;
+}
+qemu_mutex_unlock(>lock);
+
throttle_timers_detach_aio_context(tt);
tgm->aio_context = NULL;
}
I'm sorry that I didn't noticed this in my previous e-mail, but after
this call you might have destroyed the timer that was set for that
throttling group, so if there are pending requests waiting it can happen
that no one wakes them up.
I think that the queue needs to be restarted after this, probably after
having reattached the context (or actually after detaching it already,
but then what happens if you try to restart the queue while aio_context
is NULL?).
aio_co_enter in the restart queue function requires that aio_context is
non-NULL. Perhaps calling throttle_group_restart_tgm in
throttle_group_attach_aio_context will suffice.
signature.asc
Description: PGP signature
[Qemu-block] [PATCH 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks
This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new bdrv_co_drain_end callback to match bdrv_drained_begin/end and drained_begin/end of BdrvChild. This is needed because the throttle driver (block/throttle.c) needs a way to mark the end of the drain in order to toggle io_limits_disabled correctly. Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr> "block/throttle-groups.c: allocate RestartData on the heap" Which fixes a coroutine crash in block/throttle-groups.c Manos Pitsidianakis (3): block: add bdrv_co_drain_end callback block: rename bdrv_co_drain to bdrv_co_drain_begin block/throttle.c: add bdrv_co_drain_begin/end callbacks include/block/block_int.h | 4 +++- block/io.c| 42 ++ block/qed.c | 6 +++--- block/throttle.c | 18 ++ 4 files changed, 54 insertions(+), 16 deletions(-) -- 2.11.0
[Qemu-block] [PATCH 2/2] block/block-backend.c: remove blk_pread_unthrottled()
blk_pread_unthrottled was used to bypass I/O throttling on the BlockBackend in
the case of async I/O. This is not needed anymore and we can just call
blk_pread() directly.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/sysemu/block-backend.h | 2 --
block/block-backend.c | 16
hw/block/hd-geometry.c | 7 +--
3 files changed, 1 insertion(+), 24 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c4e52a5fa3..c881bfdfe4 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -117,8 +117,6 @@ char *blk_get_attached_dev_id(BlockBackend *blk);
BlockBackend *blk_by_dev(void *dev);
BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
- int bytes);
int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
diff --git a/block/block-backend.c b/block/block-backend.c
index 110a52d5b1..83329fce15 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1145,22 +1145,6 @@ static int blk_prw(BlockBackend *blk, int64_t offset,
uint8_t *buf,
return rwco.ret;
}
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
- int count)
-{
-int ret;
-
-ret = blk_check_byte_request(blk, offset, count);
-if (ret < 0) {
-return ret;
-}
-
-blk_root_drained_begin(blk->root);
-ret = blk_pread(blk, offset, buf, count);
-blk_root_drained_end(blk->root);
-return ret;
-}
-
int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 57ad5012a7..211307f73f 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -62,12 +62,7 @@ static int guess_disk_lchs(BlockBackend *blk,
blk_get_geometry(blk, _sectors);
-/**
- * The function will be invoked during startup not only in sync I/O mode,
- * but also in async I/O mode. So the I/O throttling function has to
- * be disabled temporarily here, not permanently.
- */
-if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
+if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
return -1;
}
/* test msdos magic */
--
2.11.0
[Qemu-block] [PATCH] block/throttle-groups.c: allocate RestartData on the heap
RestartData is the opaque data of the throttle_group_restart_queue_entry
coroutine. By being stack allocated, it isn't available anymore if
aio_co_enter schedules the coroutine with a bottom halve and runs after
throttle_group_restart_queue returns.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
block/throttle-groups.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 6ba992c8d7..b291a88481 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -403,17 +403,19 @@ static void coroutine_fn
throttle_group_restart_queue_entry(void *opaque)
schedule_next_request(tgm, is_write);
qemu_mutex_unlock(>lock);
}
+
+g_free(data);
}
static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool
is_write)
{
Coroutine *co;
-RestartData rd = {
-.tgm = tgm,
-.is_write = is_write
-};
+RestartData *rd = g_new0(RestartData, 1);
-co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
+rd->tgm = tgm;
+rd->is_write = is_write;
+
+co = qemu_coroutine_create(throttle_group_restart_queue_entry, rd);
aio_co_enter(tgm->aio_context, co);
}
--
2.11.0
Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling
On Fri, Sep 08, 2017 at 06:00:11PM +0200, Kevin Wolf wrote: Am 08.09.2017 um 17:44 hat Manos Pitsidianakis geschrieben: On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote: > We shouldn't really need any throttling code in > blk_root_drained_begin/end any more now because the throttle node will > be drained. If this code is necessary, a bdrv_drain() on an explicit > throttle node will work differently from one on an implicit one. > > Unfortunately, this seems to be true about the throttle node. Implicit > throttle nodes will keep ignoring the throttle limit in order to > complete the drain request quickly, where as explicit throttle nodes > will process their requests at the configured speed before the drain > request can be completed. > > This doesn't feel right to me, both should behave the same. > > Kevin > I suppose we can implement bdrv_co_drain and increase io_limits_disabled from inside the driver. And then remove the implicit filter logic from blk_root_drained_begin. But there's no _end callback equivalent so we can't decrease io_limits_disabled at the end of the drain. So I think there are two options: - make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all children to call it. Old behavior of I/O bursts (?) during a drain is kept. This is the solution I was thinking of. It was always odd to have a drained_begin/end pair in the external interface and in BdrvChildRole, but not in BlockDriver. So it was to be expected that we'd need this sooner or later. - remove io_limits_disabled and let throttled requests obey limits during a drain This was discussed earlier (at least when the disable code was introduced in BlockBackend, but I think actually more than once), and even though everyone agreed that ignoring the limits is ugly, we seem to have come to the conclusion that it's the least bad option. blk_drain() blocks and makes everything else hang, so we don't want it to wait for several seconds. Kevin That makes sense. I will look into this and resubmit the series with this additional change. signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling
On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote: We shouldn't really need any throttling code in blk_root_drained_begin/end any more now because the throttle node will be drained. If this code is necessary, a bdrv_drain() on an explicit throttle node will work differently from one on an implicit one. Unfortunately, this seems to be true about the throttle node. Implicit throttle nodes will keep ignoring the throttle limit in order to complete the drain request quickly, where as explicit throttle nodes will process their requests at the configured speed before the drain request can be completed. This doesn't feel right to me, both should behave the same. Kevin I suppose we can implement bdrv_co_drain and increase io_limits_disabled from inside the driver. And then remove the implicit filter logic from blk_root_drained_begin. But there's no _end callback equivalent so we can't decrease io_limits_disabled at the end of the drain. So I think there are two options: - make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all children to call it. Old behavior of I/O bursts (?) during a drain is kept. - remove io_limits_disabled and let throttled requests obey limits during a drain signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v9 6/6] qemu-iotests: add 184 for throttle filter driver
On Tue, Sep 05, 2017 at 04:13:39PM -0500, Eric Blake wrote:
On 09/05/2017 02:06 PM, Kevin Wolf wrote:
Am 05.09.2017 um 18:16 hat Kevin Wolf geschrieben:
Am 25.08.2017 um 15:20 hat Manos Pitsidianakis geschrieben:
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
Does this test actually (still) pass for you? I can't see that it's
related to any recent change in master, but this is the diff that I get.
I can update the reference output while applying, but obviously if it's
currently passing for you, it will fail after I "fix" it.
For the record, we discussed this on IRC. The test works correctly on
master, but on my block branch there is a conflict with "block: pass
bdrv_* methods to bs->file by default in block filters".
The correct action is to merge this throttle driver series after the
conflicting patch because throttle doesn't implement .bdrv_get_info and
needs the forwarding that the other patch implements.
I updated the test output accordingly and applied the series to my block
branch.
Could you also squash this in to 5/6? (as long as we're intentionally
basing throttle on top of defaults, then we should use the right default
instead of duplicating things)
Yes, this change makes sense, if it's no trouble with Kevin.
diff --git i/block/throttle.c w/block/throttle.c
index 7b33613372..5bca76300f 100644
--- i/block/throttle.c
+++ w/block/throttle.c
@@ -197,19 +197,6 @@ static bool
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
}
-static int64_t coroutine_fn
throttle_co_get_block_status(BlockDriverState *bs,
- int64_t
sector_num,
- int nb_sectors,
- int *pnum,
-
BlockDriverState **file)
-{
-assert(bs->file && bs->file->bs);
-*pnum = nb_sectors;
-*file = bs->file->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
- (sector_num << BDRV_SECTOR_BITS);
-}
-
static BlockDriver bdrv_throttle = {
.format_name= "throttle",
.protocol_name = "throttle",
@@ -237,7 +224,7 @@ static BlockDriver bdrv_throttle = {
.bdrv_reopen_prepare= throttle_reopen_prepare,
.bdrv_reopen_commit = throttle_reopen_commit,
.bdrv_reopen_abort = throttle_reopen_abort,
-.bdrv_co_get_block_status = throttle_co_get_block_status,
+.bdrv_co_get_block_status =
bdrv_co_get_block_status_from_file,
.is_filter = true,
};
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: PGP signature
[Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling
This commit removes all I/O throttling from block/block-backend.c. In
order to support the existing interface, it is changed to use the
block/throttle.c filter driver.
The throttle filter node that is created by the legacy interface is
stored in a 'throttle_node' field in the BlockBackendPublic of the
device. The legacy throttle node is managed by the legacy interface
completely. More advanced configurations with the filter drive are
possible using the QMP API, but these will be ignored by the legacy
interface.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 1 +
include/sysemu/block-backend.h | 6 +-
block/block-backend.c | 134 +---
block/qapi.c| 10 +--
block/throttle.c| 8 +++
blockdev.c | 37 ---
tests/test-throttle.c | 19 +++---
7 files changed, 140 insertions(+), 75 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index e2fd0513c4..8493540766 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -81,5 +81,6 @@ void throttle_group_detach_aio_context(ThrottleGroupMember
*tgm);
* mutex.
*/
bool throttle_group_exists(const char *name);
+ThrottleGroupMember *throttle_get_tgm(BlockDriverState *bs);
#endif
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 0e0cda7521..4a7ca53685 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -73,7 +73,7 @@ typedef struct BlockDevOps {
* friends so that BlockBackends can be kept in lists outside block-backend.c
* */
typedef struct BlockBackendPublic {
-ThrottleGroupMember throttle_group_member;
+BlockDriverState *throttle_node;
} BlockBackendPublic;
BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
@@ -225,7 +225,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
void blk_io_limits_disable(BlockBackend *blk);
-void blk_io_limits_enable(BlockBackend *blk, const char *group);
-void blk_io_limits_update_group(BlockBackend *blk, const char *group);
+void blk_io_limits_enable(BlockBackend *blk, const char *group, Error **errp);
+void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error
**errp);
#endif
diff --git a/block/block-backend.c b/block/block-backend.c
index c51fb8c8aa..693ad27fc9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -15,6 +15,7 @@
#include "block/block_int.h"
#include "block/blockjob.h"
#include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
#include "sysemu/blockdev.h"
#include "sysemu/sysemu.h"
#include "qapi-event.h"
@@ -319,7 +320,7 @@ static void blk_delete(BlockBackend *blk)
assert(!blk->refcnt);
assert(!blk->name);
assert(!blk->dev);
-if (blk->public.throttle_group_member.throttle_state) {
+if (blk->public.throttle_node) {
blk_io_limits_disable(blk);
}
if (blk->root) {
@@ -634,13 +635,7 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
*/
void blk_remove_bs(BlockBackend *blk)
{
-ThrottleTimers *tt;
-
notifier_list_notify(>remove_bs_notifiers, blk);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
-}
blk_update_root_state(blk);
@@ -661,12 +656,6 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs,
Error **errp)
bdrv_ref(bs);
notifier_list_notify(>insert_bs_notifiers, blk);
-if (blk->public.throttle_group_member.throttle_state) {
-throttle_timers_attach_aio_context(
->public.throttle_group_member.throttle_timers,
-bdrv_get_aio_context(bs));
-}
-
return 0;
}
@@ -1024,13 +1013,6 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk,
int64_t offset,
}
bdrv_inc_in_flight(bs);
-
-/* throttling disk I/O */
-if (blk->public.throttle_group_member.throttle_state) {
-
throttle_group_co_io_limits_intercept(>public.throttle_group_member,
-bytes, false);
-}
-
ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
bdrv_dec_in_flight(bs);
return ret;
@@ -1051,11 +1033,6 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk,
int64_t offset,
}
bdrv_inc_in_flight(bs);
-/* throttling disk I/O */
-if (blk->public.throttle_group_member.throttle_state) {
-
throttle_group_co_io_limits_intercept(>public.throttle_group_member,
-bytes, true);
-}
if (!blk->enable_write_cache) {
flags |= BDRV_REQ_FUA;
@@ -1723,13 +1700,8 @@ static AioContext *blk_aiocb_get_aio_context
[Qemu-block] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list
block/throttle-groups.c uses a list to keep track of all groups and make
sure all names are unique.
This patch moves all ThrottleGroup objects under the root container.
While block/throttle.c requires that all throttle groups are created
with object-add or -object, the legacy throttling interface still used
the throttle_groups list. By using the root container we get the name
collision check for free and have all groups, legacy or not, available
through the qom-get/qom-set commands. Legacy groups are marked and freed
when they have no users left instead of waiting for an explicit
object-del.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 1 +
block/block-backend.c | 15 +++--
block/throttle-groups.c | 145 +++-
tests/test-throttle.c | 3 +
4 files changed, 96 insertions(+), 68 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 8493540766..13fbc63f1e 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -58,6 +58,7 @@ typedef struct ThrottleGroupMember {
const char *throttle_group_get_name(ThrottleGroupMember *tgm);
+void throttle_group_new_legacy(const char *name, Error **errp);
ThrottleState *throttle_group_incref(const char *name);
void throttle_group_unref(ThrottleState *ts);
diff --git a/block/block-backend.c b/block/block-backend.c
index 693ad27fc9..65f458ce8f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const char
*group, Error **errp)
BlockDriverState *bs = blk_bs(blk), *throttle_node;
QDict *options = qdict_new();
Error *local_err = NULL;
-ThrottleState *ts;
-
-bdrv_drained_begin(bs);
/* Force creation of group in case it doesn't exist */
-ts = throttle_group_incref(group);
+if (!throttle_group_exists(group)) {
+throttle_group_new_legacy(group, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+}
+
+bdrv_drained_begin(bs);
+
qdict_set_default_str(options, "file", bs->node_name);
qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"), NULL,
@@ -1987,7 +1993,6 @@ void blk_io_limits_enable(BlockBackend *blk, const char
*group, Error **errp)
assert(throttle_node->refcnt == 1);
end:
-throttle_group_unref(ts);
bdrv_drained_end(bs);
blk_get_public(blk)->throttle_node = throttle_node;
}
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 454bfcb067..238b648489 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -33,8 +33,10 @@
#include "qapi-visit.h"
#include "qom/object.h"
#include "qom/object_interfaces.h"
+#include "qapi/qobject-input-visitor.h"
static void throttle_group_obj_init(Object *obj);
+static bool throttle_group_can_be_deleted(UserCreatable *uc, Error **errp);
static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
/* The ThrottleGroup structure (with its ThrottleState) is shared
@@ -66,6 +68,7 @@ typedef struct ThrottleGroup {
/* refuse individual property change if initialization is complete */
bool is_initialized;
+bool legacy;
char *name; /* This is constant during the lifetime of the group */
QemuMutex lock; /* This lock protects the following four fields */
@@ -74,34 +77,47 @@ typedef struct ThrottleGroup {
ThrottleGroupMember *tokens[2];
bool any_timer_armed[2];
QEMUClockType clock_type;
-
-/* This field is protected by the global QEMU mutex */
-QTAILQ_ENTRY(ThrottleGroup) list;
} ThrottleGroup;
-/* This is protected by the global QEMU mutex */
-static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
-QTAILQ_HEAD_INITIALIZER(throttle_groups);
+struct ThrottleGroupQuery {
+const char *name;
+ThrottleGroup *result;
+};
-/* This function reads throttle_groups and must be called under the global
- * mutex.
+static int query_throttle_groups_foreach(Object *obj, void *data)
+{
+ThrottleGroup *tg;
+struct ThrottleGroupQuery *query = data;
+
+tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
+if (!tg) {
+return 0;
+}
+
+if (!g_strcmp0(query->name, tg->name)) {
+query->result = tg;
+return 1;
+}
+
+return 0;
+}
+
+
+/* This function reads the QOM root container and must be called under the
+ * global mutex.
*/
static ThrottleGroup *throttle_group_by_name(const char *name)
{
-ThrottleGroup *iter;
+struct ThrottleGroupQuery query = { name = name };
/* Look for an existing group with that name */
-QTAILQ_FOREACH(iter, _groups, list) {
-if (!g_strcmp0
[Qemu-block] [PATCH v3 6/7] block: remove BlockBackendPublic
All BlockBackend level throttling (via the implicit throttle filter node) is
done in block/block-backend.c and block/throttle-groups.c doesn't know
about BlockBackends anymore. Since BlockBackendPublic is not needed anymore,
remove it.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/sysemu/block-backend.h | 12 +---
block/block-backend.c | 43 +++---
block/qapi.c | 4 ++--
blockdev.c | 4 ++--
4 files changed, 24 insertions(+), 39 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a7ca53685..a05d75fa5f 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -68,14 +68,6 @@ typedef struct BlockDevOps {
void (*drained_end)(void *opaque);
} BlockDevOps;
-/* This struct is embedded in (the private) BlockBackend struct and contains
- * fields that must be public. This is in particular for QLIST_ENTRY() and
- * friends so that BlockBackends can be kept in lists outside block-backend.c
- * */
-typedef struct BlockBackendPublic {
-BlockDriverState *throttle_node;
-} BlockBackendPublic;
-
BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
BlockBackend *blk_new_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
@@ -90,9 +82,7 @@ BlockBackend *blk_all_next(BlockBackend *blk);
bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
void monitor_remove_blk(BlockBackend *blk);
-BlockBackendPublic *blk_get_public(BlockBackend *blk);
-BlockBackend *blk_by_public(BlockBackendPublic *public);
-
+BlockDriverState *blk_get_throttle_node(BlockBackend *blk);
BlockDriverState *blk_bs(BlockBackend *blk);
void blk_remove_bs(BlockBackend *blk);
int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp);
diff --git a/block/block-backend.c b/block/block-backend.c
index 65f458ce8f..c70e1c5cf7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -37,7 +37,10 @@ struct BlockBackend {
DriveInfo *legacy_dinfo;/* null unless created by drive_new() */
QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */
QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
-BlockBackendPublic public;
+
+/* implicit throttle filter node for backwards compatibility with legacy
+ * throttling commands */
+BlockDriverState *throttle_node;
void *dev; /* attached device model, if any */
bool legacy_dev;/* true if dev is not a DeviceState */
@@ -320,7 +323,7 @@ static void blk_delete(BlockBackend *blk)
assert(!blk->refcnt);
assert(!blk->name);
assert(!blk->dev);
-if (blk->public.throttle_node) {
+if (blk->throttle_node) {
blk_io_limits_disable(blk);
}
if (blk->root) {
@@ -615,19 +618,11 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
}
/*
- * Returns a pointer to the publicly accessible fields of @blk.
+ * Returns the throttle_node field of @blk.
*/
-BlockBackendPublic *blk_get_public(BlockBackend *blk)
+BlockDriverState *blk_get_throttle_node(BlockBackend *blk)
{
-return >public;
-}
-
-/*
- * Returns a BlockBackend given the associated @public fields.
- */
-BlockBackend *blk_by_public(BlockBackendPublic *public)
-{
-return container_of(public, BlockBackend, public);
+return blk->throttle_node;
}
/*
@@ -1920,15 +1915,15 @@ int blk_commit_all(void)
/* throttling disk I/O limits */
void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
{
-assert(blk->public.throttle_node);
-throttle_group_config(throttle_get_tgm(blk->public.throttle_node), cfg);
+assert(blk->throttle_node);
+throttle_group_config(throttle_get_tgm(blk->throttle_node), cfg);
}
void blk_io_limits_disable(BlockBackend *blk)
{
BlockDriverState *bs, *throttle_node;
-throttle_node = blk_get_public(blk)->throttle_node;
+throttle_node = blk->throttle_node;
assert(throttle_node);
@@ -1944,7 +1939,7 @@ void blk_io_limits_disable(BlockBackend *blk)
* blk, at this point it might have more than one parent, so use
* bdrv_replace_node(). This destroys throttle_node */
bdrv_replace_node(throttle_node, bs, _abort);
-blk_get_public(blk)->throttle_node = NULL;
+blk->throttle_node = NULL;
bdrv_unref(bs);
bdrv_drained_end(bs);
@@ -1994,7 +1989,7 @@ void blk_io_limits_enable(BlockBackend *blk, const char
*group, Error **errp)
end:
bdrv_drained_end(bs);
-blk_get_public(blk)->throttle_node = throttle_node;
+blk->throttle_node = throttle_node;
}
void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error
**errp)
@@ -2002,11 +1997,11 @@ void blk_io_limits_update_group(BlockBac
[Qemu-block] [PATCH v3 3/7] block: require job-id when device is a node name
With implicit filter nodes on the top of the graph it is not possible to
generate job-ids with the name of the device in block_job_create()
anymore, since the job's bs will not be a child_root.
Instead we can require that job-id is not NULL in block_job_create(),
and check that a job-id has been set in the callers of
block_job_create() in blockdev.c. It is more consistent to require an
explicit job-id when the device parameter in the job creation command,
eg
{ "execute": "drive-backup",
"arguments": { "device": "drive0",
"sync": "full",
"target": "backup.img" } }
is not a BlockBackend name, instead of automatically getting it from the
root BS if device is a node name. That information is lost after calling
block_job_create(), so we can do it in its caller instead.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/blockjob_int.h | 4 +--
blockdev.c | 65 +++-
blockjob.c | 19 ++---
tests/test-blockjob.c| 9 +++---
4 files changed, 72 insertions(+), 25 deletions(-)
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..a4ab7f3d59 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,8 @@ struct BlockJobDriver {
/**
* block_job_create:
- * @job_id: The id of the newly-created job, or %NULL to have one
- * generated automatically.
+ * @job_id: The id of the newly-created job, must be non %NULL for external
+ * jobs and %NULL for internal jobs.
* @job_type: The class object for the newly-created job.
* @bs: The block
* @perm, @shared_perm: Permissions to request for @bs
diff --git a/blockdev.c b/blockdev.c
index fc7b65c3f0..6ffa5b0b04 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3004,6 +3004,16 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
return;
}
+/* Always require a job-id when device is a node name */
+if (!has_job_id) {
+if (blk_by_name(device)) {
+job_id = device;
+} else {
+error_setg(errp, "An explicit job ID is required for this node");
+return;
+}
+}
+
/* Skip implicit filter nodes */
bs = bdrv_get_first_explicit(bs);
@@ -3058,7 +3068,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
/* backing_file string overrides base bs filename */
base_name = has_backing_file ? backing_file : base_name;
-stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+stream_start(job_id, bs, base_bs, base_name,
has_speed ? speed : 0, on_error, _err);
if (local_err) {
error_propagate(errp, local_err);
@@ -3117,6 +3127,16 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
/* Skip implicit filter nodes */
bs = bdrv_get_first_explicit(bs);
+/* Always require a job-id when device is a node name */
+if (!has_job_id) {
+if (blk_by_name(device)) {
+job_id = device;
+} else {
+error_setg(errp, "An explicit job ID is required for this node");
+return;
+}
+}
+
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3171,7 +3191,7 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
" but 'top' is the active layer");
goto out;
}
-commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
+commit_active_start(job_id, bs, base_bs,
BLOCK_JOB_DEFAULT, speed, on_error,
filter_node_name, NULL, NULL, false, _err);
} else {
@@ -3179,7 +3199,7 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp))
{
goto out;
}
-commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
+commit_start(job_id, bs, base_bs, top_bs, speed,
on_error, has_backing_file ? backing_file : NULL,
filter_node_name, _err);
}
@@ -3220,7 +3240,13 @@ static BlockJob *do_drive_backup(DriveBackup *backup,
BlockJobTxn *txn,
backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
}
if (!backup->has_job_id) {
-backup->job_id = NULL;
+/* Always require a job-id when device is a node name */
+if (blk_by_name(backup->device)) {
+backup->job_id = backup->device;
+} else {
+error_setg(errp, "An explicit job ID is required for this node");
+return NULL;
+
[Qemu-block] [PATCH v3 2/7] block: add options parameter to bdrv_new_open_driver()
Allow passing a QDict *options parameter to bdrv_new_open_driver() so
that it can be used if a driver needs it upon creation. The previous
behaviour (empty bs->options and bs->explicit_options) remains when
options is NULL.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block.h | 2 +-
block.c | 16 +---
block/commit.c| 4 ++--
block/mirror.c| 2 +-
block/vvfat.c | 2 +-
5 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index ab80195378..d1f03cb48b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -263,7 +263,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict
*parent_options,
BlockDriverState *bdrv_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
- int flags, Error **errp);
+ int flags, QDict *options, Error
**errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
QDict *options, int flags);
diff --git a/block.c b/block.c
index e35d546c08..2de1c29eb3 100644
--- a/block.c
+++ b/block.c
@@ -1153,16 +1153,26 @@ open_failed:
return ret;
}
+/*
+ * If options is not NULL, its ownership is transferred to the block layer. The
+ * caller must use QINCREF() if they wish to keep ownership.
+ */
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
- int flags, Error **errp)
+ int flags, QDict *options, Error **errp)
{
BlockDriverState *bs;
int ret;
bs = bdrv_new();
bs->open_flags = flags;
-bs->explicit_options = qdict_new();
-bs->options = qdict_new();
+if (options) {
+bs->explicit_options = qdict_clone_shallow(options);
+bs->options = qdict_clone_shallow(options);
+QDECREF(options);
+} else {
+bs->explicit_options = qdict_new();
+bs->options = qdict_new();
+}
bs->opaque = NULL;
update_options_from_flags(bs->options, flags);
diff --git a/block/commit.c b/block/commit.c
index c7857c3321..539e23c3f8 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -342,7 +342,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
/* Insert commit_top block node above top, so we can block consistent read
* on the backing chain below it */
commit_top_bs = bdrv_new_open_driver(_commit_top, filter_node_name, 0,
- errp);
+ NULL, errp);
if (commit_top_bs == NULL) {
goto fail;
}
@@ -494,7 +494,7 @@ int bdrv_commit(BlockDriverState *bs)
backing_file_bs = backing_bs(bs);
commit_top_bs = bdrv_new_open_driver(_commit_top, NULL, BDRV_O_RDWR,
- _err);
+ NULL, _err);
if (commit_top_bs == NULL) {
error_report_err(local_err);
goto ro_cleanup;
diff --git a/block/mirror.c b/block/mirror.c
index c9a6a3ca86..e1a160e6ea 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1164,7 +1164,7 @@ static void mirror_start_job(const char *job_id,
BlockDriverState *bs,
* reads on the top, while disabling it in the intermediate nodes, and make
* the backing chain writable. */
mirror_top_bs = bdrv_new_open_driver(_mirror_top, filter_node_name,
- BDRV_O_RDWR, errp);
+ BDRV_O_RDWR, NULL, errp);
if (mirror_top_bs == NULL) {
return;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index a9e207f7f0..6c59473baf 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3194,7 +3194,7 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
#endif
backing = bdrv_new_open_driver(_write_target, NULL,
BDRV_O_ALLOW_RDWR,
- _abort);
+ NULL, _abort);
*(void**) backing->opaque = s;
bdrv_set_backing_hd(s->bs, backing, _abort);
--
2.11.0
[Qemu-block] [PATCH v3 7/7] qemu-iotests: add 191 for legacy throttling interface
Check that the implicit throttle filter driver node, used for
compatibility with the legacy throttling interface on the BlockBackend
level, works.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
tests/qemu-iotests/191 | 138 +
tests/qemu-iotests/191.out | 5 ++
tests/qemu-iotests/group | 1 +
3 files changed, 144 insertions(+)
create mode 100644 tests/qemu-iotests/191
create mode 100644 tests/qemu-iotests/191.out
diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
new file mode 100644
index 00..82fd0b2fe5
--- /dev/null
+++ b/tests/qemu-iotests/191
@@ -0,0 +1,138 @@
+#!/usr/bin/env python
+#
+# Tests that the legacy throttling interface using an implicit throttle filter
+# driver node works
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+
+class TestLegacyThrottling(iotests.QMPTestCase):
+test_img = os.path.join(iotests.test_dir, "test.img")
+target_img = os.path.join(iotests.test_dir, "target.img")
+base_img = os.path.join(iotests.test_dir, "base.img")
+
+def setUp(self):
+iotests.qemu_img("create", "-f", iotests.imgfmt, self.base_img, "1G")
+iotests.qemu_img("create", "-f", iotests.imgfmt, self.test_img, "-b",
self.base_img)
+iotests.qemu_io("-f", iotests.imgfmt, "-c", "write -P0x5d 1M 128M",
self.test_img)
+self.vm = iotests.VM().add_drive(self.test_img)
+self.vm.launch()
+
+def tearDown(self):
+self.do_check_throttle_node(expect=True)
+params = {"device": "drive0",
+ "bps": 0,
+ "bps_rd": 0,
+ "bps_wr": 0,
+ "iops": 0,
+ "iops_rd": 0,
+ "iops_wr": 0,
+ }
+"""
+This must remove the implicit throttle_node
+"""
+result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+ **params)
+self.do_check_throttle_node(expect=False)
+self.vm.shutdown()
+
+def do_test_job(self, cmd, **args):
+params = {"device": "drive0",
+ "bps": 1024,
+ "bps_rd": 0,
+ "bps_wr": 0,
+ "iops": 0,
+ "iops_rd": 0,
+ "iops_wr": 0,
+ }
+result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+ **params)
+self.assert_qmp(result, "return", {})
+result = self.vm.qmp(cmd, **args)
+self.assert_qmp(result, "return", {})
+result = self.vm.qmp("query-block-jobs")
+self.assert_qmp(result, "return[0]/device", "drive0")
+
+def do_check_throttle_node(self, expect):
+result = self.vm.qmp("query-named-block-nodes")
+for r in result["return"]:
+if r["drv"] == "throttle":
+self.assertTrue(expect)
+return
+if expect:
+""" throttle_node missing! """
+self.assertTrue(False)
+
+def do_check_params(self, file):
+result = self.vm.qmp("query-block")
+self.assert_qmp(result, "return[0]/inserted/bps", 1024)
+self.assert_qmp(result, "return[0]/inserted/drv", iotests.imgfmt)
+self.assert_qmp(result, "return[0]/inserted/file", file)
+
+"""
+Check that query-block reports the correct throttling parameters while
+ignoring the implicit throttle node.
+"""
+def test_query_block(self):
+params = {"device": "drive0",
+ "bps": 1024,
+ "bps_rd": 0,
+ "bps_wr": 0,
+ "iops":
[Qemu-block] [PATCH v3 1/7] block: skip implicit nodes in snapshots, blockjobs
Implicit filter nodes added at the top of nodes can interfere with block
jobs. This is not a problem when they are added by other jobs since
adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
the next commit which introduces an implicitly created throttle filter
node below BlockBackend, which we want to be skipped during automatic
operations on the graph since the user does not necessarily know about
their existence.
All implicit filters will have either bs->file or bs->backing set. This
is a needed assumption so we can know which direction we will skip down
the graph.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/block_int.h | 17 +
block.c | 10 ++
block/qapi.c | 14 +-
blockdev.c| 34 ++
4 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7571c0aaaf..9e0f70e055 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -699,6 +699,21 @@ static inline BlockDriverState
*backing_bs(BlockDriverState *bs)
return bs->backing ? bs->backing->bs : NULL;
}
+static inline BlockDriverState *file_bs(BlockDriverState *bs)
+{
+return bs->file ? bs->file->bs : NULL;
+}
+
+/* This is a convenience function to get either bs->file->bs or
+ * bs->backing->bs * */
+static inline BlockDriverState *child_bs(BlockDriverState *bs)
+{
+BlockDriverState *backing = backing_bs(bs);
+BlockDriverState *file = file_bs(bs);
+assert(!(file && backing));
+return backing ?: file;
+}
+
/* Essential block drivers which must always be statically linked into qemu,
and
* which therefore can be accessed without using bdrv_find_format() */
@@ -980,4 +995,6 @@ void bdrv_dec_in_flight(BlockDriverState *bs);
void blockdev_close_all_bdrv_states(void);
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs);
+
#endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index 3615a6809e..e35d546c08 100644
--- a/block.c
+++ b/block.c
@@ -4945,3 +4945,13 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState
*bs, const char *name,
return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
}
+
+/* Get first explicit node down a bs chain. */
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs)
+{
+while (bs && bs->drv && bs->implicit) {
+bs = child_bs(bs);
+assert(bs);
+}
+return bs;
+}
diff --git a/block/qapi.c b/block/qapi.c
index 7fa2437923..847b044d13 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -147,9 +147,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
/* Skip automatically inserted nodes that the user isn't aware of for
* query-block (blk != NULL), but not for query-named-block-nodes */
-while (blk && bs0->drv && bs0->implicit) {
-bs0 = backing_bs(bs0);
-assert(bs0);
+if (blk) {
+bs0 = bdrv_get_first_explicit(bs0);
}
}
@@ -336,9 +335,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo
**p_info,
char *qdev;
/* Skip automatically inserted nodes that the user isn't aware of */
-while (bs && bs->drv && bs->implicit) {
-bs = backing_bs(bs);
-}
+bs = bdrv_get_first_explicit(bs);
info->device = g_strdup(blk_name(blk));
info->type = g_strdup("unknown");
@@ -465,9 +462,8 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState
*bs,
/* Skip automatically inserted nodes that the user isn't aware of in
* a BlockBackend-level command. Stay at the exact node for a node-level
* command. */
-while (blk_level && bs->drv && bs->implicit) {
-bs = backing_bs(bs);
-assert(bs);
+if (blk_level) {
+bs = bdrv_get_first_explicit(bs);
}
if (bdrv_get_node_name(bs)[0]) {
diff --git a/blockdev.c b/blockdev.c
index 23475abb72..fc7b65c3f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1300,6 +1300,10 @@ SnapshotInfo
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
if (!bs) {
return NULL;
}
+
+/* Skip implicit filter nodes */
+bs = bdrv_get_first_explicit(bs);
+
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -1508,6 +1512,9 @@ static void internal_snapshot_prepare(BlkActionState
*common,
return;
}
+/* Skip implicit filter nodes */
+bs = bdrv_get_first_explicit(bs);
+
/* AioContext is released in .clean() */
state->aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(state->aio_context);
@@ -1664,6 +1671,9 @@ static void external_snapshot_prepare(BlkActionState
*common,
return;
}
+/*
[Qemu-block] [PATCH v3 0/6] block: remove legacy I/O throttling
This series depends on my other series 'add throttle block driver filter' currently on v9 Based-on: <20170825132028.6184-1-el13...@mail.ntua.gr> Replacing the current I/O interface means the user will use the same options as before and QEMU will create a hidden throttle filter node beneath the device's BlockBackend. v3: fix suggestions by berto new commit: remove throttle-groups list v2: new commit: require job-id when device is a node name new commit: remove BlockBackendPublic new commit: add dedicated iotest cleanup reference counting in block/block-backend.c functions add new function to get filter child bs take ownership of options in bdrv_new_open_driver() Manos Pitsidianakis (7): block: skip implicit nodes in snapshots, blockjobs block: add options parameter to bdrv_new_open_driver() block: require job-id when device is a node name block: remove legacy I/O throttling block/throttle-groups.c: remove throttle-groups list block: remove BlockBackendPublic qemu-iotests: add 191 for legacy throttling interface include/block/block.h | 2 +- include/block/block_int.h | 17 + include/block/blockjob_int.h| 4 +- include/block/throttle-groups.h | 2 + include/sysemu/block-backend.h | 16 +--- block.c | 26 ++- block/block-backend.c | 158 block/commit.c | 4 +- block/mirror.c | 2 +- block/qapi.c| 24 +++--- block/throttle-groups.c | 145 block/throttle.c| 8 ++ block/vvfat.c | 2 +- blockdev.c | 136 ++ blockjob.c | 19 ++--- tests/test-blockjob.c | 9 +-- tests/test-throttle.c | 22 -- tests/qemu-iotests/191 | 138 +++ tests/qemu-iotests/191.out | 5 ++ tests/qemu-iotests/group| 1 + 20 files changed, 538 insertions(+), 202 deletions(-) create mode 100644 tests/qemu-iotests/191 create mode 100644 tests/qemu-iotests/191.out -- 2.11.0
[Qemu-block] [PATCH v9 5/6] block: add throttle block filter driver
block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c
The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
which registers the throttle filter node with the ThrottleGroup 'bar'. The
given group must be created beforehand with object-add or -object.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 18 ++-
include/block/throttle-groups.h | 5 +
include/qemu/throttle-options.h | 1 +
block/throttle-groups.c | 15 ++-
block/throttle.c| 250
block/Makefile.objs | 1 +
6 files changed, 288 insertions(+), 2 deletions(-)
create mode 100644 block/throttle.c
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..03724146a4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
# Drivers that are supported in block device operations.
#
# @vxhs: Since 2.10
+# @throttle: Since 2.11
#
# Since: 2.9
##
@@ -2231,7 +2232,7 @@
'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
##
# @BlockdevOptionsFile:
@@ -3095,6 +3096,20 @@
'*tls-creds': 'str' } }
##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group: the name of the throttle-group object to use. It
+#must already exist.
+# @file: reference to or definition of the data source block device
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+ 'data': { 'throttle-group': 'str',
+'file' : 'BlockdevRef'
+ } }
+##
# @BlockdevOptions:
#
# Options for creating a block device. Many options are available for all
@@ -3155,6 +3170,7 @@
'replication':'BlockdevOptionsReplication',
'sheepdog': 'BlockdevOptionsSheepdog',
'ssh':'BlockdevOptionsSsh',
+ 'throttle': 'BlockdevOptionsThrottle',
'vdi':'BlockdevOptionsGenericFormat',
'vhdx': 'BlockdevOptionsGenericFormat',
'vmdk': 'BlockdevOptionsGenericCOWFormat',
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..e2fd0513c4 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,10 @@ void coroutine_fn
throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+/*
+ * throttle_group_exists() must be called under the global
+ * mutex.
+ */
+bool throttle_group_exists(const char *name);
#endif
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 182b7896e1..3528a8f4a2 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -29,6 +29,7 @@
#define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
#define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
#define QEMU_OPT_IOPS_SIZE "iops-size"
+#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
#define THROTTLE_OPT_PREFIX "throttling."
#define THROTTLE_OPTS \
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5993575838..454bfcb067 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -101,6 +101,14 @@ static ThrottleGroup *throttle_group_by_name(const char
*name)
return NULL;
}
+/* This function reads throttle_groups and must be called under the global
+ * mutex.
+ */
+bool throttle_group_exists(const char *name)
+{
+return throttle_group_by_name(name) != NULL;
+}
+
/* Increments the reference count of a ThrottleGroup given its name.
*
* If no ThrottleGroup is found with the given name a new one is
@@ -543,6 +551,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember
*tgm)
ThrottleGroupMember *token;
int i;
+if (!ts) {
+/* Discard already unregistered tgm */
+return;
+}
+
assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
assert(qemu_co_queue_empty(>throttled_reqs[0]));
assert(qemu_co_queue_empty(>throttled_reqs[1]));
@@ -709,7 +722,7 @@ static void throttle_group_obj_complete(UserCreatable *obj,
Error **errp)
assert(tg->name);
/* error if name is duplicate */
-if
[Qemu-block] [PATCH v9 1/6] block: move ThrottleGroup membership to ThrottleGroupMember
This commit eliminates the 1:1 relationship between BlockBackend and
throttle group state. Users will be able to create multiple throttle
nodes, each with its own throttle group state, in the future. The
throttle group state cannot be per-BlockBackend anymore, it must be
per-throttle node. This is done by gathering ThrottleGroup membership
details from BlockBackendPublic into ThrottleGroupMember and refactoring
existing code to use the structure.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 39 +-
include/sysemu/block-backend.h | 20 +--
block/block-backend.c | 66 +
block/qapi.c| 8 +-
block/throttle-groups.c | 288
blockdev.c | 4 +-
tests/test-throttle.c | 53
7 files changed, 252 insertions(+), 226 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index d983d34074..1a6bcdae74 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -28,19 +28,44 @@
#include "qemu/throttle.h"
#include "block/block_int.h"
-const char *throttle_group_get_name(BlockBackend *blk);
+/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup
+ * and holds related data.
+ */
+
+typedef struct ThrottleGroupMember {
+/* throttled_reqs_lock protects the CoQueues for throttled requests. */
+CoMutex throttled_reqs_lock;
+CoQueue throttled_reqs[2];
+
+/* Nonzero if the I/O limits are currently being ignored; generally
+ * it is zero. Accessed with atomic operations.
+ */
+unsigned int io_limits_disabled;
+
+/* The following fields are protected by the ThrottleGroup lock.
+ * See the ThrottleGroup documentation for details.
+ * throttle_state tells us if I/O limits are configured. */
+ThrottleState *throttle_state;
+ThrottleTimers throttle_timers;
+unsigned pending_reqs[2];
+QLIST_ENTRY(ThrottleGroupMember) round_robin;
+
+} ThrottleGroupMember;
+
+const char *throttle_group_get_name(ThrottleGroupMember *tgm);
ThrottleState *throttle_group_incref(const char *name);
void throttle_group_unref(ThrottleState *ts);
-void throttle_group_config(BlockBackend *blk, ThrottleConfig *cfg);
-void throttle_group_get_config(BlockBackend *blk, ThrottleConfig *cfg);
+void throttle_group_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
+void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
-void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
-void throttle_group_unregister_blk(BlockBackend *blk);
-void throttle_group_restart_blk(BlockBackend *blk);
+void throttle_group_register_tgm(ThrottleGroupMember *tgm,
+const char *groupname);
+void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
+void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
-void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk,
+void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember
*tgm,
unsigned int bytes,
bool is_write);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a3730596b..0e0cda7521 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -70,24 +70,10 @@ typedef struct BlockDevOps {
/* This struct is embedded in (the private) BlockBackend struct and contains
* fields that must be public. This is in particular for QLIST_ENTRY() and
- * friends so that BlockBackends can be kept in lists outside block-backend.c
*/
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ * */
typedef struct BlockBackendPublic {
-/* throttled_reqs_lock protects the CoQueues for throttled requests. */
-CoMutex throttled_reqs_lock;
-CoQueue throttled_reqs[2];
-
-/* Nonzero if the I/O limits are currently being ignored; generally
- * it is zero. Accessed with atomic operations.
- */
-unsigned int io_limits_disabled;
-
-/* The following fields are protected by the ThrottleGroup lock.
- * See the ThrottleGroup documentation for details.
- * throttle_state tells us if I/O limits are configured. */
-ThrottleState *throttle_state;
-ThrottleTimers throttle_timers;
-unsigned pending_reqs[2];
-QLIST_ENTRY(BlockBackendPublic) round_robin;
+ThrottleGroupMember throttle_group_member;
} BlockBackendPublic;
BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
diff --git a/block/block-backend.c b/block/block-backend.c
index e9798e897d..7b981e00bb 100644
--- a/block/block
[Qemu-block] [PATCH v9 4/6] block: convert ThrottleGroup to object with QOM
ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.
A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.
ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.
ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct. It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:
{ "execute": "object-add",
"arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
"limits": {
"iops-total": 100
}
}
}
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}
This also means a group's configuration can be fetched with qom-get.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 48 +
include/block/throttle-groups.h | 3 +
include/qemu/throttle-options.h | 59 --
include/qemu/throttle.h | 3 +
block/throttle-groups.c | 422
tests/test-throttle.c | 1 +
util/throttle.c | 151 ++
7 files changed, 627 insertions(+), 60 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602150..0bdc69aa5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1905,6 +1905,54 @@
'*iops_size': 'int', '*group': 'str' } }
##
+# @ThrottleLimits:
+#
+# Limit parameters for throttling.
+# Since some limit combinations are illegal, limits should always be set in one
+# transaction. All fields are optional. When setting limits, if a field is
+# missing the current value is not changed.
+#
+# @iops-total: limit total I/O operations per second
+# @iops-total-max: I/O operations burst
+# @iops-total-max-length: length of the iops-total-max burst period, in
seconds
+# It must only be set if @iops-total-max is set as
well.
+# @iops-read: limit read operations per second
+# @iops-read-max: I/O operations read burst
+# @iops-read-max-length: length of the iops-read-max burst period, in seconds
+# It must only be set if @iops-read-max is set as
well.
+# @iops-write: limit write operations per second
+# @iops-write-max: I/O operations write burst
+# @iops-write-max-length: length of the iops-write-max burst period, in
seconds
+# It must only be set if @iops-write-max is set as
well.
+# @bps-total: limit total bytes per second
+# @bps-total-max: total bytes burst
+# @bps-total-max-length: length of the bps-total-max burst period, in
seconds.
+# It must only be set if @bps-total-max is set as
well.
+# @bps-read: limit read bytes per second
+# @bps-read-max: total bytes read burst
+# @bps-read-max-length:length of the bps-read-max burst period, in seconds
+# It must only be set if @bps-read-max is set as well.
+# @bps-write: limit write bytes per second
+# @bps-write-max: total bytes write burst
+# @bps-write-max-length: length of the bps-write-max burst period, in seconds
+# It must only be set if @bps-write-max is set as
well.
+# @iops-size: when limiting by iops max size of an I/O in bytes
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleLimits',
+ 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
+'*iops-total-max-length' : 'int', '*iops-read' : 'int',
+'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
+'*iops-write' : 'int', '*iops-write-max' : 'int',
+'*iops-write-max-length' : 'int', '*bps-total' : 'int',
+'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
+'*bps-read' : 'int', '*bps-read-max' : 'int',
+'*bps-read-max-length' : 'int', '*bps-write' : 'int',
+'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
+'*iop
[Qemu-block] [PATCH v9 6/6] qemu-iotests: add 184 for throttle filter driver
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
tests/qemu-iotests/184 | 205 +++
tests/qemu-iotests/184.out | 300 +
tests/qemu-iotests/group | 1 +
3 files changed, 506 insertions(+)
create mode 100755 tests/qemu-iotests/184
create mode 100644 tests/qemu-iotests/184.out
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
new file mode 100755
index 00..704f38f936
--- /dev/null
+++ b/tests/qemu-iotests/184
@@ -0,0 +1,205 @@
+#!/bin/bash
+#
+# Test I/O throttle block filter driver interface
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner="Manos Pitsidianakis"
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@" | _filter_imgfmt
+$QEMU -nographic -qmp-pretty stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
+ | _filter_qemu_io | _filter_generated_node_ids
+}
+
+_make_test_img 64M
+test_throttle=$($QEMU_IMG --help|grep throttle)
+[ "$test_throttle" = "" ] && _supported_fmt throttle
+
+echo
+echo "== checking interface =="
+
+run_qemu <
[Qemu-block] [PATCH v9 0/6] add throttle block driver filter
This series adds a throttle block driver filter. Currently throttling is done
at the BlockBackend level. Using block driver interfaces we can move the
throttling to any point in the BDS graph using a throttle node which uses the
existing throttling code. This allows for potentially more complex
configurations (throttling at any point in the graph, chained filters)
v9:
fix suggestions by stefanha in patch 4
add 'throttle_group_can_be_deleted' function in patch 4
v8:
fix suggestions by berto
v7:
remove anonymous groups
error on missing throttle-group option on block/throttle.c
change switch case format in block/throttle-groups.c (berto)
v6:
don't allow named group limit configuration in block/throttle.c; allow either
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...
or
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
fixes suggested by berto
v5:
fix crash in 'add aio_context field in ThrottleGroupMember'
fix suggestions in block/throttle.c
v4:
fix suggestions in block/throttle.c
fix suggestions in block/throttle_groups.c
add doc note in BlockDevOptionsThrottle
v3:
fix style error in 'add aio_context field in ThrottleGroupMember'
v2:
change QOM throttle group object name
print valid ranges for uint on error
move frees in throttle_group_obj_finalize()
split throttle_group_{set,get}()
add throttle_recurse_is_first_non_filter()
Manos Pitsidianakis (6):
block: move ThrottleGroup membership to ThrottleGroupMember
block: add aio_context field in ThrottleGroupMember
block: tidy ThrottleGroupMember initializations
block: convert ThrottleGroup to object with QOM
block: add throttle block filter driver
qemu-iotests: add 184 for throttle filter driver
qapi/block-core.json| 66 +++-
include/block/throttle-groups.h | 52 ++-
include/qemu/throttle-options.h | 60 +++-
include/qemu/throttle.h | 3 +
include/sysemu/block-backend.h | 20 +-
block/block-backend.c | 62 ++--
block/qapi.c| 8 +-
block/throttle-groups.c | 748 ++--
block/throttle.c| 250 ++
blockdev.c | 4 +-
tests/test-throttle.c | 111 +++---
util/throttle.c | 151
block/Makefile.objs | 1 +
tests/qemu-iotests/184 | 205 +++
tests/qemu-iotests/184.out | 300
tests/qemu-iotests/group| 1 +
16 files changed, 1721 insertions(+), 321 deletions(-)
create mode 100644 block/throttle.c
create mode 100755 tests/qemu-iotests/184
create mode 100644 tests/qemu-iotests/184.out
--
2.11.0
[Qemu-block] [PATCH v9 2/6] block: add aio_context field in ThrottleGroupMember
timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 7 -
block/block-backend.c | 15 --
block/throttle-groups.c | 38 -
tests/test-throttle.c | 63 +
4 files changed, 69 insertions(+), 54 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 1a6bcdae74..a0f27cac63 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -33,6 +33,7 @@
*/
typedef struct ThrottleGroupMember {
+AioContext *aio_context;
/* throttled_reqs_lock protects the CoQueues for throttled requests. */
CoMutex throttled_reqs_lock;
CoQueue throttled_reqs[2];
@@ -61,12 +62,16 @@ void throttle_group_config(ThrottleGroupMember *tgm,
ThrottleConfig *cfg);
void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
void throttle_group_register_tgm(ThrottleGroupMember *tgm,
-const char *groupname);
+const char *groupname,
+AioContext *ctx);
void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember
*tgm,
unsigned int bytes,
bool is_write);
+void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
+ AioContext *new_context);
+void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
#endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 7b981e00bb..9ba800e733 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1726,18 +1726,14 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB
*acb)
void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
{
BlockDriverState *bs = blk_bs(blk);
-ThrottleTimers *tt;
+ThrottleGroupMember *tgm = >public.throttle_group_member;
if (bs) {
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
+if (tgm->throttle_state) {
+throttle_group_detach_aio_context(tgm);
+throttle_group_attach_aio_context(tgm, new_context);
}
bdrv_set_aio_context(bs, new_context);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_attach_aio_context(tt, new_context);
-}
}
}
@@ -1970,7 +1966,8 @@ void blk_io_limits_disable(BlockBackend *blk)
void blk_io_limits_enable(BlockBackend *blk, const char *group)
{
assert(!blk->public.throttle_group_member.throttle_state);
-throttle_group_register_tgm(>public.throttle_group_member, group);
+throttle_group_register_tgm(>public.throttle_group_member,
+group, blk_get_aio_context(blk));
}
void blk_io_limits_update_group(BlockBackend *blk, const char *group)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index c8ed16ddf8..3b07b25f39 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -391,9 +391,6 @@ static void coroutine_fn
throttle_group_restart_queue_entry(void *opaque)
static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool
is_write)
{
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
Coroutine *co;
RestartData rd = {
.tgm = tgm,
@@ -401,7 +398,7 @@ static void
throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
};
co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
-aio_co_enter(blk_get_aio_context(blk), co);
+aio_co_enter(tgm->aio_context, co);
}
void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
@@ -449,13 +446,11 @@ void throttle_group_get_config(ThrottleGroupMember *tgm,
ThrottleConfig *cfg)
/* ThrottleTimers callback. This wakes up a request that was waiting
* because it had been throttled.
*
- * @blk: the BlockBackend whose request had been throttled
+ * @tgm: the ThrottleGroupMember whose request had been throttled
* @is_write: the type of operation (rea
[Qemu-block] [PATCH v9 3/6] block: tidy ThrottleGroupMember initializations
Move the CoMutex and CoQueue inits inside throttle_group_register_tgm() which is called whenever a ThrottleGroupMember is initialized. There's no need for them to be separate. Reviewed-by: Alberto Garcia <be...@igalia.com> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> --- block/block-backend.c | 3 --- block/throttle-groups.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 9ba800e733..c51fb8c8aa 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -252,9 +252,6 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm) blk->shared_perm = shared_perm; blk_set_enable_write_cache(blk, true); -qemu_co_mutex_init(>public.throttle_group_member.throttled_reqs_lock); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[0]); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[1]); block_acct_init(>stats); notifier_list_init(>remove_bs_notifiers); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 3b07b25f39..7749cf043f 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -508,6 +508,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, read_timer_cb, write_timer_cb, tgm); +qemu_co_mutex_init(>throttled_reqs_lock); +qemu_co_queue_init(>throttled_reqs[0]); +qemu_co_queue_init(>throttled_reqs[1]); qemu_mutex_unlock(>lock); } -- 2.11.0
Re: [Qemu-block] [Qemu-devel] [PATCH v7 6/6] qemu-iotests: add 184 for throttle filter driver
On Thu, Aug 24, 2017 at 07:43:08PM +0100, Stefan Hajnoczi wrote: On Tue, Aug 22, 2017 at 01:15:35PM +0300, Manos Pitsidianakis wrote: +_supported_fmt qcow2 What makes this test qcow2-specific? With additional filtering for IMGFMT it should be possible to run this on any image format. I think none of the available filters hide all of the image details in query-block and company. _filter_imgfmt hides the image extensions but not the image specific fields (eg virtual-size, actual-size and format-specific). Perhaps a new filter that hides everything but "format" and "filename" in the "image" subdict can be introduced later. signature.asc Description: PGP signature
[Qemu-block] [PATCH v8 6/6] qemu-iotests: add 184 for throttle filter driver
Reviewed-by: Alberto Garcia <be...@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
tests/qemu-iotests/184 | 205 +++
tests/qemu-iotests/184.out | 300 +
tests/qemu-iotests/group | 1 +
3 files changed, 506 insertions(+)
create mode 100755 tests/qemu-iotests/184
create mode 100644 tests/qemu-iotests/184.out
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
new file mode 100755
index 00..704f38f936
--- /dev/null
+++ b/tests/qemu-iotests/184
@@ -0,0 +1,205 @@
+#!/bin/bash
+#
+# Test I/O throttle block filter driver interface
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner="Manos Pitsidianakis"
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@" | _filter_imgfmt
+$QEMU -nographic -qmp-pretty stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
+ | _filter_qemu_io | _filter_generated_node_ids
+}
+
+_make_test_img 64M
+test_throttle=$($QEMU_IMG --help|grep throttle)
+[ "$test_throttle" = "" ] && _supported_fmt throttle
+
+echo
+echo "== checking interface =="
+
+run_qemu <
[Qemu-block] [PATCH v8 2/6] block: add aio_context field in ThrottleGroupMember
timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 7 -
block/block-backend.c | 15 --
block/throttle-groups.c | 38 -
tests/test-throttle.c | 63 +
4 files changed, 69 insertions(+), 54 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 1a6bcdae74..a0f27cac63 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -33,6 +33,7 @@
*/
typedef struct ThrottleGroupMember {
+AioContext *aio_context;
/* throttled_reqs_lock protects the CoQueues for throttled requests. */
CoMutex throttled_reqs_lock;
CoQueue throttled_reqs[2];
@@ -61,12 +62,16 @@ void throttle_group_config(ThrottleGroupMember *tgm,
ThrottleConfig *cfg);
void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
void throttle_group_register_tgm(ThrottleGroupMember *tgm,
-const char *groupname);
+const char *groupname,
+AioContext *ctx);
void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember
*tgm,
unsigned int bytes,
bool is_write);
+void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
+ AioContext *new_context);
+void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
#endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 7b981e00bb..9ba800e733 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1726,18 +1726,14 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB
*acb)
void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
{
BlockDriverState *bs = blk_bs(blk);
-ThrottleTimers *tt;
+ThrottleGroupMember *tgm = >public.throttle_group_member;
if (bs) {
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
+if (tgm->throttle_state) {
+throttle_group_detach_aio_context(tgm);
+throttle_group_attach_aio_context(tgm, new_context);
}
bdrv_set_aio_context(bs, new_context);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_attach_aio_context(tt, new_context);
-}
}
}
@@ -1970,7 +1966,8 @@ void blk_io_limits_disable(BlockBackend *blk)
void blk_io_limits_enable(BlockBackend *blk, const char *group)
{
assert(!blk->public.throttle_group_member.throttle_state);
-throttle_group_register_tgm(>public.throttle_group_member, group);
+throttle_group_register_tgm(>public.throttle_group_member,
+group, blk_get_aio_context(blk));
}
void blk_io_limits_update_group(BlockBackend *blk, const char *group)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index c8ed16ddf8..3b07b25f39 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -391,9 +391,6 @@ static void coroutine_fn
throttle_group_restart_queue_entry(void *opaque)
static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool
is_write)
{
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
Coroutine *co;
RestartData rd = {
.tgm = tgm,
@@ -401,7 +398,7 @@ static void
throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
};
co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
-aio_co_enter(blk_get_aio_context(blk), co);
+aio_co_enter(tgm->aio_context, co);
}
void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
@@ -449,13 +446,11 @@ void throttle_group_get_config(ThrottleGroupMember *tgm,
ThrottleConfig *cfg)
/* ThrottleTimers callback. This wakes up a request that was waiting
* because it had been throttled.
*
- * @blk: the BlockBackend whose request had been throttled
+ * @tgm: the ThrottleGroupMember whose request had been throttled
* @is_write: the type of operation (rea
[Qemu-block] [PATCH v8 0/6] add throttle block driver filter
This series adds a throttle block driver filter. Currently throttling is done
at the BlockBackend level. Using block driver interfaces we can move the
throttling to any point in the BDS graph using a throttle node which uses the
existing throttling code. This allows for potentially more complex
configurations (throttling at any point in the graph, chained filters)
v8:
fix suggestions by berto
v7:
remove anonymous groups
error on missing throttle-group option on block/throttle.c
change switch case format in block/throttle-groups.c (berto)
v6:
don't allow named group limit configuration in block/throttle.c; allow either
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...
or
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
fixes suggested by berto
v5:
fix crash in 'add aio_context field in ThrottleGroupMember'
fix suggestions in block/throttle.c
v4:
fix suggestions in block/throttle.c
fix suggestions in block/throttle_groups.c
add doc note in BlockDevOptionsThrottle
v3:
fix style error in 'add aio_context field in ThrottleGroupMember'
v2:
change QOM throttle group object name
print valid ranges for uint on error
move frees in throttle_group_obj_finalize()
split throttle_group_{set,get}()
add throttle_recurse_is_first_non_filter()
Manos Pitsidianakis (6):
block: move ThrottleGroup membership to ThrottleGroupMember
block: add aio_context field in ThrottleGroupMember
block: tidy ThrottleGroupMember initializations
block: convert ThrottleGroup to object with QOM
block: add throttle block filter driver
qemu-iotests: add 184 for throttle filter driver
qapi/block-core.json| 66 +++-
include/block/throttle-groups.h | 48 ++-
include/qemu/throttle-options.h | 60 ++--
include/qemu/throttle.h | 3 +
include/sysemu/block-backend.h | 20 +-
block/block-backend.c | 62 ++--
block/qapi.c| 8 +-
block/throttle-groups.c | 736 +---
block/throttle.c| 250 ++
blockdev.c | 4 +-
tests/test-throttle.c | 111 +++---
util/throttle.c | 151 +
block/Makefile.objs | 1 +
tests/qemu-iotests/184 | 205 +++
tests/qemu-iotests/184.out | 300
tests/qemu-iotests/group| 1 +
16 files changed, 1705 insertions(+), 321 deletions(-)
create mode 100644 block/throttle.c
create mode 100755 tests/qemu-iotests/184
create mode 100644 tests/qemu-iotests/184.out
--
2.11.0
[Qemu-block] [PATCH v8 4/6] block: convert ThrottleGroup to object with QOM
ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.
A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.
ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.
ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct. It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:
{ "execute": "object-add",
"arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
"limits": {
"iops-total": 100
}
}
}
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}
This also means a group's configuration can be fetched with qom-get.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 48 +
include/block/throttle-groups.h | 3 +
include/qemu/throttle-options.h | 59 --
include/qemu/throttle.h | 3 +
block/throttle-groups.c | 413
tests/test-throttle.c | 1 +
util/throttle.c | 151 +++
7 files changed, 618 insertions(+), 60 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602150..0bdc69aa5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1905,6 +1905,54 @@
'*iops_size': 'int', '*group': 'str' } }
##
+# @ThrottleLimits:
+#
+# Limit parameters for throttling.
+# Since some limit combinations are illegal, limits should always be set in one
+# transaction. All fields are optional. When setting limits, if a field is
+# missing the current value is not changed.
+#
+# @iops-total: limit total I/O operations per second
+# @iops-total-max: I/O operations burst
+# @iops-total-max-length: length of the iops-total-max burst period, in
seconds
+# It must only be set if @iops-total-max is set as
well.
+# @iops-read: limit read operations per second
+# @iops-read-max: I/O operations read burst
+# @iops-read-max-length: length of the iops-read-max burst period, in seconds
+# It must only be set if @iops-read-max is set as
well.
+# @iops-write: limit write operations per second
+# @iops-write-max: I/O operations write burst
+# @iops-write-max-length: length of the iops-write-max burst period, in
seconds
+# It must only be set if @iops-write-max is set as
well.
+# @bps-total: limit total bytes per second
+# @bps-total-max: total bytes burst
+# @bps-total-max-length: length of the bps-total-max burst period, in
seconds.
+# It must only be set if @bps-total-max is set as
well.
+# @bps-read: limit read bytes per second
+# @bps-read-max: total bytes read burst
+# @bps-read-max-length:length of the bps-read-max burst period, in seconds
+# It must only be set if @bps-read-max is set as well.
+# @bps-write: limit write bytes per second
+# @bps-write-max: total bytes write burst
+# @bps-write-max-length: length of the bps-write-max burst period, in seconds
+# It must only be set if @bps-write-max is set as
well.
+# @iops-size: when limiting by iops max size of an I/O in bytes
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleLimits',
+ 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
+'*iops-total-max-length' : 'int', '*iops-read' : 'int',
+'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
+'*iops-write' : 'int', '*iops-write-max' : 'int',
+'*iops-write-max-length' : 'int', '*bps-total' : 'int',
+'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
+'*bps-read' : 'int', '*bps-read-max' : 'int',
+'*bps-read-max-length' : 'int', '*bps-write' : 'int',
+'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
+
[Qemu-block] [PATCH v8 5/6] block: add throttle block filter driver
block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c
The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
which registers the throttle filter node with the ThrottleGroup 'bar'. The
given group must be created beforehand with object-add or -object.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 18 ++-
include/block/throttle-groups.h | 1 +
include/qemu/throttle-options.h | 1 +
block/throttle-groups.c | 12 +-
block/throttle.c| 250
block/Makefile.objs | 1 +
6 files changed, 281 insertions(+), 2 deletions(-)
create mode 100644 block/throttle.c
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..03724146a4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
# Drivers that are supported in block device operations.
#
# @vxhs: Since 2.10
+# @throttle: Since 2.11
#
# Since: 2.9
##
@@ -2231,7 +2232,7 @@
'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
##
# @BlockdevOptionsFile:
@@ -3095,6 +3096,20 @@
'*tls-creds': 'str' } }
##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group: the name of the throttle-group object to use. It
+#must already exist.
+# @file: reference to or definition of the data source block device
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+ 'data': { 'throttle-group': 'str',
+'file' : 'BlockdevRef'
+ } }
+##
# @BlockdevOptions:
#
# Options for creating a block device. Many options are available for all
@@ -3155,6 +3170,7 @@
'replication':'BlockdevOptionsReplication',
'sheepdog': 'BlockdevOptionsSheepdog',
'ssh':'BlockdevOptionsSsh',
+ 'throttle': 'BlockdevOptionsThrottle',
'vdi':'BlockdevOptionsGenericFormat',
'vhdx': 'BlockdevOptionsGenericFormat',
'vmdk': 'BlockdevOptionsGenericCOWFormat',
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..ec969e84fe 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,6 @@ void coroutine_fn
throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+bool throttle_group_exists(const char *name);
#endif
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 182b7896e1..3528a8f4a2 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -29,6 +29,7 @@
#define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
#define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
#define QEMU_OPT_IOPS_SIZE "iops-size"
+#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
#define THROTTLE_OPT_PREFIX "throttling."
#define THROTTLE_OPTS \
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 8421921bed..98ec39df3f 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -101,6 +101,11 @@ static ThrottleGroup *throttle_group_by_name(const char
*name)
return NULL;
}
+bool throttle_group_exists(const char *name)
+{
+return throttle_group_by_name(name) != NULL;
+}
+
/* Increments the reference count of a ThrottleGroup given its name.
*
* If no ThrottleGroup is found with the given name a new one is
@@ -543,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember
*tgm)
ThrottleGroupMember *token;
int i;
+if (!ts) {
+/* Discard already unregistered tgm */
+return;
+}
+
assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
assert(qemu_co_queue_empty(>throttled_reqs[0]));
assert(qemu_co_queue_empty(>throttled_reqs[1]));
@@ -709,7 +719,7 @@ static void throttle_group_obj_complete(UserCreatable *obj,
Error **errp)
assert(tg->name);
/* error if name is duplicate */
-if (throttle_group_by_name(tg->name) != NULL) {
+if (throttle_group_exists(tg->name)) {
error_setg(errp, "A group with this name already exists");
return;
}
diff --git a/block/throttle.c
[Qemu-block] [PATCH v8 3/6] block: tidy ThrottleGroupMember initializations
Move the CoMutex and CoQueue inits inside throttle_group_register_tgm() which is called whenever a ThrottleGroupMember is initialized. There's no need for them to be separate. Reviewed-by: Alberto Garcia <be...@igalia.com> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> --- block/block-backend.c | 3 --- block/throttle-groups.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 9ba800e733..c51fb8c8aa 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -252,9 +252,6 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm) blk->shared_perm = shared_perm; blk_set_enable_write_cache(blk, true); -qemu_co_mutex_init(>public.throttle_group_member.throttled_reqs_lock); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[0]); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[1]); block_acct_init(>stats); notifier_list_init(>remove_bs_notifiers); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 3b07b25f39..7749cf043f 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -508,6 +508,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, read_timer_cb, write_timer_cb, tgm); +qemu_co_mutex_init(>throttled_reqs_lock); +qemu_co_queue_init(>throttled_reqs[0]); +qemu_co_queue_init(>throttled_reqs[1]); qemu_mutex_unlock(>lock); } -- 2.11.0
[Qemu-block] [PATCH v8 1/6] block: move ThrottleGroup membership to ThrottleGroupMember
This commit eliminates the 1:1 relationship between BlockBackend and
throttle group state. Users will be able to create multiple throttle
nodes, each with its own throttle group state, in the future. The
throttle group state cannot be per-BlockBackend anymore, it must be
per-throttle node. This is done by gathering ThrottleGroup membership
details from BlockBackendPublic into ThrottleGroupMember and refactoring
existing code to use the structure.
Reviewed-by: Alberto Garcia <be...@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
include/block/throttle-groups.h | 39 +-
include/sysemu/block-backend.h | 20 +--
block/block-backend.c | 66 +
block/qapi.c| 8 +-
block/throttle-groups.c | 288
blockdev.c | 4 +-
tests/test-throttle.c | 53
7 files changed, 252 insertions(+), 226 deletions(-)
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index d983d34074..1a6bcdae74 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -28,19 +28,44 @@
#include "qemu/throttle.h"
#include "block/block_int.h"
-const char *throttle_group_get_name(BlockBackend *blk);
+/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup
+ * and holds related data.
+ */
+
+typedef struct ThrottleGroupMember {
+/* throttled_reqs_lock protects the CoQueues for throttled requests. */
+CoMutex throttled_reqs_lock;
+CoQueue throttled_reqs[2];
+
+/* Nonzero if the I/O limits are currently being ignored; generally
+ * it is zero. Accessed with atomic operations.
+ */
+unsigned int io_limits_disabled;
+
+/* The following fields are protected by the ThrottleGroup lock.
+ * See the ThrottleGroup documentation for details.
+ * throttle_state tells us if I/O limits are configured. */
+ThrottleState *throttle_state;
+ThrottleTimers throttle_timers;
+unsigned pending_reqs[2];
+QLIST_ENTRY(ThrottleGroupMember) round_robin;
+
+} ThrottleGroupMember;
+
+const char *throttle_group_get_name(ThrottleGroupMember *tgm);
ThrottleState *throttle_group_incref(const char *name);
void throttle_group_unref(ThrottleState *ts);
-void throttle_group_config(BlockBackend *blk, ThrottleConfig *cfg);
-void throttle_group_get_config(BlockBackend *blk, ThrottleConfig *cfg);
+void throttle_group_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
+void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
-void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
-void throttle_group_unregister_blk(BlockBackend *blk);
-void throttle_group_restart_blk(BlockBackend *blk);
+void throttle_group_register_tgm(ThrottleGroupMember *tgm,
+const char *groupname);
+void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
+void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
-void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk,
+void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember
*tgm,
unsigned int bytes,
bool is_write);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a3730596b..0e0cda7521 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -70,24 +70,10 @@ typedef struct BlockDevOps {
/* This struct is embedded in (the private) BlockBackend struct and contains
* fields that must be public. This is in particular for QLIST_ENTRY() and
- * friends so that BlockBackends can be kept in lists outside block-backend.c
*/
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ * */
typedef struct BlockBackendPublic {
-/* throttled_reqs_lock protects the CoQueues for throttled requests. */
-CoMutex throttled_reqs_lock;
-CoQueue throttled_reqs[2];
-
-/* Nonzero if the I/O limits are currently being ignored; generally
- * it is zero. Accessed with atomic operations.
- */
-unsigned int io_limits_disabled;
-
-/* The following fields are protected by the ThrottleGroup lock.
- * See the ThrottleGroup documentation for details.
- * throttle_state tells us if I/O limits are configured. */
-ThrottleState *throttle_state;
-ThrottleTimers throttle_timers;
-unsigned pending_reqs[2];
-QLIST_ENTRY(BlockBackendPublic) round_robin;
+ThrottleGroupMember throttle_group_member;
} BlockBackendPublic;
BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
diff --git a/block/block-backend.c b/block/block-backend.c
index e9798e897d..7b981e00bb 100644
--- a/block/block
Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver
On Tue, Aug 22, 2017 at 04:16:26PM +0200, Alberto Garcia wrote:
On Tue 22 Aug 2017 12:15:34 PM CEST, Manos Pitsidianakis wrote:
@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember
*tgm)
ThrottleGroupMember *token;
int i;
+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+
Is this change needed in case throttle_configure_tgm() fails?
Yes, now all errors are before throttle_group_register_tgm(), therefore
the unregister part in throttle_close() will not have valid data.
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM
On Tue, Aug 22, 2017 at 03:25:41PM +0200, Alberto Garcia wrote:
+/* This function edits throttle_groups and must be called under the global
+ * mutex */
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
+{
+ThrottleGroup *tg = THROTTLE_GROUP(obj);
+ThrottleConfig cfg;
+
+/* set group name to object id if it exists */
+if (!tg->name && tg->parent_obj.parent) {
+tg->name = object_get_canonical_path_component(OBJECT(obj));
+}
+/* We must have a group name at this point */
+assert(tg->name);
+
+/* error if name is duplicate */
+if (throttle_group_exists(tg->name)) {
+error_setg(errp, "A group with this name already exists");
+return;
+}
+
+/* check validity */
+throttle_get_config(>ts, );
+if (!throttle_is_valid(, errp)) {
+return;
+}
My fault here because I suggested its removal, but I actually think we
still need to call throttle_config() here so bkt->max is initialized in
throttle_fix_bucket(). I'll take care of updating this call once my
patches to remove throttle_fix_bucket() are applied, but for now we
still need it.
Oh yes, that is the reason I originally had done it but forgot why.
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v2 3/6] block: require job-id when device is a node name
On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
-if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
-job_id = bdrv_get_device_name(bs);
-if (!*job_id) {
-error_setg(errp, "An explicit job ID is required for this node");
-return NULL;
-}
-}
-
-if (job_id) {
-if (flags & BLOCK_JOB_INTERNAL) {
+if (flags & BLOCK_JOB_INTERNAL) {
+if (job_id) {
error_setg(errp, "Cannot specify job ID for internal block job");
return NULL;
}
When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
-
+} else {
+/* Require job-id. */
+assert(job_id);
if (!id_wellformed(job_id)) {
error_setg(errp, "Invalid job ID '%s'", job_id);
return NULL;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..ff906808a6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,7 @@ struct BlockJobDriver {
/**
* block_job_create:
- * @job_id: The id of the newly-created job, or %NULL to have one
- * generated automatically.
+ * @job_id: The id of the newly-created job, must be non %NULL.
...but here you say that it must not be NULL.
And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().
@@ -93,9 +93,6 @@ static void test_job_ids(void)
blk[1] = create_blk("drive1");
blk[2] = create_blk("drive2");
-/* No job ID provided and the block backend has no name */
-job[0] = do_test_id(blk[0], NULL, false);
-
If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.
Berto
Thanks, I will change that. What cases are you thinking of besides
internal jobs though?
Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.
I thought we handled the external job case by requiring job_id is set
before calling block_job_create(). I will check my patch again.
And should documentation of block_job_create reflect that internal
jobs have job_id == NULL?
Yes, it should document the restrictions of the job_id parameter.
Berto
signature.asc
Description: PGP signature
[Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver
block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c
The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
which registers the throttle filter node with the ThrottleGroup 'bar'. The
given group must be created beforehand with object-add or -object.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 18 ++-
include/block/throttle-groups.h | 1 +
include/qemu/throttle-options.h | 1 +
block/throttle-groups.c | 7 +-
block/throttle.c| 250
block/Makefile.objs | 1 +
6 files changed, 276 insertions(+), 2 deletions(-)
create mode 100644 block/throttle.c
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..405c181954 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
# Drivers that are supported in block device operations.
#
# @vxhs: Since 2.10
+# @throttle: Since 2.11
#
# Since: 2.9
##
@@ -2231,7 +2232,7 @@
'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
##
# @BlockdevOptionsFile:
@@ -3095,6 +3096,20 @@
'*tls-creds': 'str' } }
##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group: the name of the throttle-group object to use. It
+#must already exist.
+# @file: reference to or definition of the data source block device
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+ 'data': { '*throttle-group': 'str',
+'file' : 'BlockdevRef'
+ } }
+##
# @BlockdevOptions:
#
# Options for creating a block device. Many options are available for all
@@ -3155,6 +3170,7 @@
'replication':'BlockdevOptionsReplication',
'sheepdog': 'BlockdevOptionsSheepdog',
'ssh':'BlockdevOptionsSsh',
+ 'throttle': 'BlockdevOptionsThrottle',
'vdi':'BlockdevOptionsGenericFormat',
'vhdx': 'BlockdevOptionsGenericFormat',
'vmdk': 'BlockdevOptionsGenericCOWFormat',
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..ec969e84fe 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,6 @@ void coroutine_fn
throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+bool throttle_group_exists(const char *name);
#endif
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 182b7896e1..3528a8f4a2 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -29,6 +29,7 @@
#define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
#define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
#define QEMU_OPT_IOPS_SIZE "iops-size"
+#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
#define THROTTLE_OPT_PREFIX "throttling."
#define THROTTLE_OPTS \
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index a4268a954e..4b483a16d4 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -101,7 +101,7 @@ static ThrottleGroup *throttle_group_by_name(const char
*name)
return NULL;
}
-static bool throttle_group_exists(const char *name)
+bool throttle_group_exists(const char *name)
{
return throttle_group_by_name(name) ? true : false;
}
@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember
*tgm)
ThrottleGroupMember *token;
int i;
+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+
assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
assert(qemu_co_queue_empty(>throttled_reqs[0]));
assert(qemu_co_queue_empty(>throttled_reqs[1]));
diff --git a/block/throttle.c b/block/throttle.c
new file mode 100644
index 00..910de27dcd
--- /dev/null
+++ b/block/throttle.c
@@ -0,0 +1,250 @@
+/*
+ * QEMU block throttling filter driver infrastructure
+ *
+ * Copyright (c) 2017 Manos Pitsidianakis
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your optio
[Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM
ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.
A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.
ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.
ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct. It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:
{ "execute": "object-add",
"arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
"limits": {
"iops-total": 100
}
}
}
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}
This also means a group's configuration can be fetched with qom-get.
Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>
---
qapi/block-core.json| 48 +
include/block/throttle-groups.h | 3 +
include/qemu/throttle-options.h | 59 --
include/qemu/throttle.h | 3 +
block/throttle-groups.c | 419
tests/test-throttle.c | 1 +
util/throttle.c | 151 +++
7 files changed, 624 insertions(+), 60 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602150..0bdc69aa5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1905,6 +1905,54 @@
'*iops_size': 'int', '*group': 'str' } }
##
+# @ThrottleLimits:
+#
+# Limit parameters for throttling.
+# Since some limit combinations are illegal, limits should always be set in one
+# transaction. All fields are optional. When setting limits, if a field is
+# missing the current value is not changed.
+#
+# @iops-total: limit total I/O operations per second
+# @iops-total-max: I/O operations burst
+# @iops-total-max-length: length of the iops-total-max burst period, in
seconds
+# It must only be set if @iops-total-max is set as
well.
+# @iops-read: limit read operations per second
+# @iops-read-max: I/O operations read burst
+# @iops-read-max-length: length of the iops-read-max burst period, in seconds
+# It must only be set if @iops-read-max is set as
well.
+# @iops-write: limit write operations per second
+# @iops-write-max: I/O operations write burst
+# @iops-write-max-length: length of the iops-write-max burst period, in
seconds
+# It must only be set if @iops-write-max is set as
well.
+# @bps-total: limit total bytes per second
+# @bps-total-max: total bytes burst
+# @bps-total-max-length: length of the bps-total-max burst period, in
seconds.
+# It must only be set if @bps-total-max is set as
well.
+# @bps-read: limit read bytes per second
+# @bps-read-max: total bytes read burst
+# @bps-read-max-length:length of the bps-read-max burst period, in seconds
+# It must only be set if @bps-read-max is set as well.
+# @bps-write: limit write bytes per second
+# @bps-write-max: total bytes write burst
+# @bps-write-max-length: length of the bps-write-max burst period, in seconds
+# It must only be set if @bps-write-max is set as
well.
+# @iops-size: when limiting by iops max size of an I/O in bytes
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleLimits',
+ 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
+'*iops-total-max-length' : 'int', '*iops-read' : 'int',
+'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
+'*iops-write' : 'int', '*iops-write-max' : 'int',
+'*iops-write-max-length' : 'int', '*bps-total' : 'int',
+'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
+'*bps-read' : 'int', '*bps-read-max' : 'int',
+'*bps-read-max-length' : 'int', '*bps-write' : 'int',
+'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
+
[Qemu-block] [PATCH v7 3/6] block: tidy ThrottleGroupMember initializations
Move the CoMutex and CoQueue inits inside throttle_group_register_tgm() which is called whenever a ThrottleGroupMember is initialized. There's no need for them to be separate. Reviewed-by: Alberto Garcia <be...@igalia.com> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> --- block/block-backend.c | 3 --- block/throttle-groups.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 9ba800e733..c51fb8c8aa 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -252,9 +252,6 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm) blk->shared_perm = shared_perm; blk_set_enable_write_cache(blk, true); -qemu_co_mutex_init(>public.throttle_group_member.throttled_reqs_lock); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[0]); -qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[1]); block_acct_init(>stats); notifier_list_init(>remove_bs_notifiers); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 3b07b25f39..7749cf043f 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -508,6 +508,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, read_timer_cb, write_timer_cb, tgm); +qemu_co_mutex_init(>throttled_reqs_lock); +qemu_co_queue_init(>throttled_reqs[0]); +qemu_co_queue_init(>throttled_reqs[1]); qemu_mutex_unlock(>lock); } -- 2.11.0
