* [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 17:36 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 2/9] block: User BdrvChild callback for device name Kevin Wolf
` (8 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
We want to get rid of BlockDriverState.blk in order to allow multiple
BlockBackends per BDS. Converting the device callbacks in block.c (which
assume a single BlockBackend) to per-child callbacks gets us rid of the
first few instances.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 38 +++++++++++++++++++++++++-------------
block/block-backend.c | 15 ++++++++++++++-
include/block/block_int.h | 4 +++-
3 files changed, 42 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index fb37b91..1fb5dac 100644
--- a/block.c
+++ b/block.c
@@ -1216,6 +1216,27 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
bdrv_root_unref_child(child);
}
+
+static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool load)
+{
+ BdrvChild *c;
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->change_media) {
+ c->role->change_media(c, load);
+ }
+ }
+}
+
+static void bdrv_parent_cb_resize(BlockDriverState *bs)
+{
+ BdrvChild *c;
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->resize) {
+ c->role->resize(c);
+ }
+ }
+}
+
/*
* Sets the backing file link of a BDS. A new reference is created; callers
* which don't need their own reference any more must call bdrv_unref().
@@ -1674,9 +1695,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
}
if (!bdrv_key_required(bs)) {
- if (bs->blk) {
- blk_dev_change_media_cb(bs->blk, true);
- }
+ bdrv_parent_cb_change_media(bs, true);
} else if (!runstate_check(RUN_STATE_PRELAUNCH)
&& !runstate_check(RUN_STATE_INMIGRATE)
&& !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
@@ -2122,9 +2141,7 @@ static void bdrv_close(BlockDriverState *bs)
bdrv_release_named_dirty_bitmaps(bs);
assert(QLIST_EMPTY(&bs->dirty_bitmaps));
- if (bs->blk) {
- blk_dev_change_media_cb(bs->blk, false);
- }
+ bdrv_parent_cb_change_media(bs, false);
if (bs->drv) {
BdrvChild *child, *next;
@@ -2605,9 +2622,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
if (ret == 0) {
ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
bdrv_dirty_bitmap_truncate(bs);
- if (bs->blk) {
- blk_dev_resize_cb(bs->blk);
- }
+ bdrv_parent_cb_resize(bs);
}
return ret;
}
@@ -2718,10 +2733,7 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
bs->valid_key = 0;
} else if (!bs->valid_key) {
bs->valid_key = 1;
- if (bs->blk) {
- /* call the change callback now, we skipped it on open */
- blk_dev_change_media_cb(bs->blk, true);
- }
+ bdrv_parent_cb_change_media(bs, true);
}
return ret;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index 8b4eb1a..9d973ba 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -92,9 +92,15 @@ static void blk_root_inherit_options(int *child_flags, QDict *child_options,
}
static bool blk_drain_throttling_queue(BdrvChild *child);
+static void blk_root_change_media(BdrvChild *child, bool load);
+static void blk_root_resize(BdrvChild *child);
+
static const BdrvChildRole child_root = {
.inherit_options = blk_root_inherit_options,
+ .change_media = blk_root_change_media,
+ .resize = blk_root_resize,
+
.drain_queue = blk_drain_throttling_queue,
};
@@ -553,6 +559,11 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load)
}
}
+static void blk_root_change_media(BdrvChild *child, bool load)
+{
+ blk_dev_change_media_cb(child->opaque, load);
+}
+
/*
* Does @blk's attached device model have removable media?
* %true if no device model is attached.
@@ -607,8 +618,10 @@ bool blk_dev_is_medium_locked(BlockBackend *blk)
/*
* Notify @blk's attached device model of a backend size change.
*/
-void blk_dev_resize_cb(BlockBackend *blk)
+static void blk_root_resize(BdrvChild *child)
{
+ BlockBackend *blk = child->opaque;
+
if (blk->dev_ops && blk->dev_ops->resize_cb) {
blk->dev_ops->resize_cb(blk->dev_opaque);
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 000d2c0..f60cb7c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,6 +356,9 @@ struct BdrvChildRole {
void (*inherit_options)(int *child_flags, QDict *child_options,
int parent_flags, QDict *parent_options);
+ void (*change_media)(BdrvChild *child, bool load);
+ void (*resize)(BdrvChild *child);
+
bool (*drain_queue)(BdrvChild *child);
};
@@ -695,7 +698,6 @@ bool blk_dev_has_tray(BlockBackend *blk);
void blk_dev_eject_request(BlockBackend *blk, bool force);
bool blk_dev_is_tray_open(BlockBackend *blk);
bool blk_dev_is_medium_locked(BlockBackend *blk);
-void blk_dev_resize_cb(BlockBackend *blk);
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
bool bdrv_requests_pending(BlockDriverState *bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize
2016-03-22 19:36 ` [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize Kevin Wolf
@ 2016-03-29 17:36 ` Max Reitz
0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-29 17:36 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 6019 bytes --]
On 22.03.2016 20:36, Kevin Wolf wrote:
> We want to get rid of BlockDriverState.blk in order to allow multiple
> BlockBackends per BDS. Converting the device callbacks in block.c (which
> assume a single BlockBackend) to per-child callbacks gets us rid of the
> first few instances.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 38 +++++++++++++++++++++++++-------------
> block/block-backend.c | 15 ++++++++++++++-
> include/block/block_int.h | 4 +++-
> 3 files changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index fb37b91..1fb5dac 100644
> --- a/block.c
> +++ b/block.c
> @@ -1216,6 +1216,27 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
> bdrv_root_unref_child(child);
> }
>
> +
> +static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool load)
> +{
> + BdrvChild *c;
> + QLIST_FOREACH(c, &bs->parents, next_parent) {
> + if (c->role->change_media) {
> + c->role->change_media(c, load);
> + }
> + }
> +}
> +
> +static void bdrv_parent_cb_resize(BlockDriverState *bs)
> +{
> + BdrvChild *c;
> + QLIST_FOREACH(c, &bs->parents, next_parent) {
> + if (c->role->resize) {
> + c->role->resize(c);
> + }
> + }
> +}
> +
> /*
> * Sets the backing file link of a BDS. A new reference is created; callers
> * which don't need their own reference any more must call bdrv_unref().
> @@ -1674,9 +1695,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
> }
>
> if (!bdrv_key_required(bs)) {
> - if (bs->blk) {
> - blk_dev_change_media_cb(bs->blk, true);
> - }
> + bdrv_parent_cb_change_media(bs, true);
> } else if (!runstate_check(RUN_STATE_PRELAUNCH)
> && !runstate_check(RUN_STATE_INMIGRATE)
> && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> @@ -2122,9 +2141,7 @@ static void bdrv_close(BlockDriverState *bs)
> bdrv_release_named_dirty_bitmaps(bs);
> assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>
> - if (bs->blk) {
> - blk_dev_change_media_cb(bs->blk, false);
> - }
> + bdrv_parent_cb_change_media(bs, false);
This is probably unnecessary altogether, but I'm certain about it after
http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00349.html.
So I'll take a note of removing this then.
> if (bs->drv) {
> BdrvChild *child, *next;
> @@ -2605,9 +2622,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> if (ret == 0) {
> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> bdrv_dirty_bitmap_truncate(bs);
> - if (bs->blk) {
> - blk_dev_resize_cb(bs->blk);
> - }
> + bdrv_parent_cb_resize(bs);
> }
> return ret;
> }
> @@ -2718,10 +2733,7 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
> bs->valid_key = 0;
> } else if (!bs->valid_key) {
> bs->valid_key = 1;
> - if (bs->blk) {
> - /* call the change callback now, we skipped it on open */
> - blk_dev_change_media_cb(bs->blk, true);
> - }
> + bdrv_parent_cb_change_media(bs, true);
Not sure why you removed the comment here. In case you send a v2, I'd
keep it.
Whichever way you decide:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> }
> return ret;
> }
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 8b4eb1a..9d973ba 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -92,9 +92,15 @@ static void blk_root_inherit_options(int *child_flags, QDict *child_options,
> }
> static bool blk_drain_throttling_queue(BdrvChild *child);
>
> +static void blk_root_change_media(BdrvChild *child, bool load);
> +static void blk_root_resize(BdrvChild *child);
> +
> static const BdrvChildRole child_root = {
> .inherit_options = blk_root_inherit_options,
>
> + .change_media = blk_root_change_media,
> + .resize = blk_root_resize,
> +
> .drain_queue = blk_drain_throttling_queue,
> };
>
> @@ -553,6 +559,11 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load)
> }
> }
>
> +static void blk_root_change_media(BdrvChild *child, bool load)
> +{
> + blk_dev_change_media_cb(child->opaque, load);
> +}
> +
> /*
> * Does @blk's attached device model have removable media?
> * %true if no device model is attached.
> @@ -607,8 +618,10 @@ bool blk_dev_is_medium_locked(BlockBackend *blk)
> /*
> * Notify @blk's attached device model of a backend size change.
> */
> -void blk_dev_resize_cb(BlockBackend *blk)
> +static void blk_root_resize(BdrvChild *child)
> {
> + BlockBackend *blk = child->opaque;
> +
> if (blk->dev_ops && blk->dev_ops->resize_cb) {
> blk->dev_ops->resize_cb(blk->dev_opaque);
> }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 000d2c0..f60cb7c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -356,6 +356,9 @@ struct BdrvChildRole {
> void (*inherit_options)(int *child_flags, QDict *child_options,
> int parent_flags, QDict *parent_options);
>
> + void (*change_media)(BdrvChild *child, bool load);
> + void (*resize)(BdrvChild *child);
> +
> bool (*drain_queue)(BdrvChild *child);
> };
>
> @@ -695,7 +698,6 @@ bool blk_dev_has_tray(BlockBackend *blk);
> void blk_dev_eject_request(BlockBackend *blk, bool force);
> bool blk_dev_is_tray_open(BlockBackend *blk);
> bool blk_dev_is_medium_locked(BlockBackend *blk);
> -void blk_dev_resize_cb(BlockBackend *blk);
>
> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> bool bdrv_requests_pending(BlockDriverState *bs);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 2/9] block: User BdrvChild callback for device name
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
2016-03-22 19:36 ` [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 17:43 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations Kevin Wolf
` (7 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
In order to get rid of bs->blk for bdrv_get_device_name() and
bdrv_get_device_or_node_name(), ask all parents for their name and
simply pick the first one.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 22 ++++++++++++++++++++--
block/block-backend.c | 6 ++++++
include/block/block_int.h | 1 +
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 1fb5dac..4cc117d 100644
--- a/block.c
+++ b/block.c
@@ -2924,10 +2924,28 @@ const char *bdrv_get_node_name(const BlockDriverState *bs)
return bs->node_name;
}
+static const char *bdrv_get_parent_name(const BlockDriverState *bs)
+{
+ BdrvChild *c;
+ const char *name;
+
+ /* If multiple parents have a name, just pick the first one. */
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->get_name) {
+ name = c->role->get_name(c);
+ if (name && *name) {
+ return name;
+ }
+ }
+ }
+
+ return NULL;
+}
+
/* TODO check what callers really want: bs->node_name or blk_name() */
const char *bdrv_get_device_name(const BlockDriverState *bs)
{
- return bs->blk ? blk_name(bs->blk) : "";
+ return bdrv_get_parent_name(bs) ?: "";
}
/* This can be used to identify nodes that might not have a device
@@ -2936,7 +2954,7 @@ const char *bdrv_get_device_name(const BlockDriverState *bs)
* absent, then this returns an empty (non-null) string. */
const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)
{
- return bs->blk ? blk_name(bs->blk) : bs->node_name;
+ return bdrv_get_parent_name(bs) ?: bs->node_name;
}
int bdrv_get_flags(BlockDriverState *bs)
diff --git a/block/block-backend.c b/block/block-backend.c
index 9d973ba..c4c0dc0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -95,11 +95,17 @@ static bool blk_drain_throttling_queue(BdrvChild *child);
static void blk_root_change_media(BdrvChild *child, bool load);
static void blk_root_resize(BdrvChild *child);
+static const char *blk_root_get_name(BdrvChild *child)
+{
+ return blk_name(child->opaque);
+}
+
static const BdrvChildRole child_root = {
.inherit_options = blk_root_inherit_options,
.change_media = blk_root_change_media,
.resize = blk_root_resize,
+ .get_name = blk_root_get_name,
.drain_queue = blk_drain_throttling_queue,
};
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f60cb7c..195abe8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -358,6 +358,7 @@ struct BdrvChildRole {
void (*change_media)(BdrvChild *child, bool load);
void (*resize)(BdrvChild *child);
+ const char* (*get_name)(BdrvChild *child);
bool (*drain_queue)(BdrvChild *child);
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 2/9] block: User BdrvChild callback for device name
2016-03-22 19:36 ` [Qemu-devel] [PATCH 2/9] block: User BdrvChild callback for device name Kevin Wolf
@ 2016-03-29 17:43 ` Max Reitz
0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-29 17:43 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1097 bytes --]
On 22.03.2016 20:36, Kevin Wolf wrote:
> In order to get rid of bs->blk for bdrv_get_device_name() and
> bdrv_get_device_or_node_name(), ask all parents for their name and
> simply pick the first one.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 22 ++++++++++++++++++++--
> block/block-backend.c | 6 ++++++
> include/block/block_int.h | 1 +
> 3 files changed, 27 insertions(+), 2 deletions(-)
[...]
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f60cb7c..195abe8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -358,6 +358,7 @@ struct BdrvChildRole {
>
> void (*change_media)(BdrvChild *child, bool load);
> void (*resize)(BdrvChild *child);
> + const char* (*get_name)(BdrvChild *child);
>
> bool (*drain_queue)(BdrvChild *child);
> };
I wouldn't mind an explanation there what this is supposed to be used
for. From the name alone I'd expect a parent BDS to return its node name.
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
2016-03-22 19:36 ` [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize Kevin Wolf
2016-03-22 19:36 ` [Qemu-devel] [PATCH 2/9] block: User BdrvChild callback for device name Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:15 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 4/9] block: Remove bdrv_aio_multiwrite() Kevin Wolf
` (6 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
The block jobs currently modify the target BB's error handling options
and require that the source BB's iostatus is enabled in order to
implement the per-job error options. It's obvious that this is something
between ugly, adventurous and plain wrong, so we should ideally fix this
instead of thinking about how to handle multiple BBs in this case.
Unfortunately we have a chicken-and-egg problem here: In order to fix
the problem, the block jobs need their own BBs. This requires multiple
BBs per BDS, which in turn means that BDS.blk must be removed. Removing
BDS.blk means that the jobs already need their own BB. The only other
options is that we lift the iostatus operations to the BdrvChild level
and deal with multiple BBs now.
So even though we don't want to have these callbacks in the end (because
they indicate broken logic), this patch converts the iostatus operations
for block jobs targets to BdrvChild callbacks; if more than one parent
implements iostatus operations, they are called for each of them.
Once the conversion is completed, block jobs will get their own internal
BB and the callbacks can be removed again.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/backup.c | 20 +++++------
block/block-backend.c | 42 +++++++++++++++++++++++
block/commit.c | 2 +-
block/mirror.c | 21 ++++++------
block/stream.c | 2 +-
blockjob.c | 85 +++++++++++++++++++++++++++++++++++++++++++++--
include/block/block_int.h | 10 ++++++
include/block/blockjob.h | 9 +++++
8 files changed, 165 insertions(+), 26 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 10397e2..016741e 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -220,9 +220,8 @@ static void backup_iostatus_reset(BlockJob *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common);
- if (s->target->blk) {
- blk_iostatus_reset(s->target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_reset(s->target);
}
static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
@@ -402,10 +401,9 @@ static void coroutine_fn backup_run(void *opaque)
job->done_bitmap = bitmap_new(end);
- if (target->blk) {
- blk_set_on_error(target->blk, on_target_error, on_target_error);
- blk_iostatus_enable(target->blk);
- }
+ /* FIXME Only touch on_error and iostatus of a job-owned BB */
+ bdrv_set_on_error(target, on_target_error, on_target_error);
+ bdrv_iostatus_enable(target);
bdrv_add_before_write_notifier(bs, &before_write);
@@ -482,9 +480,9 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_unlock(&job->flush_rwlock);
g_free(job->done_bitmap);
- if (target->blk) {
- blk_iostatus_disable(target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_disable(target);
+
bdrv_op_unblock_all(target, job->common.blocker);
data = g_malloc(sizeof(*data));
@@ -515,7 +513,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
return;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index c4c0dc0..03e68a7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -100,6 +100,39 @@ static const char *blk_root_get_name(BdrvChild *child)
return blk_name(child->opaque);
}
+static void blk_root_iostatus_reset(BdrvChild *child)
+{
+ return blk_iostatus_reset(child->opaque);
+}
+
+static void blk_root_iostatus_enable(BdrvChild *child)
+{
+ return blk_iostatus_enable(child->opaque);
+}
+
+static void blk_root_iostatus_disable(BdrvChild *child)
+{
+ return blk_iostatus_disable(child->opaque);
+}
+
+static bool blk_root_iostatus_is_enabled(BdrvChild *child)
+{
+ return blk_iostatus_is_enabled(child->opaque);
+}
+
+static void blk_root_iostatus_set_err(BdrvChild *child, int error)
+{
+ return blk_iostatus_set_err(child->opaque, error);
+}
+
+static void blk_root_set_on_error(BdrvChild *child,
+ BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error)
+{
+ return blk_set_on_error(child->opaque, on_read_error, on_write_error);
+}
+
+
static const BdrvChildRole child_root = {
.inherit_options = blk_root_inherit_options,
@@ -108,6 +141,15 @@ static const BdrvChildRole child_root = {
.get_name = blk_root_get_name,
.drain_queue = blk_drain_throttling_queue,
+
+ /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+ * it should be controlled by the parents. */
+ .iostatus_reset = blk_root_iostatus_reset,
+ .iostatus_enable = blk_root_iostatus_enable,
+ .iostatus_disable = blk_root_iostatus_disable,
+ .iostatus_is_enabled = blk_root_iostatus_is_enabled,
+ .iostatus_set_err = blk_root_iostatus_set_err,
+ .set_on_error = blk_root_set_on_error,
};
/*
diff --git a/block/commit.c b/block/commit.c
index 446a3ae..5e45195 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -215,7 +215,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, "Invalid parameter combination");
return;
}
diff --git a/block/mirror.c b/block/mirror.c
index da18c0b..b9a93fd 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -689,9 +689,9 @@ immediate_exit:
g_free(s->cow_bitmap);
g_free(s->in_flight_bitmap);
bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
- if (s->target->blk) {
- blk_iostatus_disable(s->target->blk);
- }
+
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_disable(s->target);
data = g_malloc(sizeof(*data));
data->ret = ret;
@@ -716,9 +716,8 @@ static void mirror_iostatus_reset(BlockJob *job)
{
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
- if (s->target->blk) {
- blk_iostatus_reset(s->target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_reset(s->target);
}
static void mirror_complete(BlockJob *job, Error **errp)
@@ -802,7 +801,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
return;
}
@@ -855,10 +854,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
bdrv_op_block_all(s->target, s->common.blocker);
- if (s->target->blk) {
- blk_set_on_error(s->target->blk, on_target_error, on_target_error);
- blk_iostatus_enable(s->target->blk);
- }
+ /* FIXME Only touch on_error and iostatus of a job-owned BB */
+ bdrv_set_on_error(s->target, on_target_error, on_target_error);
+ bdrv_iostatus_enable(s->target);
+
s->common.co = qemu_coroutine_create(mirror_run);
trace_mirror_start(bs, s, s->common.co, opaque);
qemu_coroutine_enter(s->common.co, s);
diff --git a/block/stream.c b/block/stream.c
index cafaa07..d74f97f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -224,7 +224,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-error");
return;
}
diff --git a/blockjob.c b/blockjob.c
index 9fc37ca..e6edd81 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -50,6 +50,87 @@ struct BlockJobTxn {
int refcnt;
};
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_reset) {
+ c->role->iostatus_reset(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_enable) {
+ c->role->iostatus_enable(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_disable(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_disable) {
+ c->role->iostatus_disable(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs)
+{
+ BdrvChild *c;
+ bool ret = false;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_is_enabled) {
+ ret = c->role->iostatus_is_enabled(c);
+ if (!ret) {
+ return false;
+ }
+ }
+ }
+
+ /* Return false if no BB exists; true if BBs exist and all of them have
+ * iostatus enabled */
+ return ret;
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_set_err) {
+ c->role->iostatus_set_err(c, error);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->set_on_error) {
+ c->role->set_on_error(c, on_read_error, on_write_error);
+ }
+ }
+}
+
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
@@ -443,8 +524,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
job->user_paused = true;
block_job_pause(job);
block_job_iostatus_set_err(job, error);
- if (bs->blk && bs != job->bs) {
- blk_iostatus_set_err(bs->blk, error);
+ if (bs != job->bs) {
+ bdrv_iostatus_set_err(bs, error);
}
}
return action;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 195abe8..9358949 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -361,6 +361,16 @@ struct BdrvChildRole {
const char* (*get_name)(BdrvChild *child);
bool (*drain_queue)(BdrvChild *child);
+
+ /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+ * it should be controlled by the parents. */
+ void (*iostatus_reset)(BdrvChild *child);
+ void (*iostatus_enable)(BdrvChild *child);
+ void (*iostatus_disable)(BdrvChild *child);
+ bool (*iostatus_is_enabled)(BdrvChild *child);
+ void (*iostatus_set_err)(BdrvChild *child, int error);
+ void (*set_on_error)(BdrvChild *child, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error);
};
extern const BdrvChildRole child_file;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 8bedc49..33a42c3 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -448,4 +448,13 @@ void block_job_txn_unref(BlockJobTxn *txn);
*/
void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
+/* TODO These are functions that shouldn't exist. Remove all of their users. */
+void bdrv_iostatus_reset(BlockDriverState *bs);
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_disable(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs);
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error);
+
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations
2016-03-22 19:36 ` [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations Kevin Wolf
@ 2016-03-29 18:15 ` Max Reitz
0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-29 18:15 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3207 bytes --]
On 22.03.2016 20:36, Kevin Wolf wrote:
> The block jobs currently modify the target BB's error handling options
> and require that the source BB's iostatus is enabled in order to
> implement the per-job error options. It's obvious that this is something
> between ugly, adventurous and plain wrong, so we should ideally fix this
> instead of thinking about how to handle multiple BBs in this case.
>
> Unfortunately we have a chicken-and-egg problem here: In order to fix
> the problem, the block jobs need their own BBs. This requires multiple
> BBs per BDS, which in turn means that BDS.blk must be removed. Removing
> BDS.blk means that the jobs already need their own BB. The only other
> options is that we lift the iostatus operations to the BdrvChild level
> and deal with multiple BBs now.
>
> So even though we don't want to have these callbacks in the end (because
> they indicate broken logic), this patch converts the iostatus operations
> for block jobs targets to BdrvChild callbacks; if more than one parent
> implements iostatus operations, they are called for each of them.
>
> Once the conversion is completed, block jobs will get their own internal
> BB and the callbacks can be removed again.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/backup.c | 20 +++++------
> block/block-backend.c | 42 +++++++++++++++++++++++
> block/commit.c | 2 +-
> block/mirror.c | 21 ++++++------
> block/stream.c | 2 +-
> blockjob.c | 85 +++++++++++++++++++++++++++++++++++++++++++++--
> include/block/block_int.h | 10 ++++++
> include/block/blockjob.h | 9 +++++
> 8 files changed, 165 insertions(+), 26 deletions(-)
[...]
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c4c0dc0..03e68a7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -100,6 +100,39 @@ static const char *blk_root_get_name(BdrvChild *child)
> return blk_name(child->opaque);
> }
>
> +static void blk_root_iostatus_reset(BdrvChild *child)
> +{
> + return blk_iostatus_reset(child->opaque);
The C11 standard (draft) says in 6.8.6.4 paragraph 1 that:
> A return statement with an expression shall not appear in a function
> whose return type is void.
Even if it is a void expression, it still is an expression. Also, the
semantics of a return statement with an expression is as follows
(paragraph 3):
> If a return statement with an expression is executed, the value of
> the expression is returned to the caller as the value of the function
> call expression.
However, 6.3.2.2 ("void") says:
> The (nonexistent) value of a void expression (an expression that has
> type void) shall not be used in any way
Therefore, the value of a void expression cannot be returned through a
return instruction because such a value does not exist.
I looked it up because I remember you had quite a bit of knowledge
regarding corner cases of the void types. The patch looks good overall,
so if you can point me to why this is allowed or that it's something we
already do somewhere else in qemu, then I'll give my R-b.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 4/9] block: Remove bdrv_aio_multiwrite()
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (2 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:22 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 5/9] block: Avoid BDS.blk in bdrv_next() Kevin Wolf
` (5 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Since virtio-blk implements request merging itself these days, the only
remaining users are test cases for the function. That doesn't make the
function exactly useful any more.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 14 ---
block/io.c | 194 ---------------------------------------
include/block/block.h | 7 +-
include/sysemu/block-backend.h | 1 -
qemu-io-cmds.c | 203 -----------------------------------------
tests/qemu-iotests/100 | 146 -----------------------------
tests/qemu-iotests/100.out | 89 ------------------
tests/qemu-iotests/136 | 20 +---
tests/qemu-iotests/136.out | 4 +-
tests/qemu-iotests/group | 2 +-
trace-events | 2 -
11 files changed, 9 insertions(+), 673 deletions(-)
delete mode 100755 tests/qemu-iotests/100
delete mode 100644 tests/qemu-iotests/100.out
diff --git a/block/block-backend.c b/block/block-backend.c
index 03e68a7..dfc11b5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1140,20 +1140,6 @@ void blk_aio_cancel_async(BlockAIOCB *acb)
bdrv_aio_cancel_async(acb);
}
-int blk_aio_multiwrite(BlockBackend *blk, BlockRequest *reqs, int num_reqs)
-{
- int i, ret;
-
- for (i = 0; i < num_reqs; i++) {
- ret = blk_check_request(blk, reqs[i].sector, reqs[i].nb_sectors);
- if (ret < 0) {
- return ret;
- }
- }
-
- return bdrv_aio_multiwrite(blk_bs(blk), reqs, num_reqs);
-}
-
int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
{
if (!blk_is_available(blk)) {
diff --git a/block/io.c b/block/io.c
index 24bdd6c..6ec133f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1701,200 +1701,6 @@ BlockAIOCB *bdrv_aio_write_zeroes(BlockDriverState *bs,
cb, opaque, true);
}
-
-typedef struct MultiwriteCB {
- int error;
- int num_requests;
- int num_callbacks;
- struct {
- BlockCompletionFunc *cb;
- void *opaque;
- QEMUIOVector *free_qiov;
- } callbacks[];
-} MultiwriteCB;
-
-static void multiwrite_user_cb(MultiwriteCB *mcb)
-{
- int i;
-
- for (i = 0; i < mcb->num_callbacks; i++) {
- mcb->callbacks[i].cb(mcb->callbacks[i].opaque, mcb->error);
- if (mcb->callbacks[i].free_qiov) {
- qemu_iovec_destroy(mcb->callbacks[i].free_qiov);
- }
- g_free(mcb->callbacks[i].free_qiov);
- }
-}
-
-static void multiwrite_cb(void *opaque, int ret)
-{
- MultiwriteCB *mcb = opaque;
-
- trace_multiwrite_cb(mcb, ret);
-
- if (ret < 0 && !mcb->error) {
- mcb->error = ret;
- }
-
- mcb->num_requests--;
- if (mcb->num_requests == 0) {
- multiwrite_user_cb(mcb);
- g_free(mcb);
- }
-}
-
-static int multiwrite_req_compare(const void *a, const void *b)
-{
- const BlockRequest *req1 = a, *req2 = b;
-
- /*
- * Note that we can't simply subtract req2->sector from req1->sector
- * here as that could overflow the return value.
- */
- if (req1->sector > req2->sector) {
- return 1;
- } else if (req1->sector < req2->sector) {
- return -1;
- } else {
- return 0;
- }
-}
-
-/*
- * Takes a bunch of requests and tries to merge them. Returns the number of
- * requests that remain after merging.
- */
-static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
- int num_reqs, MultiwriteCB *mcb)
-{
- int i, outidx;
-
- // Sort requests by start sector
- qsort(reqs, num_reqs, sizeof(*reqs), &multiwrite_req_compare);
-
- // Check if adjacent requests touch the same clusters. If so, combine them,
- // filling up gaps with zero sectors.
- outidx = 0;
- for (i = 1; i < num_reqs; i++) {
- int merge = 0;
- int64_t oldreq_last = reqs[outidx].sector + reqs[outidx].nb_sectors;
-
- // Handle exactly sequential writes and overlapping writes.
- if (reqs[i].sector <= oldreq_last) {
- merge = 1;
- }
-
- if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1 >
- bs->bl.max_iov) {
- merge = 0;
- }
-
- if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors +
- reqs[i].nb_sectors > bs->bl.max_transfer_length) {
- merge = 0;
- }
-
- if (merge) {
- size_t size;
- QEMUIOVector *qiov = g_malloc0(sizeof(*qiov));
- qemu_iovec_init(qiov,
- reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1);
-
- // Add the first request to the merged one. If the requests are
- // overlapping, drop the last sectors of the first request.
- size = (reqs[i].sector - reqs[outidx].sector) << 9;
- qemu_iovec_concat(qiov, reqs[outidx].qiov, 0, size);
-
- // We should need to add any zeros between the two requests
- assert (reqs[i].sector <= oldreq_last);
-
- // Add the second request
- qemu_iovec_concat(qiov, reqs[i].qiov, 0, reqs[i].qiov->size);
-
- // Add tail of first request, if necessary
- if (qiov->size < reqs[outidx].qiov->size) {
- qemu_iovec_concat(qiov, reqs[outidx].qiov, qiov->size,
- reqs[outidx].qiov->size - qiov->size);
- }
-
- reqs[outidx].nb_sectors = qiov->size >> 9;
- reqs[outidx].qiov = qiov;
-
- mcb->callbacks[i].free_qiov = reqs[outidx].qiov;
- } else {
- outidx++;
- reqs[outidx].sector = reqs[i].sector;
- reqs[outidx].nb_sectors = reqs[i].nb_sectors;
- reqs[outidx].qiov = reqs[i].qiov;
- }
- }
-
- if (bs->blk) {
- block_acct_merge_done(blk_get_stats(bs->blk), BLOCK_ACCT_WRITE,
- num_reqs - outidx - 1);
- }
-
- return outidx + 1;
-}
-
-/*
- * Submit multiple AIO write requests at once.
- *
- * On success, the function returns 0 and all requests in the reqs array have
- * been submitted. In error case this function returns -1, and any of the
- * requests may or may not be submitted yet. In particular, this means that the
- * callback will be called for some of the requests, for others it won't. The
- * caller must check the error field of the BlockRequest to wait for the right
- * callbacks (if error != 0, no callback will be called).
- *
- * The implementation may modify the contents of the reqs array, e.g. to merge
- * requests. However, the fields opaque and error are left unmodified as they
- * are used to signal failure for a single request to the caller.
- */
-int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
-{
- MultiwriteCB *mcb;
- int i;
-
- /* don't submit writes if we don't have a medium */
- if (bs->drv == NULL) {
- for (i = 0; i < num_reqs; i++) {
- reqs[i].error = -ENOMEDIUM;
- }
- return -1;
- }
-
- if (num_reqs == 0) {
- return 0;
- }
-
- // Create MultiwriteCB structure
- mcb = g_malloc0(sizeof(*mcb) + num_reqs * sizeof(*mcb->callbacks));
- mcb->num_requests = 0;
- mcb->num_callbacks = num_reqs;
-
- for (i = 0; i < num_reqs; i++) {
- mcb->callbacks[i].cb = reqs[i].cb;
- mcb->callbacks[i].opaque = reqs[i].opaque;
- }
-
- // Check for mergable requests
- num_reqs = multiwrite_merge(bs, reqs, num_reqs, mcb);
-
- trace_bdrv_aio_multiwrite(mcb, mcb->num_callbacks, num_reqs);
-
- /* Run the aio requests. */
- mcb->num_requests = num_reqs;
- for (i = 0; i < num_reqs; i++) {
- bdrv_co_aio_rw_vector(bs, reqs[i].sector, reqs[i].qiov,
- reqs[i].nb_sectors, reqs[i].flags,
- multiwrite_cb, mcb,
- true);
- }
-
- return 0;
-}
-
void bdrv_aio_cancel(BlockAIOCB *acb)
{
qemu_aio_ref(acb);
diff --git a/include/block/block.h b/include/block/block.h
index 46cee8e..c2d1456 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -327,7 +327,7 @@ void bdrv_aio_cancel(BlockAIOCB *acb);
void bdrv_aio_cancel_async(BlockAIOCB *acb);
typedef struct BlockRequest {
- /* Fields to be filled by multiwrite caller */
+ /* Fields to be filled by caller */
union {
struct {
int64_t sector;
@@ -343,13 +343,10 @@ typedef struct BlockRequest {
BlockCompletionFunc *cb;
void *opaque;
- /* Filled by multiwrite implementation */
+ /* Filled by block layer */
int error;
} BlockRequest;
-int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs,
- int num_reqs);
-
/* sg packet commands */
int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf);
BlockAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index a2f5d97d..1ae1ac9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -141,7 +141,6 @@ BlockAIOCB *blk_aio_discard(BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque);
void blk_aio_cancel(BlockAIOCB *acb);
void blk_aio_cancel_async(BlockAIOCB *acb);
-int blk_aio_multiwrite(BlockBackend *blk, BlockRequest *reqs, int num_reqs);
int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
BlockCompletionFunc *cb, void *opaque);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 35ee50b..9f86a2f 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -612,49 +612,6 @@ static int do_aio_writev(BlockBackend *blk, QEMUIOVector *qiov,
return async_ret < 0 ? async_ret : 1;
}
-struct multiwrite_async_ret {
- int num_done;
- int error;
-};
-
-static void multiwrite_cb(void *opaque, int ret)
-{
- struct multiwrite_async_ret *async_ret = opaque;
-
- async_ret->num_done++;
- if (ret < 0) {
- async_ret->error = ret;
- }
-}
-
-static int do_aio_multiwrite(BlockBackend *blk, BlockRequest* reqs,
- int num_reqs, int *total)
-{
- int i, ret;
- struct multiwrite_async_ret async_ret = {
- .num_done = 0,
- .error = 0,
- };
-
- *total = 0;
- for (i = 0; i < num_reqs; i++) {
- reqs[i].cb = multiwrite_cb;
- reqs[i].opaque = &async_ret;
- *total += reqs[i].qiov->size;
- }
-
- ret = blk_aio_multiwrite(blk, reqs, num_reqs);
- if (ret < 0) {
- return ret;
- }
-
- while (async_ret.num_done < num_reqs) {
- main_loop_wait(false);
- }
-
- return async_ret.error < 0 ? async_ret.error : 1;
-}
-
static void read_help(void)
{
printf(
@@ -1246,165 +1203,6 @@ out:
return 0;
}
-static void multiwrite_help(void)
-{
- printf(
-"\n"
-" writes a range of bytes from the given offset source from multiple buffers,\n"
-" in a batch of requests that may be merged by qemu\n"
-"\n"
-" Example:\n"
-" 'multiwrite 512 1k 1k ; 4k 1k'\n"
-" writes 2 kB at 512 bytes and 1 kB at 4 kB into the open file\n"
-"\n"
-" Writes into a segment of the currently open file, using a buffer\n"
-" filled with a set pattern (0xcdcdcdcd). The pattern byte is increased\n"
-" by one for each request contained in the multiwrite command.\n"
-" -P, -- use different pattern to fill file\n"
-" -C, -- report statistics in a machine parsable format\n"
-" -q, -- quiet mode, do not show I/O statistics\n"
-"\n");
-}
-
-static int multiwrite_f(BlockBackend *blk, int argc, char **argv);
-
-static const cmdinfo_t multiwrite_cmd = {
- .name = "multiwrite",
- .cfunc = multiwrite_f,
- .argmin = 2,
- .argmax = -1,
- .args = "[-Cq] [-P pattern ] off len [len..] [; off len [len..]..]",
- .oneline = "issues multiple write requests at once",
- .help = multiwrite_help,
-};
-
-static int multiwrite_f(BlockBackend *blk, int argc, char **argv)
-{
- struct timeval t1, t2;
- int Cflag = 0, qflag = 0;
- int c, cnt;
- char **buf;
- int64_t offset, first_offset = 0;
- /* Some compilers get confused and warn if this is not initialized. */
- int total = 0;
- int nr_iov;
- int nr_reqs;
- int pattern = 0xcd;
- QEMUIOVector *qiovs;
- int i;
- BlockRequest *reqs;
-
- while ((c = getopt(argc, argv, "CqP:")) != -1) {
- switch (c) {
- case 'C':
- Cflag = 1;
- break;
- case 'q':
- qflag = 1;
- break;
- case 'P':
- pattern = parse_pattern(optarg);
- if (pattern < 0) {
- return 0;
- }
- break;
- default:
- return qemuio_command_usage(&writev_cmd);
- }
- }
-
- if (optind > argc - 2) {
- return qemuio_command_usage(&writev_cmd);
- }
-
- nr_reqs = 1;
- for (i = optind; i < argc; i++) {
- if (!strcmp(argv[i], ";")) {
- nr_reqs++;
- }
- }
-
- reqs = g_new0(BlockRequest, nr_reqs);
- buf = g_new0(char *, nr_reqs);
- qiovs = g_new(QEMUIOVector, nr_reqs);
-
- for (i = 0; i < nr_reqs && optind < argc; i++) {
- int j;
-
- /* Read the offset of the request */
- offset = cvtnum(argv[optind]);
- if (offset < 0) {
- print_cvtnum_err(offset, argv[optind]);
- goto out;
- }
- optind++;
-
- if (offset & 0x1ff) {
- printf("offset %lld is not sector aligned\n",
- (long long)offset);
- goto out;
- }
-
- if (i == 0) {
- first_offset = offset;
- }
-
- /* Read lengths for qiov entries */
- for (j = optind; j < argc; j++) {
- if (!strcmp(argv[j], ";")) {
- break;
- }
- }
-
- nr_iov = j - optind;
-
- /* Build request */
- buf[i] = create_iovec(blk, &qiovs[i], &argv[optind], nr_iov, pattern);
- if (buf[i] == NULL) {
- goto out;
- }
-
- reqs[i].qiov = &qiovs[i];
- reqs[i].sector = offset >> 9;
- reqs[i].nb_sectors = reqs[i].qiov->size >> 9;
-
- optind = j + 1;
-
- pattern++;
- }
-
- /* If there were empty requests at the end, ignore them */
- nr_reqs = i;
-
- gettimeofday(&t1, NULL);
- cnt = do_aio_multiwrite(blk, reqs, nr_reqs, &total);
- gettimeofday(&t2, NULL);
-
- if (cnt < 0) {
- printf("aio_multiwrite failed: %s\n", strerror(-cnt));
- goto out;
- }
-
- if (qflag) {
- goto out;
- }
-
- /* Finally, report back -- -C gives a parsable format */
- t2 = tsub(t2, t1);
- print_report("wrote", &t2, first_offset, total, total, cnt, Cflag);
-out:
- for (i = 0; i < nr_reqs; i++) {
- qemu_io_free(buf[i]);
- if (reqs[i].qiov != NULL) {
- qemu_iovec_destroy(&qiovs[i]);
- }
- }
- g_free(buf);
- g_free(reqs);
- g_free(qiovs);
- return 0;
-}
-
struct aio_ctx {
BlockBackend *blk;
QEMUIOVector qiov;
@@ -2440,7 +2238,6 @@ static void __attribute((constructor)) init_qemuio_commands(void)
qemuio_add_command(&readv_cmd);
qemuio_add_command(&write_cmd);
qemuio_add_command(&writev_cmd);
- qemuio_add_command(&multiwrite_cmd);
qemuio_add_command(&aio_read_cmd);
qemuio_add_command(&aio_write_cmd);
qemuio_add_command(&aio_flush_cmd);
diff --git a/tests/qemu-iotests/100 b/tests/qemu-iotests/100
deleted file mode 100755
index 7c1b235..0000000
--- a/tests/qemu-iotests/100
+++ /dev/null
@@ -1,146 +0,0 @@
-#!/bin/bash
-#
-# Test simple read/write using plain bdrv_read/bdrv_write
-#
-# Copyright (C) 2014 Red Hat, Inc.
-#
-# 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=stefanha@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-tmp=/tmp/$$
-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 generic
-_supported_proto generic
-_supported_os Linux
-
-
-size=128M
-
-echo
-echo "== Single request =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 0 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Sequential requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 0 4k ; 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0xce 4k 4k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 8k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Superset overlapping requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 0 4k ; 1k 2k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-# Order of overlapping in-flight requests is not guaranteed so we cannot verify
-# [1k, 3k) since it could have either pattern 0xcd or 0xce.
-$QEMU_IO -c "read -P 0xcd 0 1k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0xcd 3k 1k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Subset overlapping requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 1k 2k ; 0k 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-# Order of overlapping in-flight requests is not guaranteed so we cannot verify
-# [1k, 3k) since it could have either pattern 0xcd or 0xce.
-$QEMU_IO -c "read -P 0xce 0 1k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0xce 3k 1k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Head overlapping requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 0k 2k ; 0k 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-# Order of overlapping in-flight requests is not guaranteed so we cannot verify
-# [0k, 2k) since it could have either pattern 0xcd or 0xce.
-$QEMU_IO -c "read -P 0xce 2k 2k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Tail overlapping requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 2k 2k ; 0k 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-# Order of overlapping in-flight requests is not guaranteed so we cannot verify
-# [2k, 4k) since it could have either pattern 0xcd or 0xce.
-$QEMU_IO -c "read -P 0xce 0k 2k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 4k" "$TEST_IMG" | _filter_qemu_io
-
-_cleanup_test_img
-
-echo
-echo "== Disjoint requests =="
-_make_test_img $size
-$QEMU_IO -c "multiwrite 0 4k ; 64k 4k" "$TEST_IMG" | _filter_qemu_io
-
-echo
-echo "== verify pattern =="
-$QEMU_IO -c "read -P 0xcd 0 4k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 4k 60k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0xce 64k 4k" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0 68k 4k" "$TEST_IMG" | _filter_qemu_io
-
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
diff --git a/tests/qemu-iotests/100.out b/tests/qemu-iotests/100.out
deleted file mode 100644
index 0564903..0000000
--- a/tests/qemu-iotests/100.out
+++ /dev/null
@@ -1,89 +0,0 @@
-QA output created by 100
-
-== Single request ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 4096/4096 bytes at offset 0
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 4096/4096 bytes at offset 0
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Sequential requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 8192/8192 bytes at offset 0
-8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 4096/4096 bytes at offset 0
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 8192
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Superset overlapping requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 6144/6144 bytes at offset 0
-6 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 1024/1024 bytes at offset 0
-1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 1024/1024 bytes at offset 3072
-1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Subset overlapping requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 6144/6144 bytes at offset 1024
-6 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 1024/1024 bytes at offset 0
-1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 1024/1024 bytes at offset 3072
-1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Head overlapping requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 6144/6144 bytes at offset 0
-6 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 2048/2048 bytes at offset 2048
-2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Tail overlapping requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 6144/6144 bytes at offset 2048
-6 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 2048/2048 bytes at offset 0
-2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 4096
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== Disjoint requests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-wrote 8192/8192 bytes at offset 0
-8 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-
-== verify pattern ==
-read 4096/4096 bytes at offset 0
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 61440/61440 bytes at offset 4096
-60 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 65536
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 4096/4096 bytes at offset 69632
-4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-*** done
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index e8c6937..996265e 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -248,14 +248,6 @@ sector = "%d"
if failed_wr_ops > 0:
highest_offset = max(highest_offset, bad_offset + 512)
- for i in range(wr_merged):
- first = i * wr_size * 2
- second = first + wr_size
- ops.append("multiwrite %d %d ; %d %d" %
- (first, wr_size, second, wr_size))
-
- highest_offset = max(highest_offset, wr_merged * wr_size * 2)
-
# Now perform all operations
for op in ops:
self.vm.hmp_qemu_io("drive0", op)
@@ -309,19 +301,15 @@ sector = "%d"
def test_flush(self):
self.do_test_stats(flush_ops = 8)
- def test_merged(self):
- for i in range(5):
- self.do_test_stats(wr_merged = i * 3)
-
def test_all(self):
# rd_size, rd_ops, wr_size, wr_ops, flush_ops
# invalid_rd_ops, invalid_wr_ops,
# failed_rd_ops, failed_wr_ops
# wr_merged
- test_values = [[512, 1, 512, 1, 1, 4, 7, 5, 2, 1],
- [65536, 1, 2048, 12, 7, 7, 5, 2, 5, 5],
- [32768, 9, 8192, 1, 4, 3, 2, 4, 6, 4],
- [16384, 11, 3584, 16, 9, 8, 6, 7, 3, 4]]
+ test_values = [[512, 1, 512, 1, 1, 4, 7, 5, 2, 0],
+ [65536, 1, 2048, 12, 7, 7, 5, 2, 5, 0],
+ [32768, 9, 8192, 1, 4, 3, 2, 4, 6, 0],
+ [16384, 11, 3584, 16, 9, 8, 6, 7, 3, 0]]
for i in test_values:
self.do_test_stats(*i)
diff --git a/tests/qemu-iotests/136.out b/tests/qemu-iotests/136.out
index 0a5e958..cfa5c0d 100644
--- a/tests/qemu-iotests/136.out
+++ b/tests/qemu-iotests/136.out
@@ -1,5 +1,5 @@
-........................................
+...................................
----------------------------------------------------------------------
-Ran 40 tests
+Ran 35 tests
OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index faf0f21..e3c6e9b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -106,7 +106,7 @@
097 rw auto backing
098 rw auto backing quick
099 rw auto quick
-100 rw auto quick
+# 100 was removed, do not reuse
101 rw auto quick
102 rw auto quick
103 rw auto quick
diff --git a/trace-events b/trace-events
index d494de1..d15258d 100644
--- a/trace-events
+++ b/trace-events
@@ -62,8 +62,6 @@ bdrv_open_common(void *bs, const char *filename, int flags, const char *format_n
bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
# block/io.c
-multiwrite_cb(void *mcb, int ret) "mcb %p ret %d"
-bdrv_aio_multiwrite(void *mcb, int num_callbacks, int num_reqs) "mcb %p num_callbacks %d num_reqs %d"
bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 5/9] block: Avoid BDS.blk in bdrv_next()
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (3 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 4/9] block: Remove bdrv_aio_multiwrite() Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:26 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 6/9] block: Remove bdrv_move_feature_fields() Kevin Wolf
` (4 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
We just want to know whether a BDS has at least one BB attached in order
to avoid enumerating it twice. This doesn't depend on the exact BB that
is attached and is still a valid question when more than one BB can be
attached, so just answer it by checking the parents list.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 4 ++--
block/block-backend.c | 17 +++++++++++++++++
include/sysemu/block-backend.h | 1 +
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 4cc117d..66f918e 100644
--- a/block.c
+++ b/block.c
@@ -2904,7 +2904,7 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
* the monitor or attached to a BlockBackend */
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
- if (!bs || bs->blk) {
+ if (!bs || bdrv_has_blk(bs)) {
bs = blk_next_root_bs(bs);
if (bs) {
return bs;
@@ -2915,7 +2915,7 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
* handled by the above block already */
do {
bs = bdrv_next_monitor_owned(bs);
- } while (bs && bs->blk);
+ } while (bs && bdrv_has_blk(bs));
return bs;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index dfc11b5..fdd1727 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -434,6 +434,23 @@ BlockDriverState *blk_bs(BlockBackend *blk)
}
/*
+ * Returns true if @bs has an associated BlockBackend.
+ */
+bool bdrv_has_blk(BlockDriverState *bs)
+{
+ BdrvChild *child;
+ QLIST_FOREACH(child, &bs->parents, next_parent) {
+ if (child->role == &child_root) {
+ assert(bs->blk);
+ return true;
+ }
+ }
+
+ assert(!bs->blk);
+ return false;
+}
+
+/*
* Return @blk's DriveInfo if any, else null.
*/
DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 1ae1ac9..bd68a17 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -99,6 +99,7 @@ BlockBackend *blk_by_public(BlockBackendPublic *public);
BlockDriverState *blk_bs(BlockBackend *blk);
void blk_remove_bs(BlockBackend *blk);
void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
+bool bdrv_has_blk(BlockDriverState *bs);
void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
void blk_iostatus_enable(BlockBackend *blk);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 6/9] block: Remove bdrv_move_feature_fields()
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (4 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 5/9] block: Avoid BDS.blk in bdrv_next() Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:28 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next() Kevin Wolf
` (3 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
bdrv_move_feature_fields() and swap_feature_fields() are empty now, they
can be removed.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/block.c b/block.c
index 66f918e..3770fb0 100644
--- a/block.c
+++ b/block.c
@@ -2222,13 +2222,6 @@ void bdrv_close_all(void)
}
}
-/* Fields that need to stay with the top-level BDS */
-static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
- BlockDriverState *bs_src)
-{
- /* move some fields that need to stay attached to the device */
-}
-
static void change_parent_backing_link(BlockDriverState *from,
BlockDriverState *to)
{
@@ -2252,16 +2245,6 @@ static void change_parent_backing_link(BlockDriverState *from,
}
}
-static void swap_feature_fields(BlockDriverState *bs_top,
- BlockDriverState *bs_new)
-{
- BlockDriverState tmp;
-
- bdrv_move_feature_fields(&tmp, bs_top);
- bdrv_move_feature_fields(bs_top, bs_new);
- bdrv_move_feature_fields(bs_new, &tmp);
-}
-
/*
* Add new bs contents at the top of an image chain while the chain is
* live, while keeping required fields on the top layer.
@@ -2285,9 +2268,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
bdrv_ref(bs_top);
- /* Some fields always stay on top of the backing file chain */
- swap_feature_fields(bs_top, bs_new);
-
change_parent_backing_link(bs_top, bs_new);
bdrv_set_backing_hd(bs_new, bs_top);
bdrv_unref(bs_top);
@@ -2304,16 +2284,6 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
bdrv_ref(old);
- if (old->blk) {
- /* As long as these fields aren't in BlockBackend, but in the top-level
- * BlockDriverState, it's not possible for a BDS to have two BBs.
- *
- * We really want to copy the fields from old to new, but we go for a
- * swap instead so that pointers aren't duplicated and cause trouble.
- * (Also, bdrv_swap() used to do the same.) */
- assert(!new->blk);
- swap_feature_fields(old, new);
- }
change_parent_backing_link(old, new);
/* Change backing files if a previously independent node is added to the
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next()
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (5 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 6/9] block: Remove bdrv_move_feature_fields() Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:50 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 8/9] block: Don't return throttling info in query-named-block-nodes Kevin Wolf
` (2 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
We need to introduce a separate BdrvNextIterator struct that can keep
more state than just the current BDS in order to avoid using the bs->blk
pointer.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 34 +++++++++-----------------------
block/block-backend.c | 44 +++++++++++++++++++++++++++---------------
block/io.c | 13 +++++++------
block/snapshot.c | 30 ++++++++++++++++------------
blockdev.c | 3 ++-
include/block/block.h | 3 ++-
include/sysemu/block-backend.h | 1 -
migration/block.c | 4 +++-
monitor.c | 6 ++++--
qmp.c | 5 ++++-
10 files changed, 77 insertions(+), 66 deletions(-)
diff --git a/block.c b/block.c
index 3770fb0..eefbcf3 100644
--- a/block.c
+++ b/block.c
@@ -2870,25 +2870,6 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
return QTAILQ_NEXT(bs, node_list);
}
-/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
- * the monitor or attached to a BlockBackend */
-BlockDriverState *bdrv_next(BlockDriverState *bs)
-{
- if (!bs || bdrv_has_blk(bs)) {
- bs = blk_next_root_bs(bs);
- if (bs) {
- return bs;
- }
- }
-
- /* Ignore all BDSs that are attached to a BlockBackend here; they have been
- * handled by the above block already */
- do {
- bs = bdrv_next_monitor_owned(bs);
- } while (bs && bdrv_has_blk(bs));
- return bs;
-}
-
const char *bdrv_get_node_name(const BlockDriverState *bs)
{
return bs->node_name;
@@ -3212,10 +3193,11 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
void bdrv_invalidate_cache_all(Error **errp)
{
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
Error *local_err = NULL;
+ BdrvNextIterator *it = NULL;
- while ((bs = bdrv_next(bs)) != NULL) {
+ while ((it = bdrv_next(it, &bs)) != NULL) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3245,10 +3227,11 @@ static int bdrv_inactivate(BlockDriverState *bs)
int bdrv_inactivate_all(void)
{
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
int ret;
- while ((bs = bdrv_next(bs)) != NULL) {
+ while ((it = bdrv_next(it, &bs)) != NULL) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3748,10 +3731,11 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
*/
bool bdrv_is_first_non_filter(BlockDriverState *candidate)
{
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
/* walk down the bs forest recursively */
- while ((bs = bdrv_next(bs)) != NULL) {
+ while ((it = bdrv_next(it, &bs)) != NULL) {
bool perm;
/* try to recurse in this top level bs */
diff --git a/block/block-backend.c b/block/block-backend.c
index fdd1727..b71b822 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -325,28 +325,40 @@ BlockBackend *blk_next(BlockBackend *blk)
: QTAILQ_FIRST(&monitor_block_backends);
}
-/*
- * Iterates over all BlockDriverStates which are attached to a BlockBackend.
- * This function is for use by bdrv_next().
- *
- * @bs must be NULL or a BDS that is attached to a BB.
- */
-BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
-{
+struct BdrvNextIterator {
+ int phase;
BlockBackend *blk;
+ BlockDriverState *bs;
+};
- if (bs) {
- assert(bs->blk);
- blk = bs->blk;
- } else {
- blk = NULL;
+/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
+ * the monitor or attached to a BlockBackend */
+BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
+{
+ if (!it) {
+ it = g_new0(BdrvNextIterator, 1);
+ }
+
+ if (it->phase == 0) {
+ do {
+ it->blk = blk_all_next(it->blk);
+ *bs = it->blk ? blk_bs(it->blk) : NULL;
+ } while (it->blk && *bs == NULL);
+
+ if (*bs) {
+ return it;
+ }
+ it->phase = 1;
}
+ /* Ignore all BDSs that are attached to a BlockBackend here; they have been
+ * handled by the above block already */
do {
- blk = blk_all_next(blk);
- } while (blk && !blk->root);
+ it->bs = bdrv_next_monitor_owned(it->bs);
+ *bs = it->bs;
+ } while (*bs && bdrv_has_blk(*bs));
- return blk ? blk->root->bs : NULL;
+ return *bs ? it : NULL;
}
/*
diff --git a/block/io.c b/block/io.c
index 6ec133f..ead65cd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -213,10 +213,11 @@ void bdrv_drain_all(void)
{
/* Always run first iteration so any pending completion BHs run */
bool busy = true;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
GSList *aio_ctxs = NULL, *ctx;
- while ((bs = bdrv_next(bs))) {
+ while ((it = bdrv_next(it, &bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -242,10 +243,10 @@ void bdrv_drain_all(void)
for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
AioContext *aio_context = ctx->data;
- bs = NULL;
+ it = NULL;
aio_context_acquire(aio_context);
- while ((bs = bdrv_next(bs))) {
+ while ((it = bdrv_next(it, &bs))) {
if (aio_context == bdrv_get_aio_context(bs)) {
if (bdrv_flush_io_queue(bs)) {
busy = true;
@@ -261,8 +262,8 @@ void bdrv_drain_all(void)
}
}
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
+ it = NULL;
+ while ((it = bdrv_next(it, &bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
diff --git a/block/snapshot.c b/block/snapshot.c
index 17a27b5..6eb0371 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -372,9 +372,10 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
{
bool ok = true;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- while (ok && (bs = bdrv_next(bs))) {
+ while (ok && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -392,10 +393,11 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
Error **err)
{
int ret = 0;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- while (ret == 0 && (bs = bdrv_next(bs))) {
+ while (ret == 0 && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -414,9 +416,10 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
{
int err = 0;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- while (err == 0 && (bs = bdrv_next(bs))) {
+ while (err == 0 && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -434,9 +437,10 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
{
QEMUSnapshotInfo sn;
int err = 0;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- while (err == 0 && (bs = bdrv_next(bs))) {
+ while (err == 0 && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -456,9 +460,10 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState **first_bad_bs)
{
int err = 0;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- while (err == 0 && (bs = bdrv_next(bs))) {
+ while (err == 0 && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -479,9 +484,10 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *bdrv_all_find_vmstate_bs(void)
{
bool not_found = true;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- while (not_found && (bs = bdrv_next(bs))) {
+ while (not_found && (it = bdrv_next(it, &bs))) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
diff --git a/blockdev.c b/blockdev.c
index 99657d0..c59cf3e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4073,8 +4073,9 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
{
BlockJobInfoList *head = NULL, **p_next = &head;
BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
- for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
+ while ((it = bdrv_next(it, &bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
diff --git a/include/block/block.h b/include/block/block.h
index c2d1456..2bafd32 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -17,6 +17,7 @@ typedef struct BlockJob BlockJob;
typedef struct BdrvChild BdrvChild;
typedef struct BdrvChildRole BdrvChildRole;
typedef struct BlockJobTxn BlockJobTxn;
+typedef struct BdrvNextIterator BdrvNextIterator;
typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
@@ -398,7 +399,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
Error **errp);
bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
BlockDriverState *bdrv_next_node(BlockDriverState *bs);
-BlockDriverState *bdrv_next(BlockDriverState *bs);
+BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs);
BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
int bdrv_is_encrypted(BlockDriverState *bs);
int bdrv_key_required(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index bd68a17..4e066a2 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -89,7 +89,6 @@ void blk_remove_all_bs(void);
const char *blk_name(BlockBackend *blk);
BlockBackend *blk_by_name(const char *name);
BlockBackend *blk_next(BlockBackend *blk);
-BlockDriverState *blk_next_root_bs(BlockDriverState *bs);
bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
void monitor_remove_blk(BlockBackend *blk);
diff --git a/migration/block.c b/migration/block.c
index 72883d7..973e62a 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -381,6 +381,7 @@ static void init_blk_migration(QEMUFile *f)
BlockDriverState *bs;
BlkMigDevState *bmds;
int64_t sectors;
+ BdrvNextIterator *it = NULL;
block_mig_state.submitted = 0;
block_mig_state.read_done = 0;
@@ -390,7 +391,8 @@ static void init_blk_migration(QEMUFile *f)
block_mig_state.bulk_completed = 0;
block_mig_state.zero_blocks = migrate_zero_blocks();
- for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
+
+ while ((it = bdrv_next(it, &bs))) {
if (bdrv_is_read_only(bs)) {
continue;
}
diff --git a/monitor.c b/monitor.c
index 4c02f0f..ad0fee5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3426,11 +3426,13 @@ void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
static void vm_completion(ReadLineState *rs, const char *str)
{
size_t len;
- BlockDriverState *bs = NULL;
+ BlockDriverState *bs;
+ BdrvNextIterator *it = NULL;
len = strlen(str);
readline_set_completion_index(rs, len);
- while ((bs = bdrv_next(bs))) {
+
+ while ((it = bdrv_next(it, &bs))) {
SnapshotInfoList *snapshots, *snapshot;
AioContext *ctx = bdrv_get_aio_context(bs);
bool ok = false;
diff --git a/qmp.c b/qmp.c
index 3f16a77..0636156 100644
--- a/qmp.c
+++ b/qmp.c
@@ -181,6 +181,7 @@ void qmp_cont(Error **errp)
Error *local_err = NULL;
BlockBackend *blk;
BlockDriverState *bs;
+ BdrvNextIterator *it;
/* if there is a dump in background, we should wait until the dump
* finished */
@@ -199,7 +200,9 @@ void qmp_cont(Error **errp)
for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
blk_iostatus_reset(blk);
}
- for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
+
+ it = NULL;
+ while ((it = bdrv_next(it, &bs))) {
bdrv_add_key(bs, NULL, &local_err);
if (local_err) {
error_propagate(errp, local_err);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next()
2016-03-22 19:36 ` [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next() Kevin Wolf
@ 2016-03-29 18:50 ` Max Reitz
0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-29 18:50 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3239 bytes --]
On 22.03.2016 20:36, Kevin Wolf wrote:
> We need to introduce a separate BdrvNextIterator struct that can keep
> more state than just the current BDS in order to avoid using the bs->blk
> pointer.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 34 +++++++++-----------------------
> block/block-backend.c | 44 +++++++++++++++++++++++++++---------------
> block/io.c | 13 +++++++------
> block/snapshot.c | 30 ++++++++++++++++------------
> blockdev.c | 3 ++-
> include/block/block.h | 3 ++-
> include/sysemu/block-backend.h | 1 -
> migration/block.c | 4 +++-
> monitor.c | 6 ++++--
> qmp.c | 5 ++++-
> 10 files changed, 77 insertions(+), 66 deletions(-)
The connection with patch 5 is a bit funny. I see why you have two
patches for this issue (because the first one introduces
bdrv_has_blk()), but I think it would be better to clear up their
relationship in the first patch's commit message.
[...]
> diff --git a/block/block-backend.c b/block/block-backend.c
> index fdd1727..b71b822 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -325,28 +325,40 @@ BlockBackend *blk_next(BlockBackend *blk)
> : QTAILQ_FIRST(&monitor_block_backends);
> }
>
> -/*
> - * Iterates over all BlockDriverStates which are attached to a BlockBackend.
> - * This function is for use by bdrv_next().
> - *
> - * @bs must be NULL or a BDS that is attached to a BB.
> - */
> -BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
> -{
> +struct BdrvNextIterator {
> + int phase;
A verbose enum would have been nicer.
> BlockBackend *blk;
> + BlockDriverState *bs;
> +};
>
> - if (bs) {
> - assert(bs->blk);
> - blk = bs->blk;
> - } else {
> - blk = NULL;
> +/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
> + * the monitor or attached to a BlockBackend */
> +BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
> +{
> + if (!it) {
> + it = g_new0(BdrvNextIterator, 1);
> + }
> +
> + if (it->phase == 0) {
> + do {
> + it->blk = blk_all_next(it->blk);
> + *bs = it->blk ? blk_bs(it->blk) : NULL;
> + } while (it->blk && *bs == NULL);
This will be interesting with multiple BBs per BDS. I guess we can do
something like only take the BDS if the BB is the first one in its
parent list.
> +
> + if (*bs) {
> + return it;
:-)
> + }
> + it->phase = 1;
> }
>
> + /* Ignore all BDSs that are attached to a BlockBackend here; they have been
> + * handled by the above block already */
> do {
> - blk = blk_all_next(blk);
> - } while (blk && !blk->root);
> + it->bs = bdrv_next_monitor_owned(it->bs);
> + *bs = it->bs;
> + } while (*bs && bdrv_has_blk(*bs));
>
> - return blk ? blk->root->bs : NULL;
> + return *bs ? it : NULL;
> }
>
> /*
Reviewed-by: Max Reitz <mreitz@redhat.com>
[...]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 8/9] block: Don't return throttling info in query-named-block-nodes
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (6 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next() Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:53 ` Max Reitz
2016-03-22 19:36 ` [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk Kevin Wolf
2016-03-22 21:34 ` [Qemu-devel] [PATCH 0/9] " Paolo Bonzini
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
query-named-block-nodes should not return information that is related
to the attached BlockBackend rather than the node itself, so throttling
information needs to be removed from it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qapi.c | 6 +++---
tests/qemu-iotests/096 | 3 ++-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 0a222f6..d167e67 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -66,10 +66,10 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->backing_file_depth = bdrv_get_backing_file_depth(bs);
info->detect_zeroes = bs->detect_zeroes;
- if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
+ if (blk && blk_get_public(blk)->throttle_state) {
ThrottleConfig cfg;
- throttle_group_get_config(bs->blk, &cfg);
+ throttle_group_get_config(blk, &cfg);
info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
info->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
@@ -117,7 +117,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->iops_size = cfg.op_size;
info->has_group = true;
- info->group = g_strdup(throttle_group_get_name(bs->blk));
+ info->group = g_strdup(throttle_group_get_name(blk));
}
info->write_threshold = bdrv_write_threshold_get(bs);
diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096
index e34204b..aeeb375 100644
--- a/tests/qemu-iotests/096
+++ b/tests/qemu-iotests/096
@@ -45,8 +45,9 @@ class TestLiveSnapshot(iotests.QMPTestCase):
os.remove(self.target_img)
def checkConfig(self, active_layer):
- result = self.vm.qmp('query-named-block-nodes')
+ result = self.vm.qmp('query-block')
for r in result['return']:
+ r = r['inserted']
if r['node-name'] == active_layer:
self.assertEqual(r['group'], self.group)
self.assertEqual(r['iops'], self.iops)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (7 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 8/9] block: Don't return throttling info in query-named-block-nodes Kevin Wolf
@ 2016-03-22 19:36 ` Kevin Wolf
2016-03-29 18:59 ` Max Reitz
2016-03-22 21:34 ` [Qemu-devel] [PATCH 0/9] " Paolo Bonzini
9 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-22 19:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
This patch removes the remaining users of bs->blk, which will allow us
to have multiple BBs on top of a single BDS. All checks that are
currently in place to prevent the user from creating such setups.
Future patches can allow them and e.g. enable users to mirror to a block
device that already has a BlockBackend on it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 8 --------
block/block-backend.c | 8 --------
block/mirror.c | 4 ++--
blockdev.c | 19 ++++++++-----------
include/block/block_int.h | 2 --
tests/qemu-iotests/085.out | 6 +++---
6 files changed, 13 insertions(+), 34 deletions(-)
diff --git a/block.c b/block.c
index eefbcf3..05f9ad4 100644
--- a/block.c
+++ b/block.c
@@ -2227,14 +2227,6 @@ static void change_parent_backing_link(BlockDriverState *from,
{
BdrvChild *c, *next;
- if (from->blk) {
- /* FIXME We bypass blk_set_bs(), so we need to make these updates
- * manually. The root problem is not in this change function, but the
- * existence of BlockDriverState.blk. */
- to->blk = from->blk;
- from->blk = NULL;
- }
-
QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
assert(c->role != &child_backing);
c->bs = to;
diff --git a/block/block-backend.c b/block/block-backend.c
index b71b822..42f95a6 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -190,7 +190,6 @@ BlockBackend *blk_new_with_bs(Error **errp)
bs = bdrv_new_root();
blk->root = bdrv_root_attach_child(bs, "root", &child_root);
blk->root->opaque = blk;
- bs->blk = blk;
return blk;
}
@@ -453,12 +452,10 @@ bool bdrv_has_blk(BlockDriverState *bs)
BdrvChild *child;
QLIST_FOREACH(child, &bs->parents, next_parent) {
if (child->role == &child_root) {
- assert(bs->blk);
return true;
}
}
- assert(!bs->blk);
return false;
}
@@ -518,8 +515,6 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
*/
void blk_remove_bs(BlockBackend *blk)
{
- assert(blk->root->bs->blk == blk);
-
notifier_list_notify(&blk->remove_bs_notifiers, blk);
if (blk->public.throttle_state) {
throttle_timers_detach_aio_context(&blk->public.throttle_timers);
@@ -527,7 +522,6 @@ void blk_remove_bs(BlockBackend *blk)
blk_update_root_state(blk);
- blk->root->bs->blk = NULL;
bdrv_root_unref_child(blk->root);
blk->root = NULL;
}
@@ -537,11 +531,9 @@ void blk_remove_bs(BlockBackend *blk)
*/
void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
{
- assert(!blk->root && !bs->blk);
bdrv_ref(bs);
blk->root = bdrv_root_attach_child(bs, "root", &child_root);
blk->root->opaque = blk;
- bs->blk = blk;
notifier_list_notify(&blk->insert_bs_notifiers, blk);
if (blk->public.throttle_state) {
diff --git a/block/mirror.c b/block/mirror.c
index b9a93fd..8b32271 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -450,7 +450,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
/* This was checked in mirror_start_job(), but meanwhile one of the
* nodes could have been newly attached to a BlockBackend. */
- if (to_replace->blk && s->target->blk) {
+ if (bdrv_has_blk(to_replace) && bdrv_has_blk(s->target)) {
error_report("block job: Can't create node with two BlockBackends");
data->ret = -EINVAL;
goto out;
@@ -825,7 +825,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
} else {
replaced_bs = bs;
}
- if (replaced_bs->blk && target->blk) {
+ if (bdrv_has_blk(replaced_bs) && bdrv_has_blk(target)) {
error_setg(errp, "Can't create node with two BlockBackends");
return;
}
diff --git a/blockdev.c b/blockdev.c
index c59cf3e..a658869 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1774,9 +1774,8 @@ static void external_snapshot_prepare(BlkActionState *common,
return;
}
- if (state->new_bs->blk != NULL) {
- error_setg(errp, "The snapshot is already in use by %s",
- blk_name(state->new_bs->blk));
+ if (bdrv_has_blk(state->new_bs)) {
+ error_setg(errp, "The snapshot is already in use");
return;
}
@@ -2492,9 +2491,8 @@ void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
return;
}
- if (bs->blk) {
- error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
- blk_name(bs->blk));
+ if (bdrv_has_blk(bs)) {
+ error_setg(errp, "Node '%s' is already in use", node_name);
return;
}
@@ -3439,7 +3437,7 @@ static void blockdev_mirror_common(BlockDriverState *bs,
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
return;
}
- if (target->blk) {
+ if (bdrv_has_blk(target)) {
error_setg(errp, "Cannot mirror to an attached block device");
return;
}
@@ -4023,15 +4021,14 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
bs = blk_bs(blk);
aio_context = blk_get_aio_context(blk);
} else {
+ blk = NULL;
bs = bdrv_find_node(node_name);
if (!bs) {
error_setg(errp, "Cannot find node %s", node_name);
return;
}
- blk = bs->blk;
- if (blk) {
- error_setg(errp, "Node %s is in use by %s",
- node_name, blk_name(blk));
+ if (bdrv_has_blk(bs)) {
+ error_setg(errp, "Node %s is in use", node_name);
return;
}
aio_context = bdrv_get_aio_context(bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9358949..2bbca78 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -406,8 +406,6 @@ struct BlockDriverState {
BlockDriver *drv; /* NULL means no media */
void *opaque;
- BlockBackend *blk; /* owning backend, if any */
-
AioContext *aio_context; /* event loop used for fd handlers, timers, etc */
/* long-running tasks intended to always use the same AioContext as this
* BDS may register themselves in this list to be notified of changes
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 01c78d6..08e4bb7 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -68,9 +68,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
=== Invalid command - snapshot node used as active layer ===
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
-{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio1"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use"}}
=== Invalid command - snapshot node used as backing hd ===
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk
2016-03-22 19:36 ` [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk Kevin Wolf
@ 2016-03-29 18:59 ` Max Reitz
0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-29 18:59 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3135 bytes --]
On 22.03.2016 20:36, Kevin Wolf wrote:
> This patch removes the remaining users of bs->blk, which will allow us
> to have multiple BBs on top of a single BDS. All checks that are
> currently in place to prevent the user from creating such setups.
I think this sentence is missing a word or two.
> Future patches can allow them and e.g. enable users to mirror to a block
> device that already has a BlockBackend on it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 8 --------
> block/block-backend.c | 8 --------
> block/mirror.c | 4 ++--
> blockdev.c | 19 ++++++++-----------
> include/block/block_int.h | 2 --
> tests/qemu-iotests/085.out | 6 +++---
> 6 files changed, 13 insertions(+), 34 deletions(-)
[...]
> diff --git a/blockdev.c b/blockdev.c
> index c59cf3e..a658869 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1774,9 +1774,8 @@ static void external_snapshot_prepare(BlkActionState *common,
> return;
> }
>
> - if (state->new_bs->blk != NULL) {
> - error_setg(errp, "The snapshot is already in use by %s",
> - blk_name(state->new_bs->blk));
> + if (bdrv_has_blk(state->new_bs)) {
> + error_setg(errp, "The snapshot is already in use");
You did add bdrv_get_parent_name(), so you could use it here...
> return;
> }
>
> @@ -2492,9 +2491,8 @@ void qmp_x_blockdev_insert_medium(const char *device, const char *node_name,
> return;
> }
>
> - if (bs->blk) {
> - error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
> - blk_name(bs->blk));
> + if (bdrv_has_blk(bs)) {
> + error_setg(errp, "Node '%s' is already in use", node_name);
...and here...
> return;
> }
>
> @@ -3439,7 +3437,7 @@ static void blockdev_mirror_common(BlockDriverState *bs,
> if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
> return;
> }
> - if (target->blk) {
> + if (bdrv_has_blk(target)) {
> error_setg(errp, "Cannot mirror to an attached block device");
> return;
> }
> @@ -4023,15 +4021,14 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
> bs = blk_bs(blk);
> aio_context = blk_get_aio_context(blk);
> } else {
> + blk = NULL;
> bs = bdrv_find_node(node_name);
> if (!bs) {
> error_setg(errp, "Cannot find node %s", node_name);
> return;
> }
> - blk = bs->blk;
> - if (blk) {
> - error_setg(errp, "Node %s is in use by %s",
> - node_name, blk_name(blk));
> + if (bdrv_has_blk(bs)) {
> + error_setg(errp, "Node %s is in use", node_name);
...and here.
If you consciously decide not to use it there, and after you have fixed
the commit message:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> return;
> }
> aio_context = bdrv_get_aio_context(bs);
[...]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk
2016-03-22 19:36 [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk Kevin Wolf
` (8 preceding siblings ...)
2016-03-22 19:36 ` [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk Kevin Wolf
@ 2016-03-22 21:34 ` Paolo Bonzini
9 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2016-03-22 21:34 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel, mreitz
On 22/03/2016 20:36, Kevin Wolf wrote:
> This is the final patch series that is required before we can start allowing
> setups with more than one BlockBackend per BlockDriverState.
>
> My current plan is to put the patches up to (and including) this series into
> 2.6 so that we have a relatively consistent block layer state in the release
> that isn't in the middle of an overhaul, but not to make use of the new setups
> that we could allow now before 2.7.
>
> Depends on 'block: Move I/O throttling to BlockBackend'.
I'm not sure how "posted one month after soft freeze", "depends on a
series also posted one month after soft freeze" and "put the patches up
to (and including) this series into 2.6" could go in the same message.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread