* [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-06-06 8:41 ` Paolo Bonzini
2016-05-25 17:39 ` [Qemu-devel] [PULL 02/31] block: Drop useless bdrv_new() call Kevin Wolf
` (30 subsequent siblings)
31 siblings, 1 reply; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
The bdrv_next() users all leaked the BdrvNextIterator after completing
the iteration. Simply changing bdrv_next() to free the iterator before
returning NULL at the end of list doesn't work because some callers exit
the loop before looking at all BDSes.
This patch moves the BdrvNextIterator from the heap to the stack of
the caller and switches to a bdrv_first()/bdrv_next() interface for
initialising the iterator.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block.c | 18 ++++++++---------
block/block-backend.c | 41 +++++++++++++++++---------------------
block/io.c | 10 ++++------
block/snapshot.c | 55 ++++++++++++++++++++++++++++++++++++++-------------
blockdev.c | 4 ++--
include/block/block.h | 15 ++++++++++++--
migration/block.c | 4 ++--
monitor.c | 4 ++--
qmp.c | 5 ++---
9 files changed, 92 insertions(+), 64 deletions(-)
diff --git a/block.c b/block.c
index 1205ef8..d287771 100644
--- a/block.c
+++ b/block.c
@@ -3195,9 +3195,9 @@ void bdrv_invalidate_cache_all(Error **errp)
{
BlockDriverState *bs;
Error *local_err = NULL;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while ((it = bdrv_next(it, &bs)) != NULL) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3239,11 +3239,11 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
int bdrv_inactivate_all(void)
{
BlockDriverState *bs = NULL;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
int ret = 0;
int pass;
- while ((it = bdrv_next(it, &bs)) != NULL) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
aio_context_acquire(bdrv_get_aio_context(bs));
}
@@ -3252,8 +3252,7 @@ int bdrv_inactivate_all(void)
* the second pass sets the BDRV_O_INACTIVE flag so that no further write
* is allowed. */
for (pass = 0; pass < 2; pass++) {
- it = NULL;
- while ((it = bdrv_next(it, &bs)) != NULL) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
ret = bdrv_inactivate_recurse(bs, pass);
if (ret < 0) {
goto out;
@@ -3262,8 +3261,7 @@ int bdrv_inactivate_all(void)
}
out:
- it = NULL;
- while ((it = bdrv_next(it, &bs)) != NULL) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
aio_context_release(bdrv_get_aio_context(bs));
}
@@ -3753,10 +3751,10 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
bool bdrv_is_first_non_filter(BlockDriverState *candidate)
{
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
/* walk down the bs forest recursively */
- while ((it = bdrv_next(it, &bs)) != NULL) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
bool perm;
/* try to recurse in this top level bs */
diff --git a/block/block-backend.c b/block/block-backend.c
index 6928d61..9f306f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -286,25 +286,11 @@ BlockBackend *blk_next(BlockBackend *blk)
: QTAILQ_FIRST(&monitor_block_backends);
}
-struct BdrvNextIterator {
- enum {
- BDRV_NEXT_BACKEND_ROOTS,
- BDRV_NEXT_MONITOR_OWNED,
- } phase;
- BlockBackend *blk;
- BlockDriverState *bs;
-};
-
/* 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)
+BlockDriverState *bdrv_next(BdrvNextIterator *it)
{
- if (!it) {
- it = g_new(BdrvNextIterator, 1);
- *it = (BdrvNextIterator) {
- .phase = BDRV_NEXT_BACKEND_ROOTS,
- };
- }
+ BlockDriverState *bs;
/* First, return all root nodes of BlockBackends. In order to avoid
* returning a BDS twice when multiple BBs refer to it, we only return it
@@ -312,11 +298,11 @@ BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
if (it->phase == BDRV_NEXT_BACKEND_ROOTS) {
do {
it->blk = blk_all_next(it->blk);
- *bs = it->blk ? blk_bs(it->blk) : NULL;
- } while (it->blk && (*bs == NULL || bdrv_first_blk(*bs) != it->blk));
+ bs = it->blk ? blk_bs(it->blk) : NULL;
+ } while (it->blk && (bs == NULL || bdrv_first_blk(bs) != it->blk));
- if (*bs) {
- return it;
+ if (bs) {
+ return bs;
}
it->phase = BDRV_NEXT_MONITOR_OWNED;
}
@@ -326,10 +312,19 @@ BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
* by the above block already */
do {
it->bs = bdrv_next_monitor_owned(it->bs);
- *bs = it->bs;
- } while (*bs && bdrv_has_blk(*bs));
+ bs = it->bs;
+ } while (bs && bdrv_has_blk(bs));
+
+ return bs;
+}
+
+BlockDriverState *bdrv_first(BdrvNextIterator *it)
+{
+ *it = (BdrvNextIterator) {
+ .phase = BDRV_NEXT_BACKEND_ROOTS,
+ };
- return *bs ? it : NULL;
+ return bdrv_next(it);
}
/*
diff --git a/block/io.c b/block/io.c
index 60a6bd8..2a28d63 100644
--- a/block/io.c
+++ b/block/io.c
@@ -271,10 +271,10 @@ void bdrv_drain_all(void)
/* Always run first iteration so any pending completion BHs run */
bool busy = true;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
GSList *aio_ctxs = NULL, *ctx;
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -302,10 +302,9 @@ void bdrv_drain_all(void)
for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
AioContext *aio_context = ctx->data;
- it = NULL;
aio_context_acquire(aio_context);
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
if (aio_context == bdrv_get_aio_context(bs)) {
if (bdrv_requests_pending(bs)) {
busy = true;
@@ -318,8 +317,7 @@ void bdrv_drain_all(void)
}
}
- it = NULL;
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
diff --git a/block/snapshot.c b/block/snapshot.c
index 3917ec5..6e6e34f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -374,9 +374,9 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
{
bool ok = true;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while (ok && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -384,8 +384,12 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
ok = bdrv_can_snapshot(bs);
}
aio_context_release(ctx);
+ if (!ok) {
+ goto fail;
+ }
}
+fail:
*first_bad_bs = bs;
return ok;
}
@@ -395,20 +399,27 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
{
int ret = 0;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- while (ret == 0 && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0) {
ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
+ if (ret < 0) {
+ goto fail;
+ }
}
aio_context_release(ctx);
+ if (ret < 0) {
+ goto fail;
+ }
}
+fail:
*first_bad_bs = bs;
return ret;
}
@@ -418,9 +429,9 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
{
int err = 0;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while (err == 0 && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -428,8 +439,12 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
err = bdrv_snapshot_goto(bs, name);
}
aio_context_release(ctx);
+ if (err < 0) {
+ goto fail;
+ }
}
+fail:
*first_bad_bs = bs;
return err;
}
@@ -439,9 +454,9 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
QEMUSnapshotInfo sn;
int err = 0;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while (err == 0 && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -449,8 +464,12 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
err = bdrv_snapshot_find(bs, &sn, name);
}
aio_context_release(ctx);
+ if (err < 0) {
+ goto fail;
+ }
}
+fail:
*first_bad_bs = bs;
return err;
}
@@ -462,9 +481,9 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
{
int err = 0;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while (err == 0 && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -476,24 +495,32 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
err = bdrv_snapshot_create(bs, sn);
}
aio_context_release(ctx);
+ if (err < 0) {
+ goto fail;
+ }
}
+fail:
*first_bad_bs = bs;
return err;
}
BlockDriverState *bdrv_all_find_vmstate_bs(void)
{
- bool not_found = true;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while (not_found && (it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ bool found;
aio_context_acquire(ctx);
- not_found = !bdrv_can_snapshot(bs);
+ found = bdrv_can_snapshot(bs);
aio_context_release(ctx);
+
+ if (found) {
+ break;
+ }
}
return bs;
}
diff --git a/blockdev.c b/blockdev.c
index 40e4e6f..0e37e09 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4164,9 +4164,9 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
{
BlockJobInfoList *head = NULL, **p_next = &head;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
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 a8c15e3..770ca26 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -17,7 +17,6 @@ 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 */
@@ -402,7 +401,19 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
Error **errp);
bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
BlockDriverState *bdrv_next_node(BlockDriverState *bs);
-BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs);
+
+typedef struct BdrvNextIterator {
+ enum {
+ BDRV_NEXT_BACKEND_ROOTS,
+ BDRV_NEXT_MONITOR_OWNED,
+ } phase;
+ BlockBackend *blk;
+ BlockDriverState *bs;
+} BdrvNextIterator;
+
+BlockDriverState *bdrv_first(BdrvNextIterator *it);
+BlockDriverState *bdrv_next(BdrvNextIterator *it);
+
BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
int bdrv_is_encrypted(BlockDriverState *bs);
int bdrv_key_required(BlockDriverState *bs);
diff --git a/migration/block.c b/migration/block.c
index a7a76a0..e0628d1 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -383,7 +383,7 @@ static void init_blk_migration(QEMUFile *f)
BlockDriverState *bs;
BlkMigDevState *bmds;
int64_t sectors;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
block_mig_state.submitted = 0;
block_mig_state.read_done = 0;
@@ -394,7 +394,7 @@ static void init_blk_migration(QEMUFile *f)
block_mig_state.zero_blocks = migrate_zero_blocks();
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
if (bdrv_is_read_only(bs)) {
continue;
}
diff --git a/monitor.c b/monitor.c
index 6a32b9b..404d594 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3432,12 +3432,12 @@ static void vm_completion(ReadLineState *rs, const char *str)
{
size_t len;
BlockDriverState *bs;
- BdrvNextIterator *it = NULL;
+ BdrvNextIterator it;
len = strlen(str);
readline_set_completion_index(rs, len);
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
SnapshotInfoList *snapshots, *snapshot;
AioContext *ctx = bdrv_get_aio_context(bs);
bool ok = false;
diff --git a/qmp.c b/qmp.c
index 8f8ae3a..3165f87 100644
--- a/qmp.c
+++ b/qmp.c
@@ -181,7 +181,7 @@ void qmp_cont(Error **errp)
Error *local_err = NULL;
BlockBackend *blk;
BlockDriverState *bs;
- BdrvNextIterator *it;
+ BdrvNextIterator it;
/* if there is a dump in background, we should wait until the dump
* finished */
@@ -201,8 +201,7 @@ void qmp_cont(Error **errp)
blk_iostatus_reset(blk);
}
- it = NULL;
- while ((it = bdrv_next(it, &bs))) {
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
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] 34+ messages in thread* Re: [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak
2016-05-25 17:39 ` [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak Kevin Wolf
@ 2016-06-06 8:41 ` Paolo Bonzini
0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2016-06-06 8:41 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
On 25/05/2016 19:39, Kevin Wolf wrote:
> @@ -395,20 +399,27 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
> {
> int ret = 0;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
> QEMUSnapshotInfo sn1, *snapshot = &sn1;
>
> - while (ret == 0 && (it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> AioContext *ctx = bdrv_get_aio_context(bs);
>
> aio_context_acquire(ctx);
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0) {
> ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
> + if (ret < 0) {
> + goto fail;
> + }
This is redundant with the check below (and the check below is right;
this one is wrong).
Thanks,
Paolo
> }
> aio_context_release(ctx);
> + if (ret < 0) {
> + goto fail;
> + }
> }
>
> +fail:
> *first_bad_bs = bs;
> return ret;
> }
> @@ -418,9 +429,9 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> {
> int err = 0;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> - while (err == 0 && (it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> AioContext *ctx = bdrv_get_aio_context(bs);
>
> aio_context_acquire(ctx);
> @@ -428,8 +439,12 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> err = bdrv_snapshot_goto(bs, name);
> }
> aio_context_release(ctx);
> + if (err < 0) {
> + goto fail;
> + }
> }
>
> +fail:
> *first_bad_bs = bs;
> return err;
> }
> @@ -439,9 +454,9 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> QEMUSnapshotInfo sn;
> int err = 0;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> - while (err == 0 && (it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> AioContext *ctx = bdrv_get_aio_context(bs);
>
> aio_context_acquire(ctx);
> @@ -449,8 +464,12 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> err = bdrv_snapshot_find(bs, &sn, name);
> }
> aio_context_release(ctx);
> + if (err < 0) {
> + goto fail;
> + }
> }
>
> +fail:
> *first_bad_bs = bs;
> return err;
> }
> @@ -462,9 +481,9 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
> {
> int err = 0;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> - while (err == 0 && (it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> AioContext *ctx = bdrv_get_aio_context(bs);
>
> aio_context_acquire(ctx);
> @@ -476,24 +495,32 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
> err = bdrv_snapshot_create(bs, sn);
> }
> aio_context_release(ctx);
> + if (err < 0) {
> + goto fail;
> + }
> }
>
> +fail:
> *first_bad_bs = bs;
> return err;
> }
>
> BlockDriverState *bdrv_all_find_vmstate_bs(void)
> {
> - bool not_found = true;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> - while (not_found && (it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> AioContext *ctx = bdrv_get_aio_context(bs);
> + bool found;
>
> aio_context_acquire(ctx);
> - not_found = !bdrv_can_snapshot(bs);
> + found = bdrv_can_snapshot(bs);
> aio_context_release(ctx);
> +
> + if (found) {
> + break;
> + }
> }
> return bs;
> }
> diff --git a/blockdev.c b/blockdev.c
> index 40e4e6f..0e37e09 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4164,9 +4164,9 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> {
> BlockJobInfoList *head = NULL, **p_next = &head;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> - while ((it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> 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 a8c15e3..770ca26 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -17,7 +17,6 @@ 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 */
> @@ -402,7 +401,19 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
> Error **errp);
> bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
> BlockDriverState *bdrv_next_node(BlockDriverState *bs);
> -BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs);
> +
> +typedef struct BdrvNextIterator {
> + enum {
> + BDRV_NEXT_BACKEND_ROOTS,
> + BDRV_NEXT_MONITOR_OWNED,
> + } phase;
> + BlockBackend *blk;
> + BlockDriverState *bs;
> +} BdrvNextIterator;
> +
> +BlockDriverState *bdrv_first(BdrvNextIterator *it);
> +BlockDriverState *bdrv_next(BdrvNextIterator *it);
> +
> BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
> int bdrv_is_encrypted(BlockDriverState *bs);
> int bdrv_key_required(BlockDriverState *bs);
> diff --git a/migration/block.c b/migration/block.c
> index a7a76a0..e0628d1 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -383,7 +383,7 @@ static void init_blk_migration(QEMUFile *f)
> BlockDriverState *bs;
> BlkMigDevState *bmds;
> int64_t sectors;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> block_mig_state.submitted = 0;
> block_mig_state.read_done = 0;
> @@ -394,7 +394,7 @@ static void init_blk_migration(QEMUFile *f)
> block_mig_state.zero_blocks = migrate_zero_blocks();
>
>
> - while ((it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> if (bdrv_is_read_only(bs)) {
> continue;
> }
> diff --git a/monitor.c b/monitor.c
> index 6a32b9b..404d594 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3432,12 +3432,12 @@ static void vm_completion(ReadLineState *rs, const char *str)
> {
> size_t len;
> BlockDriverState *bs;
> - BdrvNextIterator *it = NULL;
> + BdrvNextIterator it;
>
> len = strlen(str);
> readline_set_completion_index(rs, len);
>
> - while ((it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> SnapshotInfoList *snapshots, *snapshot;
> AioContext *ctx = bdrv_get_aio_context(bs);
> bool ok = false;
> diff --git a/qmp.c b/qmp.c
> index 8f8ae3a..3165f87 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -181,7 +181,7 @@ void qmp_cont(Error **errp)
> Error *local_err = NULL;
> BlockBackend *blk;
> BlockDriverState *bs;
> - BdrvNextIterator *it;
> + BdrvNextIterator it;
>
> /* if there is a dump in background, we should wait until the dump
> * finished */
> @@ -201,8 +201,7 @@ void qmp_cont(Error **errp)
> blk_iostatus_reset(blk);
> }
>
> - it = NULL;
> - while ((it = bdrv_next(it, &bs))) {
> + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> bdrv_add_key(bs, NULL, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PULL 02/31] block: Drop useless bdrv_new() call
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 03/31] block: Let bdrv_open_inherit() return the snapshot Kevin Wolf
` (29 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
bdrv_append_temp_snapshot() uses bdrv_new() to create an empty BDS
before invoking bdrv_open() on that BDS. This is probably a relict from
when it used to do some modifications on that empty BDS, but now that is
unnecessary, so we can just set bs_snapshot to NULL and let bdrv_open()
do the rest.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block.c b/block.c
index d287771..f85c5a2 100644
--- a/block.c
+++ b/block.c
@@ -1470,8 +1470,7 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
qdict_put(snapshot_options, "driver",
qstring_from_str("qcow2"));
- bs_snapshot = bdrv_new();
-
+ bs_snapshot = NULL;
ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
flags, &local_err);
snapshot_options = NULL;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 03/31] block: Let bdrv_open_inherit() return the snapshot
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 02/31] block: Drop useless bdrv_new() call Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 04/31] tests: Drop BDS from test-throttle.c Kevin Wolf
` (28 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
If bdrv_open_inherit() creates a snapshot BDS and *pbs is NULL, that
snapshot BDS should be returned instead of the BDS under it.
This has worked so far because (nearly) all users of BDRV_O_SNAPSHOT use
blk_new_open() to create the BDS tree. bdrv_append() (which is called by
bdrv_append_temp_snapshot()) redirects pointers from parents (i.e. the
BB in this case) to the newly appended child (i.e. the overlay),
therefore, while bdrv_open_inherit() did not return the root BDS, the BB
still pointed to it.
The only instance where BDRV_O_SNAPSHOT is used but blk_new_open() is
not is in blockdev_init() if no BDS tree is created, and instead
blk_new() is used and the flags are stored in the BB root state.
However, qmp_blockdev_change_medium() filters the BDRV_O_SNAPSHOT flag
before invoking bdrv_open(), so it will not have any effect.
In any case, it would be nicer if bdrv_open_inherit() could just always
return the root of the BDS tree that has been created.
To this end, bdrv_append_temp_snapshot() now returns the snapshot BDS
instead of just appending it on top of the snapshotted BDS. Also, it
calls bdrv_ref() before bdrv_append() (which bdrv_open_inherit() has to
undo if not returning the overlay).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index f85c5a2..17ee088 100644
--- a/block.c
+++ b/block.c
@@ -1422,8 +1422,10 @@ done:
return c;
}
-static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
- QDict *snapshot_options, Error **errp)
+static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
+ int flags,
+ QDict *snapshot_options,
+ Error **errp)
{
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
@@ -1439,7 +1441,6 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
/* Get the required size from the image */
total_size = bdrv_getlength(bs);
if (total_size < 0) {
- ret = total_size;
error_setg_errno(errp, -total_size, "Could not get image size");
goto out;
}
@@ -1479,12 +1480,19 @@ static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
goto out;
}
+ /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will
+ * call bdrv_unref() on it), so in order to be able to return one, we have
+ * to increase bs_snapshot's refcount here */
+ bdrv_ref(bs_snapshot);
bdrv_append(bs_snapshot, bs);
+ g_free(tmp_filename);
+ return bs_snapshot;
+
out:
QDECREF(snapshot_options);
g_free(tmp_filename);
- return ret;
+ return NULL;
}
/*
@@ -1704,17 +1712,42 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
}
QDECREF(options);
- *pbs = bs;
/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
* temporary snapshot afterwards. */
if (snapshot_flags) {
- ret = bdrv_append_temp_snapshot(bs, snapshot_flags, snapshot_options,
- &local_err);
+ BlockDriverState *snapshot_bs;
+ snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
+ snapshot_options, &local_err);
snapshot_options = NULL;
if (local_err) {
+ ret = -EINVAL;
goto close_and_fail;
}
+ if (!*pbs) {
+ /* We are not going to return bs but the overlay on top of it
+ * (snapshot_bs); thus, we have to drop the strong reference to bs
+ * (which we obtained by calling bdrv_new()). bs will not be
+ * deleted, though, because the overlay still has a reference to it.
+ */
+ bdrv_unref(bs);
+ bs = snapshot_bs;
+ } else {
+ /* We are not going to return snapshot_bs, so we have to drop the
+ * strong reference to it (which was returned by
+ * bdrv_append_temp_snapshot()). snapshot_bs will not be deleted,
+ * though, because bdrv_append_temp_snapshot() made all parental
+ * references to bs (*pbs) point to snapshot_bs.
+ * In fact, if *pbs was not NULL, we are not going to return any new
+ * BDS. But we do not need to decrement bs's refcount here as is
+ * done above, because with a non-NULL *pbs this function never even
+ * had a strong reference to bs. */
+ bdrv_unref(snapshot_bs);
+ }
+ }
+
+ if (!*pbs) {
+ *pbs = bs;
}
return 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 04/31] tests: Drop BDS from test-throttle.c
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 03/31] block: Let bdrv_open_inherit() return the snapshot Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 05/31] block: Drop blk_new_with_bs() Kevin Wolf
` (27 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Now that throttling has been moved to the BlockBackend level, we do not
need to create a BDS along with the BB in the I/O throttling test.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-throttle.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 5ec966c..d7fb0a6 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -578,9 +578,9 @@ static void test_groups(void)
BlockBackend *blk1, *blk2, *blk3;
BlockBackendPublic *blkp1, *blkp2, *blkp3;
- blk1 = blk_new_with_bs(&error_abort);
- blk2 = blk_new_with_bs(&error_abort);
- blk3 = blk_new_with_bs(&error_abort);
+ blk1 = blk_new(&error_abort);
+ blk2 = blk_new(&error_abort);
+ blk3 = blk_new(&error_abort);
blkp1 = blk_get_public(blk1);
blkp2 = blk_get_public(blk2);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 05/31] block: Drop blk_new_with_bs()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 04/31] tests: Drop BDS from test-throttle.c Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 06/31] block: Drop bdrv_new_root() Kevin Wolf
` (26 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Its only caller is blk_new_open(), so we can just inline it there.
The bdrv_new_root() call is dropped in the process because we can just
let bdrv_open() create the BDS.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 30 +++++++-----------------------
include/sysemu/block-backend.h | 1 -
2 files changed, 7 insertions(+), 24 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 9f306f8..b96323f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -136,27 +136,7 @@ BlockBackend *blk_new(Error **errp)
}
/*
- * Create a new BlockBackend with a new BlockDriverState attached.
- * Otherwise just like blk_new(), which see.
- */
-BlockBackend *blk_new_with_bs(Error **errp)
-{
- BlockBackend *blk;
- BlockDriverState *bs;
-
- blk = blk_new(errp);
- if (!blk) {
- return NULL;
- }
-
- bs = bdrv_new_root();
- blk->root = bdrv_root_attach_child(bs, "root", &child_root);
- blk->root->opaque = blk;
- return blk;
-}
-
-/*
- * Calls blk_new_with_bs() and then calls bdrv_open() on the BlockDriverState.
+ * Creates a new BlockBackend, opens a new BlockDriverState, and connects both.
*
* Just as with bdrv_open(), after having called this function the reference to
* @options belongs to the block layer (even on failure).
@@ -171,21 +151,25 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp)
{
BlockBackend *blk;
+ BlockDriverState *bs;
int ret;
- blk = blk_new_with_bs(errp);
+ blk = blk_new(errp);
if (!blk) {
QDECREF(options);
return NULL;
}
- ret = bdrv_open(&blk->root->bs, filename, reference, options, flags, errp);
+ bs = NULL;
+ ret = bdrv_open(&bs, filename, reference, options, flags, errp);
if (ret < 0) {
blk_unref(blk);
return NULL;
}
blk_set_enable_write_cache(blk, true);
+ blk->root = bdrv_root_attach_child(bs, "root", &child_root);
+ blk->root->opaque = blk;
return blk;
}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 68d92b5..d0db3c3 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -79,7 +79,6 @@ typedef struct BlockBackendPublic {
} BlockBackendPublic;
BlockBackend *blk_new(Error **errp);
-BlockBackend *blk_new_with_bs(Error **errp);
BlockBackend *blk_new_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
int blk_get_refcnt(BlockBackend *blk);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 06/31] block: Drop bdrv_new_root()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 05/31] block: Drop blk_new_with_bs() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 07/31] block: Make bdrv_open() return a BDS Kevin Wolf
` (25 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
It is unused now, so we may just as well drop it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 5 -----
include/block/block.h | 1 -
2 files changed, 6 deletions(-)
diff --git a/block.c b/block.c
index 17ee088..ed4b487 100644
--- a/block.c
+++ b/block.c
@@ -220,11 +220,6 @@ void bdrv_register(BlockDriver *bdrv)
QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
}
-BlockDriverState *bdrv_new_root(void)
-{
- return bdrv_new();
-}
-
BlockDriverState *bdrv_new(void)
{
BlockDriverState *bs;
diff --git a/include/block/block.h b/include/block/block.h
index 770ca26..348e478 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -197,7 +197,6 @@ BlockDriver *bdrv_find_format(const char *format_name);
int bdrv_create(BlockDriver *drv, const char* filename,
QemuOpts *opts, Error **errp);
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
-BlockDriverState *bdrv_new_root(void);
BlockDriverState *bdrv_new(void);
void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
void bdrv_replace_in_backing_chain(BlockDriverState *old,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 07/31] block: Make bdrv_open() return a BDS
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 06/31] block: Drop bdrv_new_root() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 08/31] block: Assert !bs->refcnt in bdrv_close() Kevin Wolf
` (24 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
There are no callers to bdrv_open() or bdrv_open_inherit() left that
pass a pointer to a non-NULL BDS pointer as the first argument of these
functions, so we can finally drop that parameter and just make them
return the new BDS.
Generally, the following pattern is applied:
bs = NULL;
ret = bdrv_open(&bs, ..., &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
...
}
by
bs = bdrv_open(..., errp);
if (!bs) {
ret = -EINVAL;
...
}
Of course, there are only a few instances where the pattern is really
pure.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 138 ++++++++++++++++----------------------------------
block/block-backend.c | 6 +--
block/vvfat.c | 8 +--
blockdev.c | 38 +++++---------
include/block/block.h | 4 +-
5 files changed, 66 insertions(+), 128 deletions(-)
diff --git a/block.c b/block.c
index ed4b487..ad4be90 100644
--- a/block.c
+++ b/block.c
@@ -64,10 +64,12 @@ static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
static QLIST_HEAD(, BlockDriver) bdrv_drivers =
QLIST_HEAD_INITIALIZER(bdrv_drivers);
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- BlockDriverState *parent,
- const BdrvChildRole *child_role, Error **errp);
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+ const char *reference,
+ QDict *options, int flags,
+ BlockDriverState *parent,
+ const BdrvChildRole *child_role,
+ Error **errp);
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -1336,14 +1338,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
qdict_put(options, "driver", qstring_from_str(bs->backing_format));
}
- backing_hd = NULL;
- ret = bdrv_open_inherit(&backing_hd,
- *backing_filename ? backing_filename : NULL,
- reference, options, 0, bs, &child_backing,
- errp);
- if (ret < 0) {
+ backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
+ reference, options, 0, bs, &child_backing,
+ errp);
+ if (!backing_hd) {
bs->open_flags |= BDRV_O_NO_BACKING;
error_prepend(errp, "Could not open backing file: ");
+ ret = -EINVAL;
goto free_exit;
}
@@ -1383,7 +1384,6 @@ BdrvChild *bdrv_open_child(const char *filename,
BdrvChild *c = NULL;
BlockDriverState *bs;
QDict *image_options;
- int ret;
char *bdref_key_dot;
const char *reference;
@@ -1403,10 +1403,9 @@ BdrvChild *bdrv_open_child(const char *filename,
goto done;
}
- bs = NULL;
- ret = bdrv_open_inherit(&bs, filename, reference, image_options, 0,
- parent, child_role, errp);
- if (ret < 0) {
+ bs = bdrv_open_inherit(filename, reference, image_options, 0,
+ parent, child_role, errp);
+ if (!bs) {
goto done;
}
@@ -1427,7 +1426,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot;
- Error *local_err = NULL;
int ret;
/* if snapshot, we create a temporary backing file and open it
@@ -1466,12 +1464,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
qdict_put(snapshot_options, "driver",
qstring_from_str("qcow2"));
- bs_snapshot = NULL;
- ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
- flags, &local_err);
+ bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp);
snapshot_options = NULL;
- if (ret < 0) {
- error_propagate(errp, local_err);
+ if (!bs_snapshot) {
+ ret = -EINVAL;
goto out;
}
@@ -1505,10 +1501,12 @@ out:
* should be opened. If specified, neither options nor a filename may be given,
* nor can an existing BDS be reused (that is, *pbs has to be NULL).
*/
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- BlockDriverState *parent,
- const BdrvChildRole *child_role, Error **errp)
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+ const char *reference,
+ QDict *options, int flags,
+ BlockDriverState *parent,
+ const BdrvChildRole *child_role,
+ Error **errp)
{
int ret;
BdrvChild *file = NULL;
@@ -1520,7 +1518,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
QDict *snapshot_options = NULL;
int snapshot_flags = 0;
- assert(pbs);
assert(!child_role || !flags);
assert(!child_role == !parent);
@@ -1528,33 +1525,22 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
bool options_non_empty = options ? qdict_size(options) : false;
QDECREF(options);
- if (*pbs) {
- error_setg(errp, "Cannot reuse an existing BDS when referencing "
- "another block device");
- return -EINVAL;
- }
-
if (filename || options_non_empty) {
error_setg(errp, "Cannot reference an existing block device with "
"additional options or a new filename");
- return -EINVAL;
+ return NULL;
}
bs = bdrv_lookup_bs(reference, reference, errp);
if (!bs) {
- return -ENODEV;
+ return NULL;
}
bdrv_ref(bs);
- *pbs = bs;
- return 0;
+ return bs;
}
- if (*pbs) {
- bs = *pbs;
- } else {
- bs = bdrv_new();
- }
+ bs = bdrv_new();
/* NULL means an empty set of options */
if (options == NULL) {
@@ -1564,7 +1550,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
/* json: syntax counts as explicit options, as if in the QDict */
parse_json_protocol(options, &filename, &local_err);
if (local_err) {
- ret = -EINVAL;
goto fail;
}
@@ -1591,7 +1576,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
drv = bdrv_find_format(drvname);
if (!drv) {
error_setg(errp, "Unknown driver: '%s'", drvname);
- ret = -EINVAL;
goto fail;
}
}
@@ -1621,7 +1605,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
file = bdrv_open_child(filename, options, "file", bs,
&child_file, true, &local_err);
if (local_err) {
- ret = -EINVAL;
goto fail;
}
}
@@ -1648,7 +1631,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
qdict_put(options, "driver", qstring_from_str(drv->format_name));
} else if (!drv) {
error_setg(errp, "Must specify either driver or file");
- ret = -EINVAL;
goto fail;
}
@@ -1691,7 +1673,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
drv->format_name, entry->key);
}
- ret = -EINVAL;
goto close_and_fail;
}
@@ -1702,7 +1683,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
&& !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
error_setg(errp,
"Guest must be stopped for opening of encrypted image");
- ret = -EBUSY;
goto close_and_fail;
}
@@ -1716,36 +1696,17 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
snapshot_options, &local_err);
snapshot_options = NULL;
if (local_err) {
- ret = -EINVAL;
goto close_and_fail;
}
- if (!*pbs) {
- /* We are not going to return bs but the overlay on top of it
- * (snapshot_bs); thus, we have to drop the strong reference to bs
- * (which we obtained by calling bdrv_new()). bs will not be
- * deleted, though, because the overlay still has a reference to it.
- */
- bdrv_unref(bs);
- bs = snapshot_bs;
- } else {
- /* We are not going to return snapshot_bs, so we have to drop the
- * strong reference to it (which was returned by
- * bdrv_append_temp_snapshot()). snapshot_bs will not be deleted,
- * though, because bdrv_append_temp_snapshot() made all parental
- * references to bs (*pbs) point to snapshot_bs.
- * In fact, if *pbs was not NULL, we are not going to return any new
- * BDS. But we do not need to decrement bs's refcount here as is
- * done above, because with a non-NULL *pbs this function never even
- * had a strong reference to bs. */
- bdrv_unref(snapshot_bs);
- }
- }
-
- if (!*pbs) {
- *pbs = bs;
+ /* We are not going to return bs but the overlay on top of it
+ * (snapshot_bs); thus, we have to drop the strong reference to bs
+ * (which we obtained by calling bdrv_new()). bs will not be deleted,
+ * though, because the overlay still has a reference to it. */
+ bdrv_unref(bs);
+ bs = snapshot_bs;
}
- return 0;
+ return bs;
fail:
if (file != NULL) {
@@ -1756,36 +1717,26 @@ fail:
QDECREF(bs->options);
QDECREF(options);
bs->options = NULL;
- if (!*pbs) {
- /* If *pbs is NULL, a new BDS has been created in this function and
- needs to be freed now. Otherwise, it does not need to be closed,
- since it has not really been opened yet. */
- bdrv_unref(bs);
- }
+ bdrv_unref(bs);
if (local_err) {
error_propagate(errp, local_err);
}
- return ret;
+ return NULL;
close_and_fail:
- /* See fail path, but now the BDS has to be always closed */
- if (*pbs) {
- bdrv_close(bs);
- } else {
- bdrv_unref(bs);
- }
+ bdrv_unref(bs);
QDECREF(snapshot_options);
QDECREF(options);
if (local_err) {
error_propagate(errp, local_err);
}
- return ret;
+ return NULL;
}
-int bdrv_open(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags, Error **errp)
+BlockDriverState *bdrv_open(const char *filename, const char *reference,
+ QDict *options, int flags, Error **errp)
{
- return bdrv_open_inherit(pbs, filename, reference, options, flags, NULL,
+ return bdrv_open_inherit(filename, reference, options, flags, NULL,
NULL, errp);
}
@@ -3572,11 +3523,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
qstring_from_str(backing_fmt));
}
- bs = NULL;
- ret = bdrv_open(&bs, full_backing, NULL, backing_options,
- back_flags, &local_err);
+ bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
+ &local_err);
g_free(full_backing);
- if (ret < 0) {
+ if (!bs) {
goto out;
}
size = bdrv_getlength(bs);
diff --git a/block/block-backend.c b/block/block-backend.c
index b96323f..bc1f071 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -152,7 +152,6 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
{
BlockBackend *blk;
BlockDriverState *bs;
- int ret;
blk = blk_new(errp);
if (!blk) {
@@ -160,9 +159,8 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
return NULL;
}
- bs = NULL;
- ret = bdrv_open(&bs, filename, reference, options, flags, errp);
- if (ret < 0) {
+ bs = bdrv_open(filename, reference, options, flags, errp);
+ if (!bs) {
blk_unref(blk);
return NULL;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index 3e484a1..a39dbe6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2998,12 +2998,12 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
goto err;
}
- s->qcow = NULL;
options = qdict_new();
qdict_put(options, "driver", qstring_from_str("qcow"));
- ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, options,
- BDRV_O_RDWR | BDRV_O_NO_FLUSH, errp);
- if (ret < 0) {
+ s->qcow = bdrv_open(s->qcow_filename, NULL, options,
+ BDRV_O_RDWR | BDRV_O_NO_FLUSH, errp);
+ if (!s->qcow) {
+ ret = -EINVAL;
goto err;
}
diff --git a/blockdev.c b/blockdev.c
index 0e37e09..7d4a1ba 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -657,7 +657,6 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
QemuOpts *opts;
Error *local_error = NULL;
BlockdevDetectZeroesOptions detect_zeroes;
- int ret;
int bdrv_flags = 0;
opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
@@ -688,9 +687,8 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
bdrv_flags |= BDRV_O_INACTIVE;
}
- bs = NULL;
- ret = bdrv_open(&bs, NULL, NULL, bs_opts, bdrv_flags, errp);
- if (ret < 0) {
+ bs = bdrv_open(NULL, NULL, bs_opts, bdrv_flags, errp);
+ if (!bs) {
goto fail_no_bs_opts;
}
@@ -1643,7 +1641,7 @@ typedef struct ExternalSnapshotState {
static void external_snapshot_prepare(BlkActionState *common,
Error **errp)
{
- int flags = 0, ret;
+ int flags = 0;
QDict *options = NULL;
Error *local_err = NULL;
/* Device and node name of the image to generate the snapshot from */
@@ -1768,11 +1766,10 @@ static void external_snapshot_prepare(BlkActionState *common,
flags |= BDRV_O_NO_BACKING;
}
- assert(state->new_bs == NULL);
- ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
- flags, errp);
+ state->new_bs = bdrv_open(new_image_file, snapshot_ref, options, flags,
+ errp);
/* We will manually add the backing_hd field to the bs later */
- if (ret != 0) {
+ if (!state->new_bs) {
return;
}
@@ -2540,7 +2537,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
{
BlockBackend *blk;
BlockDriverState *medium_bs = NULL;
- int bdrv_flags, ret;
+ int bdrv_flags;
QDict *options = NULL;
Error *err = NULL;
@@ -2584,9 +2581,8 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
qdict_put(options, "driver", qstring_from_str(format));
}
- assert(!medium_bs);
- ret = bdrv_open(&medium_bs, filename, NULL, options, bdrv_flags, errp);
- if (ret < 0) {
+ medium_bs = bdrv_open(filename, NULL, options, bdrv_flags, errp);
+ if (!medium_bs) {
goto fail;
}
@@ -3199,7 +3195,6 @@ static void do_drive_backup(const char *device, const char *target,
Error *local_err = NULL;
int flags;
int64_t size;
- int ret;
if (!has_speed) {
speed = 0;
@@ -3283,10 +3278,8 @@ static void do_drive_backup(const char *device, const char *target,
qdict_put(options, "driver", qstring_from_str(format));
}
- target_bs = NULL;
- ret = bdrv_open(&target_bs, target, NULL, options, flags, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
+ target_bs = bdrv_open(target, NULL, options, flags, errp);
+ if (!target_bs) {
goto out;
}
@@ -3511,7 +3504,6 @@ void qmp_drive_mirror(const char *device, const char *target,
QDict *options = NULL;
int flags;
int64_t size;
- int ret;
blk = blk_by_name(device);
if (!blk) {
@@ -3620,11 +3612,9 @@ void qmp_drive_mirror(const char *device, const char *target,
/* Mirroring takes care of copy-on-write using the source's backing
* file.
*/
- target_bs = NULL;
- ret = bdrv_open(&target_bs, target, NULL, options,
- flags | BDRV_O_NO_BACKING, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
+ target_bs = bdrv_open(target, NULL, options, flags | BDRV_O_NO_BACKING,
+ errp);
+ if (!target_bs) {
goto out;
}
diff --git a/include/block/block.h b/include/block/block.h
index 348e478..e899f7d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -212,8 +212,8 @@ BdrvChild *bdrv_open_child(const char *filename,
void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
-int bdrv_open(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags, Error **errp);
+BlockDriverState *bdrv_open(const char *filename, const char *reference,
+ QDict *options, int flags, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
QDict *options, int flags);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 08/31] block: Assert !bs->refcnt in bdrv_close()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 07/31] block: Make bdrv_open() return a BDS Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 09/31] block: Drop bdrv_parent_cb_...() from bdrv_close() Kevin Wolf
` (23 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
The only caller of bdrv_close() left is bdrv_delete(). We may as well
assert that, in a way (there are some things in bdrv_close() that make
more sense under that assumption, such as the call to
bdrv_release_all_dirty_bitmaps() which in turn assumes that no frozen
bitmaps are attached to the BDS).
In addition, being called only in bdrv_delete() means that we can drop
bdrv_close()'s forward declaration at the top of block.c.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block.c b/block.c
index ad4be90..4c021c0 100644
--- a/block.c
+++ b/block.c
@@ -74,8 +74,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
-static void bdrv_close(BlockDriverState *bs);
-
#ifdef _WIN32
static int is_windows_drive_prefix(const char *filename)
{
@@ -2110,6 +2108,7 @@ static void bdrv_close(BlockDriverState *bs)
BdrvAioNotifier *ban, *ban_next;
assert(!bs->job);
+ assert(!bs->refcnt);
bdrv_drained_begin(bs); /* complete I/O */
bdrv_flush(bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 09/31] block: Drop bdrv_parent_cb_...() from bdrv_close()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 08/31] block: Assert !bs->refcnt in bdrv_close() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 10/31] block: Drop errp parameter from blk_new() Kevin Wolf
` (22 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
bdrv_close() now asserts that the BDS's refcount is 0, therefore it
cannot have any parents and the bdrv_parent_cb_change_media() call is a
no-op.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/block.c b/block.c
index 4c021c0..38df365 100644
--- a/block.c
+++ b/block.c
@@ -2117,8 +2117,6 @@ static void bdrv_close(BlockDriverState *bs)
bdrv_release_named_dirty_bitmaps(bs);
assert(QLIST_EMPTY(&bs->dirty_bitmaps));
- bdrv_parent_cb_change_media(bs, false);
-
if (bs->drv) {
BdrvChild *child, *next;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 10/31] block: Drop errp parameter from blk_new()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (8 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 09/31] block: Drop bdrv_parent_cb_...() from bdrv_close() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 11/31] block: Introduce bdrv_replace_child() Kevin Wolf
` (21 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
blk_new() cannot fail so its Error ** parameter has become superfluous.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 9 ++-------
blockdev.c | 6 +-----
include/sysemu/block-backend.h | 2 +-
tests/test-throttle.c | 6 +++---
4 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index bc1f071..4e8298b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -119,7 +119,7 @@ static const BdrvChildRole child_root = {
* Store an error through @errp on failure, unless it's null.
* Return the new BlockBackend on success, null on failure.
*/
-BlockBackend *blk_new(Error **errp)
+BlockBackend *blk_new(void)
{
BlockBackend *blk;
@@ -153,12 +153,7 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
BlockBackend *blk;
BlockDriverState *bs;
- blk = blk_new(errp);
- if (!blk) {
- QDECREF(options);
- return NULL;
- }
-
+ blk = blk_new();
bs = bdrv_open(filename, reference, options, flags, errp);
if (!bs) {
blk_unref(blk);
diff --git a/blockdev.c b/blockdev.c
index 7d4a1ba..af0b022 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -567,11 +567,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
if ((!file || !*file) && !qdict_size(bs_opts)) {
BlockBackendRootState *blk_rs;
- blk = blk_new(errp);
- if (!blk) {
- goto early_err;
- }
-
+ blk = blk_new();
blk_rs = blk_get_root_state(blk);
blk_rs->open_flags = bdrv_flags;
blk_rs->read_only = !(bdrv_flags & BDRV_O_RDWR);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d0db3c3..9d6615c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -78,7 +78,7 @@ typedef struct BlockBackendPublic {
QLIST_ENTRY(BlockBackendPublic) round_robin;
} BlockBackendPublic;
-BlockBackend *blk_new(Error **errp);
+BlockBackend *blk_new(void);
BlockBackend *blk_new_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
int blk_get_refcnt(BlockBackend *blk);
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index d7fb0a6..c02be80 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -578,9 +578,9 @@ static void test_groups(void)
BlockBackend *blk1, *blk2, *blk3;
BlockBackendPublic *blkp1, *blkp2, *blkp3;
- blk1 = blk_new(&error_abort);
- blk2 = blk_new(&error_abort);
- blk3 = blk_new(&error_abort);
+ blk1 = blk_new();
+ blk2 = blk_new();
+ blk3 = blk_new();
blkp1 = blk_get_public(blk1);
blkp2 = blk_get_public(blk2);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 11/31] block: Introduce bdrv_replace_child()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (9 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 10/31] block: Drop errp parameter from blk_new() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 12/31] block: Make bdrv_drain() use bdrv_drained_begin/end() Kevin Wolf
` (20 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This adds a common function that is called when attaching a new child to
a parent, removing a child from a parent and when reconfiguring the
graph so that an existing child points to a different node now.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 38df365..351344e 100644
--- a/block.c
+++ b/block.c
@@ -1150,18 +1150,32 @@ static int bdrv_fill_options(QDict **options, const char *filename,
return 0;
}
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
+{
+ BlockDriverState *old_bs = child->bs;
+
+ if (old_bs) {
+ QLIST_REMOVE(child, next_parent);
+ }
+ if (new_bs) {
+ QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+ }
+
+ child->bs = new_bs;
+}
+
BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
const char *child_name,
const BdrvChildRole *child_role)
{
BdrvChild *child = g_new(BdrvChild, 1);
*child = (BdrvChild) {
- .bs = child_bs,
+ .bs = NULL,
.name = g_strdup(child_name),
.role = child_role,
};
- QLIST_INSERT_HEAD(&child_bs->parents, child, next_parent);
+ bdrv_replace_child(child, child_bs);
return child;
}
@@ -1182,7 +1196,9 @@ static void bdrv_detach_child(BdrvChild *child)
QLIST_REMOVE(child, next);
child->next.le_prev = NULL;
}
- QLIST_REMOVE(child, next_parent);
+
+ bdrv_replace_child(child, NULL);
+
g_free(child->name);
g_free(child);
}
@@ -2203,10 +2219,8 @@ static void change_parent_backing_link(BlockDriverState *from,
QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
assert(c->role != &child_backing);
- c->bs = to;
- QLIST_REMOVE(c, next_parent);
- QLIST_INSERT_HEAD(&to->parents, c, next_parent);
bdrv_ref(to);
+ bdrv_replace_child(c, to);
bdrv_unref(from);
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 12/31] block: Make bdrv_drain() use bdrv_drained_begin/end()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (10 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 11/31] block: Introduce bdrv_replace_child() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 13/31] block: Fix reconfiguring graph with drained nodes Kevin Wolf
` (19 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Until now, bdrv_drained_begin() used bdrv_drain() internally to drain
the queue. This is kind of backwards and caused quiescing code to be
duplicated because bdrv_drained_begin() had to ensure that no new
requests come in even after bdrv_drain() returns, whereas bdrv_drain()
had to have them because it could be called from other places.
Instead move the bdrv_drain() code to bdrv_drained_begin() and make
bdrv_drain() a simple wrapper around bdrv_drained_begin/end().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block/io.c | 69 ++++++++++++++++++++++++++++++--------------------------------
1 file changed, 33 insertions(+), 36 deletions(-)
diff --git a/block/io.c b/block/io.c
index 2a28d63..9bc1d45 100644
--- a/block/io.c
+++ b/block/io.c
@@ -225,6 +225,34 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
assert(data.done);
}
+void bdrv_drained_begin(BlockDriverState *bs)
+{
+ if (!bs->quiesce_counter++) {
+ aio_disable_external(bdrv_get_aio_context(bs));
+ bdrv_parent_drained_begin(bs);
+ }
+
+ bdrv_io_unplugged_begin(bs);
+ bdrv_drain_recurse(bs);
+ if (qemu_in_coroutine()) {
+ bdrv_co_yield_to_drain(bs);
+ } else {
+ bdrv_drain_poll(bs);
+ }
+ bdrv_io_unplugged_end(bs);
+}
+
+void bdrv_drained_end(BlockDriverState *bs)
+{
+ assert(bs->quiesce_counter > 0);
+ if (--bs->quiesce_counter > 0) {
+ return;
+ }
+
+ bdrv_parent_drained_end(bs);
+ aio_enable_external(bdrv_get_aio_context(bs));
+}
+
/*
* Wait for pending requests to complete on a single BlockDriverState subtree,
* and suspend block driver's internal I/O until next request arrives.
@@ -238,26 +266,15 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
*/
void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
{
- bdrv_parent_drained_begin(bs);
- bdrv_io_unplugged_begin(bs);
- bdrv_drain_recurse(bs);
- bdrv_co_yield_to_drain(bs);
- bdrv_io_unplugged_end(bs);
- bdrv_parent_drained_end(bs);
+ assert(qemu_in_coroutine());
+ bdrv_drained_begin(bs);
+ bdrv_drained_end(bs);
}
void bdrv_drain(BlockDriverState *bs)
{
- bdrv_parent_drained_begin(bs);
- bdrv_io_unplugged_begin(bs);
- bdrv_drain_recurse(bs);
- if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs);
- } else {
- bdrv_drain_poll(bs);
- }
- bdrv_io_unplugged_end(bs);
- bdrv_parent_drained_end(bs);
+ bdrv_drained_begin(bs);
+ bdrv_drained_end(bs);
}
/*
@@ -2541,23 +2558,3 @@ void bdrv_io_unplugged_end(BlockDriverState *bs)
}
}
}
-
-void bdrv_drained_begin(BlockDriverState *bs)
-{
- if (!bs->quiesce_counter++) {
- aio_disable_external(bdrv_get_aio_context(bs));
- }
- bdrv_parent_drained_begin(bs);
- bdrv_drain(bs);
-}
-
-void bdrv_drained_end(BlockDriverState *bs)
-{
- bdrv_parent_drained_end(bs);
-
- assert(bs->quiesce_counter > 0);
- if (--bs->quiesce_counter > 0) {
- return;
- }
- aio_enable_external(bdrv_get_aio_context(bs));
-}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 13/31] block: Fix reconfiguring graph with drained nodes
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (11 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 12/31] block: Make bdrv_drain() use bdrv_drained_begin/end() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 14/31] block: Propagate .drained_begin/end callbacks Kevin Wolf
` (18 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
When changing the BlockDriverState that a BdrvChild points to while the
node is currently drained, we must call the .drained_end() parent
callback. Conversely, when this means attaching a new node that is
already drained, we need to call .drained_begin().
bdrv_root_attach_child() takes now an opaque parameter, which is needed
because the callbacks must also be called if we're attaching a new child
to the BlockBackend when the root node is already drained, and they need
a way to identify the BlockBackend. Previously, child->opaque was set
too late and the callbacks would still see it as NULL.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block.c | 18 ++++++++++++++----
block/block-backend.c | 9 +++++----
include/block/block_int.h | 3 ++-
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 351344e..598624f 100644
--- a/block.c
+++ b/block.c
@@ -1155,24 +1155,33 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
BlockDriverState *old_bs = child->bs;
if (old_bs) {
+ if (old_bs->quiesce_counter && child->role->drained_end) {
+ child->role->drained_end(child);
+ }
QLIST_REMOVE(child, next_parent);
}
+
+ child->bs = new_bs;
+
if (new_bs) {
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+ if (new_bs->quiesce_counter && child->role->drained_begin) {
+ child->role->drained_begin(child);
+ }
}
-
- child->bs = new_bs;
}
BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
const char *child_name,
- const BdrvChildRole *child_role)
+ const BdrvChildRole *child_role,
+ void *opaque)
{
BdrvChild *child = g_new(BdrvChild, 1);
*child = (BdrvChild) {
.bs = NULL,
.name = g_strdup(child_name),
.role = child_role,
+ .opaque = opaque,
};
bdrv_replace_child(child, child_bs);
@@ -1185,7 +1194,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
const char *child_name,
const BdrvChildRole *child_role)
{
- BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role);
+ BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role,
+ NULL);
QLIST_INSERT_HEAD(&parent_bs->children, child, next);
return child;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index 4e8298b..14e528e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -161,8 +161,7 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
}
blk_set_enable_write_cache(blk, true);
- blk->root = bdrv_root_attach_child(bs, "root", &child_root);
- blk->root->opaque = blk;
+ blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
return blk;
}
@@ -481,8 +480,7 @@ void blk_remove_bs(BlockBackend *blk)
void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
{
bdrv_ref(bs);
- blk->root = bdrv_root_attach_child(bs, "root", &child_root);
- blk->root->opaque = blk;
+ blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
notifier_list_notify(&blk->insert_bs_notifiers, blk);
if (blk->public.throttle_state) {
@@ -1676,6 +1674,9 @@ static void blk_root_drained_begin(BdrvChild *child)
{
BlockBackend *blk = child->opaque;
+ /* Note that blk->root may not be accessible here yet if we are just
+ * attaching to a BlockDriverState that is drained. Use child instead. */
+
if (blk->public.io_limits_disabled++ == 0) {
throttle_group_restart_blk(blk);
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b6f4755..30a9717 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -719,7 +719,8 @@ void hmp_drive_add_node(Monitor *mon, const char *optstr);
BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
const char *child_name,
- const BdrvChildRole *child_role);
+ const BdrvChildRole *child_role,
+ void *opaque);
void bdrv_root_unref_child(BdrvChild *child);
const char *bdrv_get_parent_name(const BlockDriverState *bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 14/31] block: Propagate .drained_begin/end callbacks
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (12 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 13/31] block: Fix reconfiguring graph with drained nodes Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 15/31] dma-helpers: change interface to byte-based Kevin Wolf
` (17 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
When draining intermediate nodes (i.e. nodes that aren't the root node
for at least one of their parents; with node references, the user can
always configure the graph to create this situation), we need to
propagate the .drained_begin/end callbacks all the way up to the root
for the drain to be effective.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
block.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 598624f..45cddd6 100644
--- a/block.c
+++ b/block.c
@@ -659,6 +659,18 @@ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)
return 0;
}
+static void bdrv_child_cb_drained_begin(BdrvChild *child)
+{
+ BlockDriverState *bs = child->opaque;
+ bdrv_drained_begin(bs);
+}
+
+static void bdrv_child_cb_drained_end(BdrvChild *child)
+{
+ BlockDriverState *bs = child->opaque;
+ bdrv_drained_end(bs);
+}
+
/*
* Returns the options and flags that a temporary snapshot should get, based on
* the originally requested flags (the originally requested image will have
@@ -705,6 +717,8 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
const BdrvChildRole child_file = {
.inherit_options = bdrv_inherited_options,
+ .drained_begin = bdrv_child_cb_drained_begin,
+ .drained_end = bdrv_child_cb_drained_end,
};
/*
@@ -723,6 +737,8 @@ static void bdrv_inherited_fmt_options(int *child_flags, QDict *child_options,
const BdrvChildRole child_format = {
.inherit_options = bdrv_inherited_fmt_options,
+ .drained_begin = bdrv_child_cb_drained_begin,
+ .drained_end = bdrv_child_cb_drained_end,
};
/*
@@ -750,6 +766,8 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
static const BdrvChildRole child_backing = {
.inherit_options = bdrv_backing_options,
+ .drained_begin = bdrv_child_cb_drained_begin,
+ .drained_end = bdrv_child_cb_drained_end,
};
static int bdrv_open_flags(BlockDriverState *bs, int flags)
@@ -1195,7 +1213,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
const BdrvChildRole *child_role)
{
BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role,
- NULL);
+ parent_bs);
QLIST_INSERT_HEAD(&parent_bs->children, child, next);
return child;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 15/31] dma-helpers: change interface to byte-based
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (13 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 14/31] block: Propagate .drained_begin/end callbacks Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 16/31] dma-helpers: change BlockBackend to opaque value in DMAIOFunc Kevin Wolf
` (16 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
dma-helpers.c | 14 +++++++-------
hw/block/nvme.c | 6 +++---
hw/ide/ahci.c | 6 ++++--
hw/ide/core.c | 8 +++++---
hw/scsi/scsi-disk.c | 6 ++++--
include/sysemu/dma.h | 6 +++---
trace-events | 2 +-
7 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/dma-helpers.c b/dma-helpers.c
index a6cc15f..af07aab 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -192,18 +192,18 @@ static const AIOCBInfo dma_aiocb_info = {
};
BlockAIOCB *dma_blk_io(
- BlockBackend *blk, QEMUSGList *sg, uint64_t sector_num,
+ BlockBackend *blk, QEMUSGList *sg, uint64_t offset,
DMAIOFunc *io_func, BlockCompletionFunc *cb,
void *opaque, DMADirection dir)
{
DMAAIOCB *dbs = blk_aio_get(&dma_aiocb_info, blk, cb, opaque);
- trace_dma_blk_io(dbs, blk, sector_num, (dir == DMA_DIRECTION_TO_DEVICE));
+ trace_dma_blk_io(dbs, blk, offset, (dir == DMA_DIRECTION_TO_DEVICE));
dbs->acb = NULL;
dbs->blk = blk;
dbs->sg = sg;
- dbs->offset = sector_num << BDRV_SECTOR_BITS;
+ dbs->offset = offset;
dbs->sg_cur_index = 0;
dbs->sg_cur_byte = 0;
dbs->dir = dir;
@@ -216,18 +216,18 @@ BlockAIOCB *dma_blk_io(
BlockAIOCB *dma_blk_read(BlockBackend *blk,
- QEMUSGList *sg, uint64_t sector,
+ QEMUSGList *sg, uint64_t offset,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk, sg, sector, blk_aio_preadv, cb, opaque,
+ return dma_blk_io(blk, sg, offset, blk_aio_preadv, cb, opaque,
DMA_DIRECTION_FROM_DEVICE);
}
BlockAIOCB *dma_blk_write(BlockBackend *blk,
- QEMUSGList *sg, uint64_t sector,
+ QEMUSGList *sg, uint64_t offset,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk, sg, sector, blk_aio_pwritev, cb, opaque,
+ return dma_blk_io(blk, sg, offset, blk_aio_pwritev, cb, opaque,
DMA_DIRECTION_TO_DEVICE);
}
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 173988e..9faad29 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -239,7 +239,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
uint64_t data_size = (uint64_t)nlb << data_shift;
- uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS);
+ uint64_t data_offset = slba << data_shift;
int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
@@ -258,8 +258,8 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
req->has_sg = true;
dma_acct_start(n->conf.blk, &req->acct, &req->qsg, acct);
req->aiocb = is_write ?
- dma_blk_write(n->conf.blk, &req->qsg, aio_slba, nvme_rw_cb, req) :
- dma_blk_read(n->conf.blk, &req->qsg, aio_slba, nvme_rw_cb, req);
+ dma_blk_write(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req) :
+ dma_blk_read(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req);
return NVME_NO_COMPLETE;
}
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f244bc0..502d4f1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1006,7 +1006,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
dma_acct_start(ide_state->blk, &ncq_tfs->acct,
&ncq_tfs->sglist, BLOCK_ACCT_READ);
ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist,
- ncq_tfs->lba, ncq_cb, ncq_tfs);
+ ncq_tfs->lba << BDRV_SECTOR_BITS,
+ ncq_cb, ncq_tfs);
break;
case WRITE_FPDMA_QUEUED:
DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
@@ -1018,7 +1019,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
dma_acct_start(ide_state->blk, &ncq_tfs->acct,
&ncq_tfs->sglist, BLOCK_ACCT_WRITE);
ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
- ncq_tfs->lba, ncq_cb, ncq_tfs);
+ ncq_tfs->lba << BDRV_SECTOR_BITS,
+ ncq_cb, ncq_tfs);
break;
default:
DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n",
diff --git a/hw/ide/core.c b/hw/ide/core.c
index fe2bfba..7326189 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -799,6 +799,7 @@ static void ide_dma_cb(void *opaque, int ret)
IDEState *s = opaque;
int n;
int64_t sector_num;
+ uint64_t offset;
bool stay_active = false;
if (ret == -ECANCELED) {
@@ -859,17 +860,18 @@ static void ide_dma_cb(void *opaque, int ret)
return;
}
+ offset = sector_num << BDRV_SECTOR_BITS;
switch (s->dma_cmd) {
case IDE_DMA_READ:
- s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, offset,
ide_dma_cb, s);
break;
case IDE_DMA_WRITE:
- s->bus->dma->aiocb = dma_blk_write(s->blk, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_blk_write(s->blk, &s->sg, offset,
ide_dma_cb, s);
break;
case IDE_DMA_TRIM:
- s->bus->dma->aiocb = dma_blk_io(s->blk, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_blk_io(s->blk, &s->sg, offset,
ide_issue_trim, ide_dma_cb, s,
DMA_DIRECTION_TO_DEVICE);
break;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ce89c98..c374ea8 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -335,7 +335,8 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
if (r->req.sg) {
dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_READ);
r->req.resid -= r->req.sg->size;
- r->req.aiocb = dma_blk_read(s->qdev.conf.blk, r->req.sg, r->sector,
+ r->req.aiocb = dma_blk_read(s->qdev.conf.blk, r->req.sg,
+ r->sector << BDRV_SECTOR_BITS,
scsi_dma_complete, r);
} else {
scsi_init_iovec(r, SCSI_DMA_BUF_SIZE);
@@ -539,7 +540,8 @@ static void scsi_write_data(SCSIRequest *req)
if (r->req.sg) {
dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE);
r->req.resid -= r->req.sg->size;
- r->req.aiocb = dma_blk_write(s->qdev.conf.blk, r->req.sg, r->sector,
+ r->req.aiocb = dma_blk_write(s->qdev.conf.blk, r->req.sg,
+ r->sector << BDRV_SECTOR_BITS,
scsi_dma_complete, r);
} else {
block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index d6e96a4..f0e0c30 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -199,14 +199,14 @@ typedef BlockAIOCB *DMAIOFunc(BlockBackend *blk, int64_t offset,
BlockCompletionFunc *cb, void *opaque);
BlockAIOCB *dma_blk_io(BlockBackend *blk,
- QEMUSGList *sg, uint64_t sector_num,
+ QEMUSGList *sg, uint64_t offset,
DMAIOFunc *io_func, BlockCompletionFunc *cb,
void *opaque, DMADirection dir);
BlockAIOCB *dma_blk_read(BlockBackend *blk,
- QEMUSGList *sg, uint64_t sector,
+ QEMUSGList *sg, uint64_t offset,
BlockCompletionFunc *cb, void *opaque);
BlockAIOCB *dma_blk_write(BlockBackend *blk,
- QEMUSGList *sg, uint64_t sector,
+ QEMUSGList *sg, uint64_t offset,
BlockCompletionFunc *cb, void *opaque);
uint64_t dma_buf_read(uint8_t *ptr, int32_t len, QEMUSGList *sg);
uint64_t dma_buf_write(uint8_t *ptr, int32_t len, QEMUSGList *sg);
diff --git a/trace-events b/trace-events
index b53c354..e01b436 100644
--- a/trace-events
+++ b/trace-events
@@ -1143,7 +1143,7 @@ win_helper_done(uint32_t tl) "tl=%d"
win_helper_retry(uint32_t tl) "tl=%d"
# dma-helpers.c
-dma_blk_io(void *dbs, void *bs, int64_t sector_num, bool to_dev) "dbs=%p bs=%p sector_num=%" PRId64 " to_dev=%d"
+dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
dma_aio_cancel(void *dbs) "dbs=%p"
dma_complete(void *dbs, int ret, void *cb) "dbs=%p ret=%d cb=%p"
dma_blk_cb(void *dbs, int ret) "dbs=%p ret=%d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 16/31] dma-helpers: change BlockBackend to opaque value in DMAIOFunc
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (14 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 15/31] dma-helpers: change interface to byte-based Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 17/31] block: Rename blk_write_zeroes() Kevin Wolf
` (15 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Paolo Bonzini <pbonzini@redhat.com>
Callers of dma_blk_io have no way to pass extra data to the DMAIOFunc,
because the original callback and opaque are gone by the time DMAIOFunc
is called. On the other hand, the BlockBackend is usually derived
from those extra data that you could pass to the DMAIOFunc (in the
next patch, that would be the SCSIRequest).
So change DMAIOFunc's prototype, decoupling it from blk_aio_readv
and blk_aio_writev's. The new prototype loses the BlockBackend
and gains an extra opaque value which, in the case of dma_blk_readv
and dma_blk_writev, is of course used for the BlockBackend.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
dma-helpers.c | 48 +++++++++++++++++++++++++++++++++++-------------
hw/ide/core.c | 14 ++++++++------
hw/ide/internal.h | 6 +++---
hw/ide/macio.c | 2 +-
include/sysemu/dma.h | 12 ++++++------
5 files changed, 53 insertions(+), 29 deletions(-)
diff --git a/dma-helpers.c b/dma-helpers.c
index af07aab..b521d84 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -70,7 +70,7 @@ void qemu_sglist_destroy(QEMUSGList *qsg)
typedef struct {
BlockAIOCB common;
- BlockBackend *blk;
+ AioContext *ctx;
BlockAIOCB *acb;
QEMUSGList *sg;
uint64_t offset;
@@ -80,6 +80,7 @@ typedef struct {
QEMUIOVector iov;
QEMUBH *bh;
DMAIOFunc *io_func;
+ void *io_func_opaque;
} DMAAIOCB;
static void dma_blk_cb(void *opaque, int ret);
@@ -154,8 +155,7 @@ static void dma_blk_cb(void *opaque, int ret)
if (dbs->iov.size == 0) {
trace_dma_map_wait(dbs);
- dbs->bh = aio_bh_new(blk_get_aio_context(dbs->blk),
- reschedule_dma, dbs);
+ dbs->bh = aio_bh_new(dbs->ctx, reschedule_dma, dbs);
cpu_register_map_client(dbs->bh);
return;
}
@@ -164,8 +164,8 @@ static void dma_blk_cb(void *opaque, int ret)
qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
}
- dbs->acb = dbs->io_func(dbs->blk, dbs->offset, &dbs->iov, 0,
- dma_blk_cb, dbs);
+ dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
+ dma_blk_cb, dbs, dbs->io_func_opaque);
assert(dbs->acb);
}
@@ -191,23 +191,25 @@ static const AIOCBInfo dma_aiocb_info = {
.cancel_async = dma_aio_cancel,
};
-BlockAIOCB *dma_blk_io(
- BlockBackend *blk, QEMUSGList *sg, uint64_t offset,
- DMAIOFunc *io_func, BlockCompletionFunc *cb,
+BlockAIOCB *dma_blk_io(AioContext *ctx,
+ QEMUSGList *sg, uint64_t offset,
+ DMAIOFunc *io_func, void *io_func_opaque,
+ BlockCompletionFunc *cb,
void *opaque, DMADirection dir)
{
- DMAAIOCB *dbs = blk_aio_get(&dma_aiocb_info, blk, cb, opaque);
+ DMAAIOCB *dbs = qemu_aio_get(&dma_aiocb_info, NULL, cb, opaque);
- trace_dma_blk_io(dbs, blk, offset, (dir == DMA_DIRECTION_TO_DEVICE));
+ trace_dma_blk_io(dbs, io_func_opaque, offset, (dir == DMA_DIRECTION_TO_DEVICE));
dbs->acb = NULL;
- dbs->blk = blk;
dbs->sg = sg;
+ dbs->ctx = ctx;
dbs->offset = offset;
dbs->sg_cur_index = 0;
dbs->sg_cur_byte = 0;
dbs->dir = dir;
dbs->io_func = io_func;
+ dbs->io_func_opaque = io_func_opaque;
dbs->bh = NULL;
qemu_iovec_init(&dbs->iov, sg->nsg);
dma_blk_cb(dbs, 0);
@@ -215,19 +217,39 @@ BlockAIOCB *dma_blk_io(
}
+static
+BlockAIOCB *dma_blk_read_io_func(int64_t offset, QEMUIOVector *iov,
+ BlockCompletionFunc *cb, void *cb_opaque,
+ void *opaque)
+{
+ BlockBackend *blk = opaque;
+ return blk_aio_preadv(blk, offset, iov, 0, cb, cb_opaque);
+}
+
BlockAIOCB *dma_blk_read(BlockBackend *blk,
QEMUSGList *sg, uint64_t offset,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk, sg, offset, blk_aio_preadv, cb, opaque,
+ return dma_blk_io(blk_get_aio_context(blk),
+ sg, offset, dma_blk_read_io_func, blk, cb, opaque,
DMA_DIRECTION_FROM_DEVICE);
}
+static
+BlockAIOCB *dma_blk_write_io_func(int64_t offset, QEMUIOVector *iov,
+ BlockCompletionFunc *cb, void *cb_opaque,
+ void *opaque)
+{
+ BlockBackend *blk = opaque;
+ return blk_aio_pwritev(blk, offset, iov, 0, cb, cb_opaque);
+}
+
BlockAIOCB *dma_blk_write(BlockBackend *blk,
QEMUSGList *sg, uint64_t offset,
void (*cb)(void *opaque, int ret), void *opaque)
{
- return dma_blk_io(blk, sg, offset, blk_aio_pwritev, cb, opaque,
+ return dma_blk_io(blk_get_aio_context(blk),
+ sg, offset, dma_blk_write_io_func, blk, cb, opaque,
DMA_DIRECTION_TO_DEVICE);
}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7326189..029f6b9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -441,13 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
}
}
-BlockAIOCB *ide_issue_trim(BlockBackend *blk,
- int64_t offset, QEMUIOVector *qiov, BdrvRequestFlags flags,
- BlockCompletionFunc *cb, void *opaque)
+BlockAIOCB *ide_issue_trim(
+ int64_t offset, QEMUIOVector *qiov,
+ BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
{
+ BlockBackend *blk = opaque;
TrimAIOCB *iocb;
- iocb = blk_aio_get(&trim_aiocb_info, blk, cb, opaque);
+ iocb = blk_aio_get(&trim_aiocb_info, blk, cb, cb_opaque);
iocb->blk = blk;
iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
iocb->ret = 0;
@@ -871,8 +872,9 @@ static void ide_dma_cb(void *opaque, int ret)
ide_dma_cb, s);
break;
case IDE_DMA_TRIM:
- s->bus->dma->aiocb = dma_blk_io(s->blk, &s->sg, offset,
- ide_issue_trim, ide_dma_cb, s,
+ s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
+ &s->sg, offset,
+ ide_issue_trim, s->blk, ide_dma_cb, s,
DMA_DIRECTION_TO_DEVICE);
break;
default:
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index ceb9e59..773928a 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -613,9 +613,9 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
EndTransferFunc *end_transfer_func);
void ide_transfer_stop(IDEState *s);
void ide_set_inactive(IDEState *s, bool more);
-BlockAIOCB *ide_issue_trim(BlockBackend *blk,
- int64_t offset, QEMUIOVector *qiov, BdrvRequestFlags flags,
- BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *ide_issue_trim(
+ int64_t offset, QEMUIOVector *qiov,
+ BlockCompletionFunc *cb, void *cb_opaque, void *opaque);
BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
QEMUIOVector *iov, int nb_sectors,
BlockCompletionFunc *cb, void *opaque);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index d7d9c0f..42ad68a 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -230,7 +230,7 @@ static void pmac_dma_trim(BlockBackend *blk,
s->io_buffer_index += io->len;
io->len = 0;
- s->bus->dma->aiocb = ide_issue_trim(blk, offset, &io->iov, 0, cb, io);
+ s->bus->dma->aiocb = ide_issue_trim(offset, &io->iov, cb, io, blk);
}
static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index f0e0c30..34c8eaf 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -194,14 +194,14 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len);
void qemu_sglist_destroy(QEMUSGList *qsg);
#endif
-typedef BlockAIOCB *DMAIOFunc(BlockBackend *blk, int64_t offset,
- QEMUIOVector *iov, BdrvRequestFlags flags,
- BlockCompletionFunc *cb, void *opaque);
+typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov,
+ BlockCompletionFunc *cb, void *cb_opaque,
+ void *opaque);
-BlockAIOCB *dma_blk_io(BlockBackend *blk,
+BlockAIOCB *dma_blk_io(AioContext *ctx,
QEMUSGList *sg, uint64_t offset,
- DMAIOFunc *io_func, BlockCompletionFunc *cb,
- void *opaque, DMADirection dir);
+ DMAIOFunc *io_func, void *io_func_opaque,
+ BlockCompletionFunc *cb, void *opaque, DMADirection dir);
BlockAIOCB *dma_blk_read(BlockBackend *blk,
QEMUSGList *sg, uint64_t offset,
BlockCompletionFunc *cb, void *opaque);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 17/31] block: Rename blk_write_zeroes()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (15 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 16/31] dma-helpers: change BlockBackend to opaque value in DMAIOFunc Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 18/31] block: keep a list of block jobs Kevin Wolf
` (14 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Eric Blake <eblake@redhat.com>
Commit 983a1600 changed the semantics of blk_write_zeroes() to
be byte-based rather than sector-based, but did not change the
name, which is an open invitation for other code to misuse the
function. Renaming to pwrite_zeroes() makes it more in line
with other byte-based interfaces, and will help make it easier
to track which remaining write_zeroes interfaces still need
conversion.
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/block-backend.c | 14 +++++++-------
block/parallels.c | 4 ++--
hw/scsi/scsi-disk.c | 2 +-
include/sysemu/block-backend.h | 14 +++++++-------
qemu-img.c | 4 ++--
qemu-io-cmds.c | 22 +++++++++++-----------
6 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 14e528e..f9da1d1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -855,8 +855,8 @@ int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
return ret;
}
-int blk_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags)
+int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags)
{
return blk_prw(blk, offset, NULL, count, blk_write_entry,
flags | BDRV_REQ_ZERO_WRITE);
@@ -971,9 +971,9 @@ static void blk_aio_write_entry(void *opaque)
blk_aio_complete(acb);
}
-BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags,
- BlockCompletionFunc *cb, void *opaque)
+BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags,
+ BlockCompletionFunc *cb, void *opaque)
{
return blk_aio_prwv(blk, offset, count, NULL, blk_aio_write_entry,
flags | BDRV_REQ_ZERO_WRITE, cb, opaque);
@@ -1462,8 +1462,8 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
return qemu_aio_get(aiocb_info, blk_bs(blk), cb, opaque);
}
-int coroutine_fn blk_co_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags)
+int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags)
{
return blk_co_pwritev(blk, offset, count, NULL,
flags | BDRV_REQ_ZERO_WRITE);
diff --git a/block/parallels.c b/block/parallels.c
index 88cface..99fc0f7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -517,8 +517,8 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
if (ret < 0) {
goto exit;
}
- ret = blk_write_zeroes(file, BDRV_SECTOR_SIZE,
- (bat_sectors - 1) << BDRV_SECTOR_BITS, 0);
+ ret = blk_pwrite_zeroes(file, BDRV_SECTOR_SIZE,
+ (bat_sectors - 1) << BDRV_SECTOR_BITS, 0);
if (ret < 0) {
goto exit;
}
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c374ea8..8865da5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1780,7 +1780,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
nb_sectors * s->qdev.blocksize,
BLOCK_ACCT_WRITE);
- r->req.aiocb = blk_aio_write_zeroes(s->qdev.conf.blk,
+ r->req.aiocb = blk_aio_pwrite_zeroes(s->qdev.conf.blk,
r->req.cmd.lba * s->qdev.blocksize,
nb_sectors * s->qdev.blocksize,
flags, scsi_aio_complete, r);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9d6615c..9571726 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -113,11 +113,11 @@ void *blk_get_attached_dev(BlockBackend *blk);
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 count);
-int blk_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags);
-BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags,
- BlockCompletionFunc *cb, void *opaque);
+int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags);
+BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags,
+ BlockCompletionFunc *cb, void *opaque);
int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count);
int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
BdrvRequestFlags flags);
@@ -195,8 +195,8 @@ int blk_get_open_flags_from_root_state(BlockBackend *blk);
void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque);
-int coroutine_fn blk_co_write_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags);
+int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int count, BdrvRequestFlags flags);
int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
const uint8_t *buf, int nb_sectors);
int blk_truncate(BlockBackend *blk, int64_t offset);
diff --git a/qemu-img.c b/qemu-img.c
index 7ed8ef2..2065348 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1606,8 +1606,8 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
if (s->has_zero_init) {
break;
}
- ret = blk_write_zeroes(s->target, sector_num << BDRV_SECTOR_BITS,
- n << BDRV_SECTOR_BITS, 0);
+ ret = blk_pwrite_zeroes(s->target, sector_num << BDRV_SECTOR_BITS,
+ n << BDRV_SECTOR_BITS, 0);
if (ret < 0) {
return ret;
}
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e766791..09e879f 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -451,12 +451,12 @@ typedef struct {
bool done;
} CoWriteZeroes;
-static void coroutine_fn co_write_zeroes_entry(void *opaque)
+static void coroutine_fn co_pwrite_zeroes_entry(void *opaque)
{
CoWriteZeroes *data = opaque;
- data->ret = blk_co_write_zeroes(data->blk, data->offset, data->count,
- data->flags);
+ data->ret = blk_co_pwrite_zeroes(data->blk, data->offset, data->count,
+ data->flags);
data->done = true;
if (data->ret < 0) {
*data->total = data->ret;
@@ -466,8 +466,8 @@ static void coroutine_fn co_write_zeroes_entry(void *opaque)
*data->total = data->count;
}
-static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int64_t count,
- int flags, int64_t *total)
+static int do_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int64_t count, int flags, int64_t *total)
{
Coroutine *co;
CoWriteZeroes data = {
@@ -483,7 +483,7 @@ static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int64_t count,
return -ERANGE;
}
- co = qemu_coroutine_create(co_write_zeroes_entry);
+ co = qemu_coroutine_create(co_pwrite_zeroes_entry);
qemu_coroutine_enter(co, &data);
while (!data.done) {
aio_poll(blk_get_aio_context(blk), true);
@@ -901,7 +901,7 @@ static void write_help(void)
" -C, -- report statistics in a machine parsable format\n"
" -q, -- quiet mode, do not show I/O statistics\n"
" -u, -- with -z, allow unmapping\n"
-" -z, -- write zeroes using blk_co_write_zeroes\n"
+" -z, -- write zeroes using blk_co_pwrite_zeroes\n"
"\n");
}
@@ -1033,7 +1033,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
if (bflag) {
cnt = do_save_vmstate(blk, buf, offset, count, &total);
} else if (zflag) {
- cnt = do_co_write_zeroes(blk, offset, count, flags, &total);
+ cnt = do_co_pwrite_zeroes(blk, offset, count, flags, &total);
} else if (cflag) {
cnt = do_write_compressed(blk, buf, offset, count, &total);
} else {
@@ -1376,7 +1376,7 @@ static void aio_write_help(void)
" -i, -- treat request as invalid, for exercising stats\n"
" -q, -- quiet mode, do not show I/O statistics\n"
" -u, -- with -z, allow unmapping\n"
-" -z, -- write zeroes using blk_aio_write_zeroes\n"
+" -z, -- write zeroes using blk_aio_pwrite_zeroes\n"
"\n");
}
@@ -1475,8 +1475,8 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
}
ctx->qiov.size = count;
- blk_aio_write_zeroes(blk, ctx->offset, count, flags, aio_write_done,
- ctx);
+ blk_aio_pwrite_zeroes(blk, ctx->offset, count, flags, aio_write_done,
+ ctx);
} else {
nr_iov = argc - optind;
ctx->buf = create_iovec(blk, &ctx->qiov, &argv[optind], nr_iov,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 18/31] block: keep a list of block jobs
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (16 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 17/31] block: Rename blk_write_zeroes() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 19/31] block: Cancel jobs first in bdrv_close_all() Kevin Wolf
` (13 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Alberto Garcia <berto@igalia.com>
The current way to obtain the list of existing block jobs is to
iterate over all root nodes and check which ones own a job.
Since we want to be able to support block jobs in other nodes as well,
this patch keeps a list of jobs that is updated every time one is
created or destroyed.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockjob.c | 13 +++++++++++++
include/block/blockjob.h | 14 ++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/blockjob.c b/blockjob.c
index 5b840a7..0f1bc77 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -50,6 +50,16 @@ struct BlockJobTxn {
int refcnt;
};
+static QLIST_HEAD(, BlockJob) block_jobs = QLIST_HEAD_INITIALIZER(block_jobs);
+
+BlockJob *block_job_next(BlockJob *job)
+{
+ if (!job) {
+ return QLIST_FIRST(&block_jobs);
+ }
+ return QLIST_NEXT(job, job_list);
+}
+
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
@@ -76,6 +86,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
job->refcnt = 1;
bs->job = job;
+ QLIST_INSERT_HEAD(&block_jobs, job, job_list);
+
/* Only set speed when necessary to avoid NotSupported error */
if (speed != 0) {
Error *local_err = NULL;
@@ -103,6 +115,7 @@ void block_job_unref(BlockJob *job)
bdrv_unref(job->bs);
error_free(job->blocker);
g_free(job->id);
+ QLIST_REMOVE(job, job_list);
g_free(job);
}
}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 073a433..30bb2c6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -135,6 +135,9 @@ struct BlockJob {
*/
bool deferred_to_main_loop;
+ /** Element of the list of block jobs */
+ QLIST_ENTRY(BlockJob) job_list;
+
/** Status that is published by the query-block-jobs QMP API */
BlockDeviceIoStatus iostatus;
@@ -173,6 +176,17 @@ struct BlockJob {
};
/**
+ * block_job_next:
+ * @job: A block job, or %NULL.
+ *
+ * Get the next element from the list of block jobs after @job, or the
+ * first one if @job is %NULL.
+ *
+ * Returns the requested job, or %NULL if there are no more jobs left.
+ */
+BlockJob *block_job_next(BlockJob *job);
+
+/**
* block_job_create:
* @job_type: The class object for the newly-created job.
* @bs: The block
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 19/31] block: Cancel jobs first in bdrv_close_all()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (17 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 18/31] block: keep a list of block jobs Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 20/31] block: Default to enabled write cache in blk_new() Kevin Wolf
` (12 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
So far, bdrv_close_all() first removed all root BlockDriverStates of
BlockBackends and monitor owned BDSes, and then assumed that the
remaining BDSes must be related to jobs and cancelled these jobs.
This order doesn't work that well any more when block jobs use
BlockBackends internally because then they will lose their BDS before
being cancelled.
This patch changes bdrv_close_all() to first cancel all jobs and then
remove all root BDSes from the remaining BBs.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 23 ++---------------------
blockjob.c | 13 +++++++++++++
include/block/blockjob.h | 7 +++++++
3 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index 45cddd6..736432f 100644
--- a/block.c
+++ b/block.c
@@ -2209,8 +2209,7 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
- BlockDriverState *bs;
- AioContext *aio_context;
+ block_job_cancel_sync_all();
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
@@ -2219,25 +2218,7 @@ void bdrv_close_all(void)
blk_remove_all_bs();
blockdev_close_all_bdrv_states();
- /* Cancel all block jobs */
- while (!QTAILQ_EMPTY(&all_bdrv_states)) {
- QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
- aio_context = bdrv_get_aio_context(bs);
-
- aio_context_acquire(aio_context);
- if (bs->job) {
- block_job_cancel_sync(bs->job);
- aio_context_release(aio_context);
- break;
- }
- aio_context_release(aio_context);
- }
-
- /* All the remaining BlockDriverStates are referenced directly or
- * indirectly from block jobs, so there needs to be at least one BDS
- * directly used by a block job */
- assert(bs);
- }
+ assert(QTAILQ_EMPTY(&all_bdrv_states));
}
static void change_parent_backing_link(BlockDriverState *from,
diff --git a/blockjob.c b/blockjob.c
index 0f1bc77..e916b41 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -331,6 +331,19 @@ int block_job_cancel_sync(BlockJob *job)
return block_job_finish_sync(job, &block_job_cancel_err, NULL);
}
+void block_job_cancel_sync_all(void)
+{
+ BlockJob *job;
+ AioContext *aio_context;
+
+ while ((job = QLIST_FIRST(&block_jobs))) {
+ aio_context = bdrv_get_aio_context(job->bs);
+ aio_context_acquire(aio_context);
+ block_job_cancel_sync(job);
+ aio_context_release(aio_context);
+ }
+}
+
int block_job_complete_sync(BlockJob *job, Error **errp)
{
return block_job_finish_sync(job, &block_job_complete, errp);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 30bb2c6..4ac6831 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -371,6 +371,13 @@ bool block_job_is_paused(BlockJob *job);
int block_job_cancel_sync(BlockJob *job);
/**
+ * block_job_cancel_sync_all:
+ *
+ * Synchronously cancels all jobs using block_job_cancel_sync().
+ */
+void block_job_cancel_sync_all(void);
+
+/**
* block_job_complete_sync:
* @job: The job to be completed.
* @errp: Error object which may be set by block_job_complete(); this is not
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 20/31] block: Default to enabled write cache in blk_new()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (18 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 19/31] block: Cancel jobs first in bdrv_close_all() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 21/31] block: Convert block job core to BlockBackend Kevin Wolf
` (11 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
The existing users of the function are:
1. blk_new_open(), which already enabled the write cache
2. Some test cases that don't care about the setting
3. blockdev_init() for empty drives, where the cache mode is overridden
with the value from the options when a medium is inserted
Therefore, this patch doesn't change the current behaviour. It will be
convenient, however, for additional users of blk_new() (like block
jobs) if the most sensible WCE setting is the default.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/block-backend.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index f9da1d1..c8b13f1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -125,6 +125,8 @@ BlockBackend *blk_new(void)
blk = g_new0(BlockBackend, 1);
blk->refcnt = 1;
+ blk_set_enable_write_cache(blk, true);
+
qemu_co_queue_init(&blk->public.throttled_reqs[0]);
qemu_co_queue_init(&blk->public.throttled_reqs[1]);
@@ -160,7 +162,6 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
return NULL;
}
- blk_set_enable_write_cache(blk, true);
blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
return blk;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 21/31] block: Convert block job core to BlockBackend
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (19 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 20/31] block: Default to enabled write cache in blk_new() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 22/31] block: Make blk_co_preadv/pwritev() public Kevin Wolf
` (10 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This adds a new BlockBackend field to the BlockJob struct, which
coexists with the BlockDriverState while converting the individual jobs.
When creating a block job, a new BlockBackend is created on top of the
given BlockDriverState, and it is destroyed when the BlockJob ends. The
reference to the BDS is now held by the BlockBackend instead of calling
bdrv_ref/unref manually.
We have to be careful when we use bdrv_replace_in_backing_chain() in
block jobs because this changes the BDS that job->blk points to. At the
moment block jobs are too tightly coupled with their BDS, so that moving
a job to another BDS isn't easily possible; therefore, we need to just
manually undo this change afterwards.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 3 +++
blockjob.c | 37 ++++++++++++++++++++-----------------
include/block/blockjob.h | 3 ++-
3 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index b9986d8..efca8fc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -478,6 +478,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
}
bdrv_replace_in_backing_chain(to_replace, s->target);
+ /* We just changed the BDS the job BB refers to */
+ blk_remove_bs(job->blk);
+ blk_insert_bs(job->blk, src);
}
out:
diff --git a/blockjob.c b/blockjob.c
index e916b41..2097e1d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -64,13 +64,17 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
{
+ BlockBackend *blk;
BlockJob *job;
if (bs->job) {
error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
return NULL;
}
- bdrv_ref(bs);
+
+ blk = blk_new();
+ blk_insert_bs(blk, bs);
+
job = g_malloc0(driver->instance_size);
error_setg(&job->blocker, "block device is in use by block job: %s",
BlockJobType_lookup[driver->job_type]);
@@ -80,6 +84,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
job->driver = driver;
job->id = g_strdup(bdrv_get_device_name(bs));
job->bs = bs;
+ job->blk = blk;
job->cb = cb;
job->opaque = opaque;
job->busy = true;
@@ -110,9 +115,10 @@ void block_job_ref(BlockJob *job)
void block_job_unref(BlockJob *job)
{
if (--job->refcnt == 0) {
- job->bs->job = NULL;
- bdrv_op_unblock_all(job->bs, job->blocker);
- bdrv_unref(job->bs);
+ BlockDriverState *bs = blk_bs(job->blk);
+ bs->job = NULL;
+ bdrv_op_unblock_all(bs, job->blocker);
+ blk_unref(job->blk);
error_free(job->blocker);
g_free(job->id);
QLIST_REMOVE(job, job_list);
@@ -153,7 +159,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
txn->aborting = true;
/* We are the first failed job. Cancel other jobs. */
QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
- ctx = bdrv_get_aio_context(other_job->bs);
+ ctx = blk_get_aio_context(other_job->blk);
aio_context_acquire(ctx);
}
QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
@@ -170,7 +176,7 @@ static void block_job_completed_txn_abort(BlockJob *job)
assert(other_job->completed);
}
QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
- ctx = bdrv_get_aio_context(other_job->bs);
+ ctx = blk_get_aio_context(other_job->blk);
block_job_completed_single(other_job);
aio_context_release(ctx);
}
@@ -192,7 +198,7 @@ static void block_job_completed_txn_success(BlockJob *job)
}
/* We are the last completed job, commit the transaction. */
QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
- ctx = bdrv_get_aio_context(other_job->bs);
+ ctx = blk_get_aio_context(other_job->blk);
aio_context_acquire(ctx);
assert(other_job->ret == 0);
block_job_completed_single(other_job);
@@ -202,9 +208,7 @@ static void block_job_completed_txn_success(BlockJob *job)
void block_job_completed(BlockJob *job, int ret)
{
- BlockDriverState *bs = job->bs;
-
- assert(bs->job == job);
+ assert(blk_bs(job->blk)->job == job);
assert(!job->completed);
job->completed = true;
job->ret = ret;
@@ -295,11 +299,10 @@ static int block_job_finish_sync(BlockJob *job,
void (*finish)(BlockJob *, Error **errp),
Error **errp)
{
- BlockDriverState *bs = job->bs;
Error *local_err = NULL;
int ret;
- assert(bs->job == job);
+ assert(blk_bs(job->blk)->job == job);
block_job_ref(job);
finish(job, &local_err);
@@ -310,7 +313,7 @@ static int block_job_finish_sync(BlockJob *job,
}
while (!job->completed) {
aio_poll(job->deferred_to_main_loop ? qemu_get_aio_context() :
- bdrv_get_aio_context(bs),
+ blk_get_aio_context(job->blk),
true);
}
ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
@@ -337,7 +340,7 @@ void block_job_cancel_sync_all(void)
AioContext *aio_context;
while ((job = QLIST_FIRST(&block_jobs))) {
- aio_context = bdrv_get_aio_context(job->bs);
+ aio_context = blk_get_aio_context(job->blk);
aio_context_acquire(aio_context);
block_job_cancel_sync(job);
aio_context_release(aio_context);
@@ -362,7 +365,7 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
if (block_job_is_paused(job)) {
qemu_coroutine_yield();
} else {
- co_aio_sleep_ns(bdrv_get_aio_context(job->bs), type, ns);
+ co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
}
job->busy = true;
}
@@ -491,7 +494,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque)
aio_context_acquire(data->aio_context);
/* Fetch BDS AioContext again, in case it has changed */
- aio_context = bdrv_get_aio_context(data->job->bs);
+ aio_context = blk_get_aio_context(data->job->blk);
aio_context_acquire(aio_context);
data->job->deferred_to_main_loop = false;
@@ -511,7 +514,7 @@ void block_job_defer_to_main_loop(BlockJob *job,
BlockJobDeferToMainLoopData *data = g_malloc(sizeof(*data));
data->job = job;
data->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, data);
- data->aio_context = bdrv_get_aio_context(job->bs);
+ data->aio_context = blk_get_aio_context(job->blk);
data->fn = fn;
data->opaque = opaque;
job->deferred_to_main_loop = true;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ac6831..32012af 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -82,7 +82,8 @@ struct BlockJob {
const BlockJobDriver *driver;
/** The block device on which the job is operating. */
- BlockDriverState *bs;
+ BlockDriverState *bs; /* TODO Remove */
+ BlockBackend *blk;
/**
* The ID of the block job. Currently the BlockBackend name of the BDS
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 22/31] block: Make blk_co_preadv/pwritev() public
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (20 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 21/31] block: Convert block job core to BlockBackend Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 23/31] stream: Use BlockBackend for I/O Kevin Wolf
` (9 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Also add trace points now that the function can be directly called.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/block-backend.c | 21 ++++++++++++++-------
include/sysemu/block-backend.h | 6 ++++++
trace-events | 4 ++++
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index c8b13f1..34500e6 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -19,6 +19,7 @@
#include "sysemu/sysemu.h"
#include "qapi-event.h"
#include "qemu/id.h"
+#include "trace.h"
/* Number of coroutines to reserve per attached device model */
#define COROUTINE_POOL_RESERVATION 64
@@ -741,11 +742,15 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
nb_sectors * BDRV_SECTOR_SIZE);
}
-static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
- unsigned int bytes, QEMUIOVector *qiov,
- BdrvRequestFlags flags)
+int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
+ unsigned int bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
- int ret = blk_check_byte_request(blk, offset, bytes);
+ int ret;
+
+ trace_blk_co_preadv(blk, blk_bs(blk), offset, bytes, flags);
+
+ ret = blk_check_byte_request(blk, offset, bytes);
if (ret < 0) {
return ret;
}
@@ -758,12 +763,14 @@ static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
return bdrv_co_preadv(blk_bs(blk), offset, bytes, qiov, flags);
}
-static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
- unsigned int bytes, QEMUIOVector *qiov,
- BdrvRequestFlags flags)
+int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
+ unsigned int bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
int ret;
+ trace_blk_co_pwritev(blk, blk_bs(blk), offset, bytes, flags);
+
ret = blk_check_byte_request(blk, offset, bytes);
if (ret < 0) {
return ret;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9571726..c04af8e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -113,6 +113,12 @@ void *blk_get_attached_dev(BlockBackend *blk);
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 count);
+int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
+ unsigned int bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags);
+int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
+ unsigned int bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags);
int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int count, BdrvRequestFlags flags);
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
diff --git a/trace-events b/trace-events
index e01b436..b85fb49 100644
--- a/trace-events
+++ b/trace-events
@@ -61,6 +61,10 @@ virtio_console_chr_event(unsigned int port, int event) "port %u, event %d"
bdrv_open_common(void *bs, const char *filename, int flags, const char *format_name) "bs %p filename \"%s\" flags %#x format_name \"%s\""
bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
+# block/block-backend.c
+blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags %x"
+blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags %x"
+
# block/io.c
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"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 23/31] stream: Use BlockBackend for I/O
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (21 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 22/31] block: Make blk_co_preadv/pwritev() public Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 24/31] mirror: Allow target that already has a BlockBackend Kevin Wolf
` (8 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This changes the streaming block job to use the job's BlockBackend for
performing the COR reads. job->bs isn't used by the streaming code any
more afterwards.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/io.c | 9 ---------
block/stream.c | 15 +++++++++------
include/block/block.h | 2 --
trace-events | 1 -
4 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/block/io.c b/block/io.c
index 9bc1d45..bc2eda2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1117,15 +1117,6 @@ int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
BDRV_REQ_NO_SERIALISING);
}
-int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
- trace_bdrv_co_copy_on_readv(bs, sector_num, nb_sectors);
-
- return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov,
- BDRV_REQ_COPY_ON_READ);
-}
-
#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 40aa322..c0efbda 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -39,7 +39,7 @@ typedef struct StreamBlockJob {
char *backing_file_str;
} StreamBlockJob;
-static int coroutine_fn stream_populate(BlockDriverState *bs,
+static int coroutine_fn stream_populate(BlockBackend *blk,
int64_t sector_num, int nb_sectors,
void *buf)
{
@@ -52,7 +52,8 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
qemu_iovec_init_external(&qiov, &iov, 1);
/* Copy-on-read the unallocated clusters */
- return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov);
+ return blk_co_preadv(blk, sector_num * BDRV_SECTOR_SIZE, qiov.size, &qiov,
+ BDRV_REQ_COPY_ON_READ);
}
typedef struct {
@@ -64,6 +65,7 @@ static void stream_complete(BlockJob *job, void *opaque)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common);
StreamCompleteData *data = opaque;
+ BlockDriverState *bs = blk_bs(job->blk);
BlockDriverState *base = s->base;
if (!block_job_is_cancelled(&s->common) && data->reached_end &&
@@ -75,8 +77,8 @@ static void stream_complete(BlockJob *job, void *opaque)
base_fmt = base->drv->format_name;
}
}
- data->ret = bdrv_change_backing_file(job->bs, base_id, base_fmt);
- bdrv_set_backing_hd(job->bs, base);
+ data->ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+ bdrv_set_backing_hd(bs, base);
}
g_free(s->backing_file_str);
@@ -88,7 +90,8 @@ static void coroutine_fn stream_run(void *opaque)
{
StreamBlockJob *s = opaque;
StreamCompleteData *data;
- BlockDriverState *bs = s->common.bs;
+ BlockBackend *blk = s->common.blk;
+ BlockDriverState *bs = blk_bs(blk);
BlockDriverState *base = s->base;
int64_t sector_num = 0;
int64_t end = -1;
@@ -159,7 +162,7 @@ wait:
goto wait;
}
}
- ret = stream_populate(bs, sector_num, n, buf);
+ ret = stream_populate(blk, sector_num, n, buf);
}
if (ret < 0) {
BlockErrorAction action =
diff --git a/include/block/block.h b/include/block/block.h
index e899f7d..0a4b973 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -242,8 +242,6 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov);
-int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
diff --git a/trace-events b/trace-events
index b85fb49..6966dd6 100644
--- a/trace-events
+++ b/trace-events
@@ -72,7 +72,6 @@ bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %
bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 24/31] mirror: Allow target that already has a BlockBackend
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (22 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 23/31] stream: Use BlockBackend for I/O Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 25/31] mirror: Use BlockBackend for I/O Kevin Wolf
` (7 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
We had to forbid mirroring to a target BDS that already had a BB
attached because the node swapping at job completion would add a second
BB and we didn't support multiple BBs on a single BDS at the time. Now
we do, so we can lift the restriction.
As we allow additional BlockBackends for the target, we must expect
other users to be sending requests. There may no requests be in flight
during the graph modification, so we have to drain those users now.
The core part of this patch is a revert of commit 40365552.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 33 ++++++---------------------------
blockdev.c | 4 ----
tests/qemu-iotests/041 | 27 ---------------------------
tests/qemu-iotests/041.out | 4 ++--
4 files changed, 8 insertions(+), 60 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index efca8fc..23aa10e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -20,7 +20,6 @@
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
#include "qemu/bitmap.h"
-#include "qemu/error-report.h"
#define SLICE_TIME 100000000ULL /* ns */
#define MAX_IN_FLIGHT 16
@@ -466,24 +465,20 @@ static void mirror_exit(BlockJob *job, void *opaque)
to_replace = s->to_replace;
}
- /* This was checked in mirror_start_job(), but meanwhile one of the
- * nodes could have been newly attached to a BlockBackend. */
- 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;
- }
-
if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
}
+
+ /* The mirror job has no requests in flight any more, but we need to
+ * drain potential other users of the BDS before changing the graph. */
+ bdrv_drained_begin(s->target);
bdrv_replace_in_backing_chain(to_replace, s->target);
+ bdrv_drained_end(s->target);
+
/* We just changed the BDS the job BB refers to */
blk_remove_bs(job->blk);
blk_insert_bs(job->blk, src);
}
-
-out:
if (s->to_replace) {
bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
error_free(s->replace_blocker);
@@ -807,7 +802,6 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
bool is_none_mode, BlockDriverState *base)
{
MirrorBlockJob *s;
- BlockDriverState *replaced_bs;
if (granularity == 0) {
granularity = bdrv_get_default_bitmap_granularity(target);
@@ -824,21 +818,6 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
buf_size = DEFAULT_MIRROR_BUF_SIZE;
}
- /* We can't support this case as long as the block layer can't handle
- * multiple BlockBackends per BlockDriverState. */
- if (replaces) {
- replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
- if (replaced_bs == NULL) {
- return;
- }
- } else {
- replaced_bs = bs;
- }
- if (bdrv_has_blk(replaced_bs) && bdrv_has_blk(target)) {
- error_setg(errp, "Can't create node with two BlockBackends");
- return;
- }
-
s = block_job_create(driver, bs, speed, cb, opaque, errp);
if (!s) {
return;
diff --git a/blockdev.c b/blockdev.c
index af0b022..3c06746 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3459,10 +3459,6 @@ static void blockdev_mirror_common(BlockDriverState *bs,
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
return;
}
- if (bdrv_has_blk(target)) {
- error_setg(errp, "Cannot mirror to an attached block device");
- return;
- }
if (!bs->backing && sync == MIRROR_SYNC_MODE_TOP) {
sync = MIRROR_SYNC_MODE_FULL;
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index b1c542f..ed1d9d4 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -207,33 +207,6 @@ class TestSingleBlockdev(TestSingleDrive):
test_image_not_found = None
test_small_buffer2 = None
-class TestBlockdevAttached(iotests.QMPTestCase):
- image_len = 1 * 1024 * 1024 # MB
-
- def setUp(self):
- iotests.create_image(backing_img, self.image_len)
- qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
- qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
- self.vm = iotests.VM().add_drive(test_img)
- self.vm.launch()
-
- def tearDown(self):
- self.vm.shutdown()
- os.remove(test_img)
- os.remove(target_img)
-
- def test_blockdev_attached(self):
- self.assert_no_active_block_jobs()
- args = {'options':
- {'driver': iotests.imgfmt,
- 'id': 'drive1',
- 'file': { 'filename': target_img, 'driver': 'file' } } }
- result = self.vm.qmp("blockdev-add", **args)
- self.assert_qmp(result, 'return', {})
- result = self.vm.qmp('blockdev-mirror', device='drive0', sync='full',
- target='drive1')
- self.assert_qmp(result, 'error/class', 'GenericError')
-
class TestSingleDriveZeroLength(TestSingleDrive):
image_len = 0
test_small_buffer2 = None
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index b67d050..b0cadc8 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-............................................................................
+...........................................................................
----------------------------------------------------------------------
-Ran 76 tests
+Ran 75 tests
OK
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 25/31] mirror: Use BlockBackend for I/O
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (23 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 24/31] mirror: Allow target that already has a BlockBackend Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 26/31] backup: Don't leak BackupBlockJob in error path Kevin Wolf
` (6 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This changes the mirror block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the mirroring code any more
afterwards.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 70 ++++++++++++++++++++++++++++++++--------------------------
blockdev.c | 4 +---
2 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 23aa10e..80fd3c7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -35,7 +35,7 @@ typedef struct MirrorBuffer {
typedef struct MirrorBlockJob {
BlockJob common;
RateLimit limit;
- BlockDriverState *target;
+ BlockBackend *target;
BlockDriverState *base;
/* The name of the graph node to replace */
char *replaces;
@@ -156,7 +156,8 @@ static void mirror_read_complete(void *opaque, int ret)
mirror_iteration_done(op, ret);
return;
}
- bdrv_aio_writev(s->target, op->sector_num, &op->qiov, op->nb_sectors,
+ blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, &op->qiov,
+ op->nb_sectors * BDRV_SECTOR_SIZE,
mirror_write_complete, op);
}
@@ -185,7 +186,7 @@ static int mirror_cow_align(MirrorBlockJob *s,
need_cow |= !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
s->cow_bitmap);
if (need_cow) {
- bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
+ bdrv_round_to_clusters(blk_bs(s->target), *sector_num, *nb_sectors,
&align_sector_num, &align_nb_sectors);
}
@@ -223,7 +224,7 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
int nb_sectors)
{
- BlockDriverState *source = s->common.bs;
+ BlockBackend *source = s->common.blk;
int sectors_per_chunk, nb_chunks;
int ret = nb_sectors;
MirrorOp *op;
@@ -273,7 +274,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
s->sectors_in_flight += nb_sectors;
trace_mirror_one_iteration(s, sector_num, nb_sectors);
- bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
+ blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov,
+ nb_sectors * BDRV_SECTOR_SIZE,
mirror_read_complete, op);
return ret;
}
@@ -295,10 +297,11 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
s->in_flight++;
s->sectors_in_flight += nb_sectors;
if (is_discard) {
- bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
- mirror_write_complete, op);
+ blk_aio_discard(s->target, sector_num, op->nb_sectors,
+ mirror_write_complete, op);
} else {
- bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
+ blk_aio_pwrite_zeroes(s->target, sector_num * BDRV_SECTOR_SIZE,
+ op->nb_sectors * BDRV_SECTOR_SIZE,
s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
mirror_write_complete, op);
}
@@ -306,7 +309,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
{
- BlockDriverState *source = s->common.bs;
+ BlockDriverState *source = blk_bs(s->common.blk);
int64_t sector_num, first_chunk;
uint64_t delay_ns = 0;
/* At least the first dirty chunk is mirrored in one iteration. */
@@ -383,7 +386,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
} else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
int64_t target_sector_num;
int target_nb_sectors;
- bdrv_round_to_clusters(s->target, sector_num, io_sectors,
+ bdrv_round_to_clusters(blk_bs(s->target), sector_num, io_sectors,
&target_sector_num, &target_nb_sectors);
if (target_sector_num == sector_num &&
target_nb_sectors == io_sectors) {
@@ -448,7 +451,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
MirrorExitData *data = opaque;
AioContext *replace_aio_context = NULL;
- BlockDriverState *src = s->common.bs;
+ BlockDriverState *src = blk_bs(s->common.blk);
+ BlockDriverState *target_bs = blk_bs(s->target);
/* Make sure that the source BDS doesn't go away before we called
* block_job_completed(). */
@@ -460,20 +464,20 @@ static void mirror_exit(BlockJob *job, void *opaque)
}
if (s->should_complete && data->ret == 0) {
- BlockDriverState *to_replace = s->common.bs;
+ BlockDriverState *to_replace = src;
if (s->to_replace) {
to_replace = s->to_replace;
}
- if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
- bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
+ if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
+ bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
}
/* The mirror job has no requests in flight any more, but we need to
* drain potential other users of the BDS before changing the graph. */
- bdrv_drained_begin(s->target);
- bdrv_replace_in_backing_chain(to_replace, s->target);
- bdrv_drained_end(s->target);
+ bdrv_drained_begin(target_bs);
+ bdrv_replace_in_backing_chain(to_replace, target_bs);
+ bdrv_drained_end(target_bs);
/* We just changed the BDS the job BB refers to */
blk_remove_bs(job->blk);
@@ -488,8 +492,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
aio_context_release(replace_aio_context);
}
g_free(s->replaces);
- bdrv_op_unblock_all(s->target, s->common.blocker);
- bdrv_unref(s->target);
+ bdrv_op_unblock_all(target_bs, s->common.blocker);
+ blk_unref(s->target);
block_job_completed(&s->common, data->ret);
g_free(data);
bdrv_drained_end(src);
@@ -503,7 +507,8 @@ static void coroutine_fn mirror_run(void *opaque)
{
MirrorBlockJob *s = opaque;
MirrorExitData *data;
- BlockDriverState *bs = s->common.bs;
+ BlockDriverState *bs = blk_bs(s->common.blk);
+ BlockDriverState *target_bs = blk_bs(s->target);
int64_t sector_num, end, length;
uint64_t last_pause_ns;
BlockDriverInfo bdi;
@@ -539,18 +544,18 @@ static void coroutine_fn mirror_run(void *opaque)
* the destination do COW. Instead, we copy sectors around the
* dirty data if needed. We need a bitmap to do that.
*/
- bdrv_get_backing_filename(s->target, backing_filename,
+ bdrv_get_backing_filename(target_bs, backing_filename,
sizeof(backing_filename));
- if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
+ if (!bdrv_get_info(target_bs, &bdi) && bdi.cluster_size) {
target_cluster_size = bdi.cluster_size;
}
- if (backing_filename[0] && !s->target->backing
+ if (backing_filename[0] && !target_bs->backing
&& s->granularity < target_cluster_size) {
s->buf_size = MAX(s->buf_size, target_cluster_size);
s->cow_bitmap = bitmap_new(length);
}
s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
- s->max_iov = MIN(s->common.bs->bl.max_iov, s->target->bl.max_iov);
+ s->max_iov = MIN(bs->bl.max_iov, target_bs->bl.max_iov);
end = s->bdev_length / BDRV_SECTOR_SIZE;
s->buf = qemu_try_blockalign(bs, s->buf_size);
@@ -565,7 +570,7 @@ static void coroutine_fn mirror_run(void *opaque)
if (!s->is_none_mode) {
/* First part, loop on the sectors and initialize the dirty bitmap. */
BlockDriverState *base = s->base;
- bool mark_all_dirty = s->base == NULL && !bdrv_has_zero_init(s->target);
+ bool mark_all_dirty = s->base == NULL && !bdrv_has_zero_init(target_bs);
for (sector_num = 0; sector_num < end; ) {
/* Just to make sure we are not exceeding int limit. */
@@ -635,7 +640,7 @@ static void coroutine_fn mirror_run(void *opaque)
should_complete = false;
if (s->in_flight == 0 && cnt == 0) {
trace_mirror_before_flush(s);
- ret = bdrv_flush(s->target);
+ ret = blk_flush(s->target);
if (ret < 0) {
if (mirror_error_action(s, false, -ret) ==
BLOCK_ERROR_ACTION_REPORT) {
@@ -713,7 +718,7 @@ immediate_exit:
data->ret = ret;
/* Before we switch to target in mirror_exit, make sure data doesn't
* change. */
- bdrv_drained_begin(s->common.bs);
+ bdrv_drained_begin(bs);
if (qemu_get_aio_context() == bdrv_get_aio_context(bs)) {
/* FIXME: virtio host notifiers run on iohandler_ctx, therefore the
* above bdrv_drained_end isn't enough to quiesce it. This is ugly, we
@@ -740,7 +745,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
Error *local_err = NULL;
int ret;
- ret = bdrv_open_backing_file(s->target, NULL, "backing", &local_err);
+ ret = bdrv_open_backing_file(blk_bs(s->target), NULL, "backing",
+ &local_err);
if (ret < 0) {
error_propagate(errp, local_err);
return;
@@ -823,10 +829,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ s->target = blk_new();
+ blk_insert_bs(s->target, target);
+
s->replaces = g_strdup(replaces);
s->on_source_error = on_source_error;
s->on_target_error = on_target_error;
- s->target = target;
s->is_none_mode = is_none_mode;
s->base = base;
s->granularity = granularity;
@@ -836,11 +844,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
if (!s->dirty_bitmap) {
g_free(s->replaces);
+ blk_unref(s->target);
block_job_unref(&s->common);
return;
}
- bdrv_op_block_all(s->target, s->common.blocker);
+ bdrv_op_block_all(target, s->common.blocker);
s->common.co = qemu_coroutine_create(mirror_run);
trace_mirror_start(bs, s, s->common.co, opaque);
@@ -913,7 +922,6 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
}
}
- bdrv_ref(base);
mirror_start_job(bs, base, NULL, speed, 0, 0,
on_error, on_error, false, cb, opaque, &local_err,
&commit_active_job_driver, false, base);
diff --git a/blockdev.c b/blockdev.c
index 3c06746..66477ff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3621,9 +3621,9 @@ void qmp_drive_mirror(const char *device, const char *target,
has_on_target_error, on_target_error,
has_unmap, unmap,
&local_err);
+ bdrv_unref(target_bs);
if (local_err) {
error_propagate(errp, local_err);
- bdrv_unref(target_bs);
}
out:
aio_context_release(aio_context);
@@ -3667,7 +3667,6 @@ void qmp_blockdev_mirror(const char *device, const char *target,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
blockdev_mirror_common(bs, target_bs,
@@ -3681,7 +3680,6 @@ void qmp_blockdev_mirror(const char *device, const char *target,
&local_err);
if (local_err) {
error_propagate(errp, local_err);
- bdrv_unref(target_bs);
}
aio_context_release(aio_context);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 26/31] backup: Don't leak BackupBlockJob in error path
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (24 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 25/31] mirror: Use BlockBackend for I/O Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 27/31] backup: Pack Notifier within BackupBlockJob Kevin Wolf
` (5 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/backup.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index fec45e8..a990cf1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -485,6 +485,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
{
int64_t len;
BlockDriverInfo bdi;
+ BackupBlockJob *job = NULL;
int ret;
assert(bs);
@@ -542,8 +543,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
goto error;
}
- BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
- cb, opaque, errp);
+ job = block_job_create(&backup_job_driver, bs, speed, cb, opaque, errp);
if (!job) {
goto error;
}
@@ -584,4 +584,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
if (sync_bitmap) {
bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
}
+ if (job) {
+ block_job_unref(&job->common);
+ }
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 27/31] backup: Pack Notifier within BackupBlockJob
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (25 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 26/31] backup: Don't leak BackupBlockJob in error path Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 28/31] backup: Remove bs parameter from backup_do_cow() Kevin Wolf
` (4 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: John Snow <jsnow@redhat.com>
Instead of relying on peeking at bs->job, we want to explicitly get
a reference to the job that was involved in this notifier callback.
Pack the Notifier inside of the BackupBlockJob so we can use
container_of to get a reference back to the BackupBlockJob object.
This cuts out one more case where we rely unnecessarily on bs->job.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index a990cf1..a57288f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -47,6 +47,7 @@ typedef struct BackupBlockJob {
uint64_t sectors_read;
unsigned long *done_bitmap;
int64_t cluster_size;
+ NotifierWithReturn before_write;
QLIST_HEAD(, CowRequest) inflight_reqs;
} BackupBlockJob;
@@ -94,11 +95,11 @@ static void cow_request_end(CowRequest *req)
}
static int coroutine_fn backup_do_cow(BlockDriverState *bs,
+ BackupBlockJob *job,
int64_t sector_num, int nb_sectors,
bool *error_is_read,
bool is_write_notifier)
{
- BackupBlockJob *job = (BackupBlockJob *)bs->job;
CowRequest cow_request;
struct iovec iov;
QEMUIOVector bounce_qiov;
@@ -197,6 +198,7 @@ static int coroutine_fn backup_before_write_notify(
NotifierWithReturn *notifier,
void *opaque)
{
+ BackupBlockJob *job = container_of(notifier, BackupBlockJob, before_write);
BdrvTrackedRequest *req = opaque;
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
@@ -204,7 +206,8 @@ static int coroutine_fn backup_before_write_notify(
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- return backup_do_cow(req->bs, sector_num, nb_sectors, NULL, true);
+ return backup_do_cow(req->bs, job, sector_num,
+ nb_sectors, NULL, true);
}
static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -343,7 +346,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
if (yield_and_check(job)) {
return ret;
}
- ret = backup_do_cow(bs, cluster * sectors_per_cluster,
+ ret = backup_do_cow(bs, job, cluster * sectors_per_cluster,
sectors_per_cluster, &error_is_read,
false);
if ((ret < 0) &&
@@ -378,9 +381,6 @@ static void coroutine_fn backup_run(void *opaque)
BackupCompleteData *data;
BlockDriverState *bs = job->common.bs;
BlockDriverState *target = job->target;
- NotifierWithReturn before_write = {
- .notify = backup_before_write_notify,
- };
int64_t start, end;
int64_t sectors_per_cluster = cluster_size_sectors(job);
int ret = 0;
@@ -393,7 +393,8 @@ static void coroutine_fn backup_run(void *opaque)
job->done_bitmap = bitmap_new(end);
- bdrv_add_before_write_notifier(bs, &before_write);
+ job->before_write.notify = backup_before_write_notify;
+ bdrv_add_before_write_notifier(bs, &job->before_write);
if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
while (!block_job_is_cancelled(&job->common)) {
@@ -445,7 +446,7 @@ static void coroutine_fn backup_run(void *opaque)
}
}
/* FULL sync mode we copy the whole drive. */
- ret = backup_do_cow(bs, start * sectors_per_cluster,
+ ret = backup_do_cow(bs, job, start * sectors_per_cluster,
sectors_per_cluster, &error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
@@ -461,7 +462,7 @@ static void coroutine_fn backup_run(void *opaque)
}
}
- notifier_with_return_remove(&before_write);
+ notifier_with_return_remove(&job->before_write);
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 28/31] backup: Remove bs parameter from backup_do_cow()
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (26 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 27/31] backup: Pack Notifier within BackupBlockJob Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 29/31] backup: Use BlockBackend for I/O Kevin Wolf
` (3 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Now that we pass the job to the function, bs is implied by that.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/backup.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index a57288f..670ba50 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -94,12 +94,12 @@ static void cow_request_end(CowRequest *req)
qemu_co_queue_restart_all(&req->wait_queue);
}
-static int coroutine_fn backup_do_cow(BlockDriverState *bs,
- BackupBlockJob *job,
+static int coroutine_fn backup_do_cow(BackupBlockJob *job,
int64_t sector_num, int nb_sectors,
bool *error_is_read,
bool is_write_notifier)
{
+ BlockDriverState *bs = job->common.bs;
CowRequest cow_request;
struct iovec iov;
QEMUIOVector bounce_qiov;
@@ -203,11 +203,11 @@ static int coroutine_fn backup_before_write_notify(
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
+ assert(req->bs == job->common.bs);
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- return backup_do_cow(req->bs, job, sector_num,
- nb_sectors, NULL, true);
+ return backup_do_cow(job, sector_num, nb_sectors, NULL, true);
}
static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -324,7 +324,6 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
int64_t end;
int64_t last_cluster = -1;
int64_t sectors_per_cluster = cluster_size_sectors(job);
- BlockDriverState *bs = job->common.bs;
HBitmapIter hbi;
granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
@@ -346,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
if (yield_and_check(job)) {
return ret;
}
- ret = backup_do_cow(bs, job, cluster * sectors_per_cluster,
+ ret = backup_do_cow(job, cluster * sectors_per_cluster,
sectors_per_cluster, &error_is_read,
false);
if ((ret < 0) &&
@@ -446,7 +445,7 @@ static void coroutine_fn backup_run(void *opaque)
}
}
/* FULL sync mode we copy the whole drive. */
- ret = backup_do_cow(bs, job, start * sectors_per_cluster,
+ ret = backup_do_cow(job, start * sectors_per_cluster,
sectors_per_cluster, &error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 29/31] backup: Use BlockBackend for I/O
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (27 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 28/31] backup: Remove bs parameter from backup_do_cow() Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 30/31] commit: " Kevin Wolf
` (2 subsequent siblings)
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This changes the backup block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the backup code any more
afterwards.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 46 +++++++++++++++++++++-------------------------
block/io.c | 9 ---------
blockdev.c | 4 +---
include/block/block.h | 2 --
trace-events | 1 -
5 files changed, 22 insertions(+), 40 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 670ba50..feeb9f8 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -36,7 +36,7 @@ typedef struct CowRequest {
typedef struct BackupBlockJob {
BlockJob common;
- BlockDriverState *target;
+ BlockBackend *target;
/* bitmap for sync=incremental */
BdrvDirtyBitmap *sync_bitmap;
MirrorSyncMode sync_mode;
@@ -99,7 +99,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
bool *error_is_read,
bool is_write_notifier)
{
- BlockDriverState *bs = job->common.bs;
+ BlockBackend *blk = job->common.blk;
CowRequest cow_request;
struct iovec iov;
QEMUIOVector bounce_qiov;
@@ -132,20 +132,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
start * sectors_per_cluster);
if (!bounce_buffer) {
- bounce_buffer = qemu_blockalign(bs, job->cluster_size);
+ bounce_buffer = blk_blockalign(blk, job->cluster_size);
}
iov.iov_base = bounce_buffer;
iov.iov_len = n * BDRV_SECTOR_SIZE;
qemu_iovec_init_external(&bounce_qiov, &iov, 1);
- if (is_write_notifier) {
- ret = bdrv_co_readv_no_serialising(bs,
- start * sectors_per_cluster,
- n, &bounce_qiov);
- } else {
- ret = bdrv_co_readv(bs, start * sectors_per_cluster, n,
- &bounce_qiov);
- }
+ ret = blk_co_preadv(blk, start * job->cluster_size,
+ bounce_qiov.size, &bounce_qiov,
+ is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
if (ret < 0) {
trace_backup_do_cow_read_fail(job, start, ret);
if (error_is_read) {
@@ -155,13 +150,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
}
if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
- ret = bdrv_co_write_zeroes(job->target,
- start * sectors_per_cluster,
- n, BDRV_REQ_MAY_UNMAP);
+ ret = blk_co_pwrite_zeroes(job->target, start * job->cluster_size,
+ bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
} else {
- ret = bdrv_co_writev(job->target,
- start * sectors_per_cluster, n,
- &bounce_qiov);
+ ret = blk_co_pwritev(job->target, start * job->cluster_size,
+ bounce_qiov.size, &bounce_qiov, 0);
}
if (ret < 0) {
trace_backup_do_cow_write_fail(job, start, ret);
@@ -203,7 +196,7 @@ static int coroutine_fn backup_before_write_notify(
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
- assert(req->bs == job->common.bs);
+ assert(req->bs == blk_bs(job->common.blk));
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
@@ -224,7 +217,7 @@ static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
{
BdrvDirtyBitmap *bm;
- BlockDriverState *bs = job->common.bs;
+ BlockDriverState *bs = blk_bs(job->common.blk);
if (ret < 0 || block_job_is_cancelled(&job->common)) {
/* Merge the successor back into the parent, delete nothing. */
@@ -282,7 +275,7 @@ static void backup_complete(BlockJob *job, void *opaque)
BackupBlockJob *s = container_of(job, BackupBlockJob, common);
BackupCompleteData *data = opaque;
- bdrv_unref(s->target);
+ blk_unref(s->target);
block_job_completed(job, data->ret);
g_free(data);
@@ -378,8 +371,8 @@ static void coroutine_fn backup_run(void *opaque)
{
BackupBlockJob *job = opaque;
BackupCompleteData *data;
- BlockDriverState *bs = job->common.bs;
- BlockDriverState *target = job->target;
+ BlockDriverState *bs = blk_bs(job->common.blk);
+ BlockBackend *target = job->target;
int64_t start, end;
int64_t sectors_per_cluster = cluster_size_sectors(job);
int ret = 0;
@@ -468,7 +461,7 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_unlock(&job->flush_rwlock);
g_free(job->done_bitmap);
- bdrv_op_unblock_all(target, job->common.blocker);
+ bdrv_op_unblock_all(blk_bs(target), job->common.blocker);
data = g_malloc(sizeof(*data));
data->ret = ret;
@@ -548,9 +541,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
goto error;
}
+ job->target = blk_new();
+ blk_insert_bs(job->target, target);
+
job->on_source_error = on_source_error;
job->on_target_error = on_target_error;
- job->target = target;
job->sync_mode = sync_mode;
job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
sync_bitmap : NULL;
@@ -558,7 +553,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
/* If there is no backing file on the target, we cannot rely on COW if our
* backup cluster size is smaller than the target cluster size. Even for
* targets with a backing file, try to avoid COW if possible. */
- ret = bdrv_get_info(job->target, &bdi);
+ ret = bdrv_get_info(target, &bdi);
if (ret < 0 && !target->backing) {
error_setg_errno(errp, -ret,
"Couldn't determine the cluster size of the target image, "
@@ -585,6 +580,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
}
if (job) {
+ blk_unref(job->target);
block_job_unref(&job->common);
}
}
diff --git a/block/io.c b/block/io.c
index bc2eda2..2d832aa 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1108,15 +1108,6 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
}
-int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
-{
- trace_bdrv_co_readv_no_serialising(bs, sector_num, nb_sectors);
-
- return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov,
- BDRV_REQ_NO_SERIALISING);
-}
-
#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 66477ff..717785e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3293,8 +3293,8 @@ static void do_drive_backup(const char *device, const char *target,
backup_start(bs, target_bs, speed, sync, bmap,
on_source_error, on_target_error,
block_job_cb, bs, txn, &local_err);
+ bdrv_unref(target_bs);
if (local_err != NULL) {
- bdrv_unref(target_bs);
error_propagate(errp, local_err);
goto out;
}
@@ -3378,12 +3378,10 @@ void do_blockdev_backup(const char *device, const char *target,
}
target_bs = blk_bs(target_blk);
- bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
on_target_error, block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
- bdrv_unref(target_bs);
error_propagate(errp, local_err);
}
out:
diff --git a/include/block/block.h b/include/block/block.h
index 0a4b973..70ea299 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -242,8 +242,6 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov);
-int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov);
/*
diff --git a/trace-events b/trace-events
index 6966dd6..4450d8f 100644
--- a/trace-events
+++ b/trace-events
@@ -72,7 +72,6 @@ bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %
bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t cluster_sector_num, int cluster_nb_sectors) "bs %p sector_num %"PRId64" nb_sectors %d cluster_sector_num %"PRId64" cluster_nb_sectors %d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 30/31] commit: Use BlockBackend for I/O
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (28 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 29/31] backup: Use BlockBackend for I/O Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-25 17:39 ` [Qemu-devel] [PULL 31/31] blockjob: Remove BlockJob.bs Kevin Wolf
2016-05-26 14:06 ` [Qemu-devel] [PULL 00/31] Block layer patches Peter Maydell
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This changes the commit block job to use the job's BlockBackend for
performing its I/O. job->bs isn't used by the commit code any more
afterwards.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/commit.c | 53 +++++++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 20 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index f308c8c..8a00e11 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,28 +36,36 @@ typedef struct CommitBlockJob {
BlockJob common;
RateLimit limit;
BlockDriverState *active;
- BlockDriverState *top;
- BlockDriverState *base;
+ BlockBackend *top;
+ BlockBackend *base;
BlockdevOnError on_error;
int base_flags;
int orig_overlay_flags;
char *backing_file_str;
} CommitBlockJob;
-static int coroutine_fn commit_populate(BlockDriverState *bs,
- BlockDriverState *base,
+static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
int64_t sector_num, int nb_sectors,
void *buf)
{
int ret = 0;
+ QEMUIOVector qiov;
+ struct iovec iov = {
+ .iov_base = buf,
+ .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
+ };
- ret = bdrv_read(bs, sector_num, buf, nb_sectors);
- if (ret) {
+ qemu_iovec_init_external(&qiov, &iov, 1);
+
+ ret = blk_co_preadv(bs, sector_num * BDRV_SECTOR_SIZE,
+ qiov.size, &qiov, 0);
+ if (ret < 0) {
return ret;
}
- ret = bdrv_write(base, sector_num, buf, nb_sectors);
- if (ret) {
+ ret = blk_co_pwritev(base, sector_num * BDRV_SECTOR_SIZE,
+ qiov.size, &qiov, 0);
+ if (ret < 0) {
return ret;
}
@@ -73,8 +81,8 @@ static void commit_complete(BlockJob *job, void *opaque)
CommitBlockJob *s = container_of(job, CommitBlockJob, common);
CommitCompleteData *data = opaque;
BlockDriverState *active = s->active;
- BlockDriverState *top = s->top;
- BlockDriverState *base = s->base;
+ BlockDriverState *top = blk_bs(s->top);
+ BlockDriverState *base = blk_bs(s->base);
BlockDriverState *overlay_bs;
int ret = data->ret;
@@ -94,6 +102,8 @@ static void commit_complete(BlockJob *job, void *opaque)
bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
}
g_free(s->backing_file_str);
+ blk_unref(s->top);
+ blk_unref(s->base);
block_job_completed(&s->common, ret);
g_free(data);
}
@@ -102,8 +112,6 @@ static void coroutine_fn commit_run(void *opaque)
{
CommitBlockJob *s = opaque;
CommitCompleteData *data;
- BlockDriverState *top = s->top;
- BlockDriverState *base = s->base;
int64_t sector_num, end;
int ret = 0;
int n = 0;
@@ -111,27 +119,27 @@ static void coroutine_fn commit_run(void *opaque)
int bytes_written = 0;
int64_t base_len;
- ret = s->common.len = bdrv_getlength(top);
+ ret = s->common.len = blk_getlength(s->top);
if (s->common.len < 0) {
goto out;
}
- ret = base_len = bdrv_getlength(base);
+ ret = base_len = blk_getlength(s->base);
if (base_len < 0) {
goto out;
}
if (base_len < s->common.len) {
- ret = bdrv_truncate(base, s->common.len);
+ ret = blk_truncate(s->base, s->common.len);
if (ret) {
goto out;
}
}
end = s->common.len >> BDRV_SECTOR_BITS;
- buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
+ buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
for (sector_num = 0; sector_num < end; sector_num += n) {
uint64_t delay_ns = 0;
@@ -146,7 +154,8 @@ wait:
break;
}
/* Copy if allocated above the base */
- ret = bdrv_is_allocated_above(top, base, sector_num,
+ ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
+ sector_num,
COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
&n);
copy = (ret == 1);
@@ -158,7 +167,7 @@ wait:
goto wait;
}
}
- ret = commit_populate(top, base, sector_num, n, buf);
+ ret = commit_populate(s->top, s->base, sector_num, n, buf);
bytes_written += n * BDRV_SECTOR_SIZE;
}
if (ret < 0) {
@@ -253,8 +262,12 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
return;
}
- s->base = base;
- s->top = top;
+ s->base = blk_new();
+ blk_insert_bs(s->base, base);
+
+ s->top = blk_new();
+ blk_insert_bs(s->top, top);
+
s->active = bs;
s->base_flags = orig_base_flags;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PULL 31/31] blockjob: Remove BlockJob.bs
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (29 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 30/31] commit: " Kevin Wolf
@ 2016-05-25 17:39 ` Kevin Wolf
2016-05-26 14:06 ` [Qemu-devel] [PULL 00/31] Block layer patches Peter Maydell
31 siblings, 0 replies; 34+ messages in thread
From: Kevin Wolf @ 2016-05-25 17:39 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
There is a single remaining user in qemu-img, and another one in a test
case, both of which can be trivially converted to using BlockJob.blk
instead.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
blockjob.c | 1 -
include/block/blockjob.h | 1 -
qemu-img.c | 2 +-
tests/test-blockjob-txn.c | 3 ++-
4 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 2097e1d..c095cc5 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -83,7 +83,6 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
job->driver = driver;
job->id = g_strdup(bdrv_get_device_name(bs));
- job->bs = bs;
job->blk = blk;
job->cb = cb;
job->opaque = opaque;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 32012af..86d2807 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -82,7 +82,6 @@ struct BlockJob {
const BlockJobDriver *driver;
/** The block device on which the job is operating. */
- BlockDriverState *bs; /* TODO Remove */
BlockBackend *blk;
/**
diff --git a/qemu-img.c b/qemu-img.c
index 2065348..4b56ad3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -775,7 +775,7 @@ static void common_block_job_cb(void *opaque, int ret)
static void run_block_job(BlockJob *job, Error **errp)
{
- AioContext *aio_context = bdrv_get_aio_context(job->bs);
+ AioContext *aio_context = blk_get_aio_context(job->blk);
do {
aio_poll(aio_context, true);
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 55fad95..828389b 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -15,6 +15,7 @@
#include "qapi/error.h"
#include "qemu/main-loop.h"
#include "block/blockjob.h"
+#include "sysemu/block-backend.h"
typedef struct {
BlockJob common;
@@ -30,7 +31,7 @@ static const BlockJobDriver test_block_job_driver = {
static void test_block_job_complete(BlockJob *job, void *opaque)
{
- BlockDriverState *bs = job->bs;
+ BlockDriverState *bs = blk_bs(job->blk);
int rc = (intptr_t)opaque;
if (block_job_is_cancelled(job)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PULL 00/31] Block layer patches
2016-05-25 17:39 [Qemu-devel] [PULL 00/31] Block layer patches Kevin Wolf
` (30 preceding siblings ...)
2016-05-25 17:39 ` [Qemu-devel] [PULL 31/31] blockjob: Remove BlockJob.bs Kevin Wolf
@ 2016-05-26 14:06 ` Peter Maydell
31 siblings, 0 replies; 34+ messages in thread
From: Peter Maydell @ 2016-05-26 14:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
On 25 May 2016 at 18:39, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 287db79df8af8e31f18e262feb5e05103a09e4d4:
>
> Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2016-05-24 13:06:33 +0100)
>
> are available in the git repository at:
>
>
> git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to b75536c9fa742f887304769d0608557bb8e3a27f:
>
> blockjob: Remove BlockJob.bs (2016-05-25 19:04:21 +0200)
>
> ----------------------------------------------------------------
> Block layer patches
>
> ----------------------------------------------------------------
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 34+ messages in thread