OpenE2K/qemu-e2k
13
4
Fork 0
Code Issues 4 Pull Requests Projects 2 Releases Activity

block: Make bdrv_next() keep strong references

Browse Source 
On one hand, it is a good idea for bdrv_next() to return a strong
reference because ideally nearly every pointer should be refcounted.
This fixes intermittent failure of iotest 194.

On the other, it is absolutely necessary for bdrv_next() itself to keep
a strong reference to both the BB (in its first phase) and the BDS (at
least in the second phase) because when called the next time, it will
dereference those objects to get a link to the next one.  Therefore, it
needs these objects to stay around until then.  Just storing the pointer
to the next in the iterator is not really viable because that pointer
might become invalid as well.

Both arguments taken together means we should probably just invoke
bdrv_ref() and blk_ref() in bdrv_next().  This means we have to assert
that bdrv_next() is always called from the main loop, but that was
probably necessary already before this patch and judging from the
callers, it also looks to actually be the case.

Keeping these strong references means however that callers need to give
them up if they decide to abort the iteration early.  They can do so
through the new bdrv_next_cleanup() function.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110172545.32609-1-mreitz@redhat.com
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
This commit is contained in:
Max Reitz 2017-11-10 18:25:45 +01:00
parent 08546bcfb2
commit 5e003f17ec
5 changed files with 57 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
block.c
View File

@ -4255,6 +4255,7 @@ void bdrv_invalidate_cache_all(Error **errp)
aio_context_release(aio_context);
if (local_err) {
error_propagate(errp, local_err);
bdrv_next_cleanup(&it);
return;
}
}
@ -4330,6 +4331,7 @@ int bdrv_inactivate_all(void)
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
ret = bdrv_inactivate_recurse(bs, pass);
if (ret < 0) {
bdrv_next_cleanup(&it);
goto out;
}
}
@ -4864,6 +4866,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
/* candidate is the first non filter */
if (perm) {
bdrv_next_cleanup(&it);
return true;
}
}

48
block/block-backend.c
View File

@ -442,21 +442,37 @@ BlockBackend *blk_next(BlockBackend *blk)
* the monitor or attached to a BlockBackend */
BlockDriverState *bdrv_next(BdrvNextIterator *it)
{
BlockDriverState *bs;
BlockDriverState *bs, *old_bs;
/* Must be called from the main loop */
assert(qemu_get_current_aio_context() == qemu_get_aio_context());
/* 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
* if the BB is the first one in the parent list of the BDS. */
if (it->phase == BDRV_NEXT_BACKEND_ROOTS) {
BlockBackend *old_blk = it->blk;
old_bs = old_blk ? blk_bs(old_blk) : NULL;
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));
if (it->blk) {
blk_ref(it->blk);
}
blk_unref(old_blk);
if (bs) {
bdrv_ref(bs);
bdrv_unref(old_bs);
return bs;
}
it->phase = BDRV_NEXT_MONITOR_OWNED;
} else {
old_bs = it->bs;
}
/* Then return the monitor-owned BDSes without a BB attached. Ignore all
@ -467,18 +483,46 @@ BlockDriverState *bdrv_next(BdrvNextIterator *it)
bs = it->bs;
} while (bs && bdrv_has_blk(bs));
if (bs) {
bdrv_ref(bs);
}
bdrv_unref(old_bs);
return bs;
}
BlockDriverState *bdrv_first(BdrvNextIterator *it)
static void bdrv_next_reset(BdrvNextIterator *it)
{
*it = (BdrvNextIterator) {
.phase = BDRV_NEXT_BACKEND_ROOTS,
};
}
BlockDriverState *bdrv_first(BdrvNextIterator *it)
{
bdrv_next_reset(it);
return bdrv_next(it);
}
/* Must be called when aborting a bdrv_next() iteration before
* bdrv_next() returns NULL */
void bdrv_next_cleanup(BdrvNextIterator *it)
{
/* Must be called from the main loop */
assert(qemu_get_current_aio_context() == qemu_get_aio_context());
if (it->phase == BDRV_NEXT_BACKEND_ROOTS) {
if (it->blk) {
bdrv_unref(blk_bs(it->blk));
blk_unref(it->blk);
}
} else {
bdrv_unref(it->bs);
}
bdrv_next_reset(it);
}
/*
* Add a BlockBackend into the list of backends referenced by the monitor, with
* the given @name acting as the handle for the monitor.

6
block/snapshot.c
View File

@ -417,6 +417,7 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
}
aio_context_release(ctx);
if (!ok) {
bdrv_next_cleanup(&it);
goto fail;
}
}
@ -444,6 +445,7 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
}
aio_context_release(ctx);
if (ret < 0) {
bdrv_next_cleanup(&it);
goto fail;
}
}
@ -469,6 +471,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
}
aio_context_release(ctx);
if (err < 0) {
bdrv_next_cleanup(&it);
goto fail;
}
}
@ -494,6 +497,7 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
}
aio_context_release(ctx);
if (err < 0) {
bdrv_next_cleanup(&it);
goto fail;
}
}
@ -525,6 +529,7 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
}
aio_context_release(ctx);
if (err < 0) {
bdrv_next_cleanup(&it);
goto fail;
}
}
@ -548,6 +553,7 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
aio_context_release(ctx);
if (found) {
bdrv_next_cleanup(&it);
break;
}
}

1
include/block/block.h
View File

@ -461,6 +461,7 @@ typedef struct BdrvNextIterator {
BlockDriverState *bdrv_first(BdrvNextIterator *it);
BlockDriverState *bdrv_next(BdrvNextIterator *it);
void bdrv_next_cleanup(BdrvNextIterator *it);
BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
bool bdrv_is_encrypted(BlockDriverState *bs);

1
migration/block.c
View File

@ -415,6 +415,7 @@ static int init_blk_migration(QEMUFile *f)
sectors = bdrv_nb_sectors(bs);
if (sectors <= 0) {
ret = sectors;
bdrv_next_cleanup(&it);
goto out;
}