block: Make bdrv_next() keep strong references
PATCH v2 1/1 block: Make bdrv_next() keep strong references
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.
---
v2: Instead of keeping the strong reference in bdrv_drain_all_*() only,
have them for all callers of bdrv_next() [Fam, Kevin] (Completely different patch now, so no git-backport-diff included
here)
--- include/block/block.h | 1 + block.c | 3 +++ block/block-backend.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- block/snapshot.c | 6 ++++++ migration/block.c | 1 + 5 files changed, 57 insertions(+), 2 deletions(-)
include/block/block.h +1 -0
@@ -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);
block.c +3 -0
@@ -4226,6 +4226,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 ; } }
@@ -4297,6 +4298,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; } }
@@ -4828,6 +4830,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)/* candidate is the first non filter */ if (perm) { +bdrv_next_cleanup (&it);return true; } }
block/block-backend.c +46 -2
85 lines not on this page · raw message
block/snapshot.c +6 -0
@@ -403,6 +403,7 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) }aio_context_release (ctx);if (!ok) { +bdrv_next_cleanup (&it); goto fail; } }
@@ -430,6 +431,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; } }
@@ -455,6 +457,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; } }
@@ -480,6 +483,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; } }
@@ -511,6 +515,7 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, }aio_context_release (ctx);if (err <0 ) { +bdrv_next_cleanup (&it); goto fail; } }
@@ -534,6 +539,7 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)aio_context_release (ctx);if (found) { +bdrv_next_cleanup (&it);break ; } }
migration/block.c +1 -0
@@ -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; }
Signature · 1 line
2.13.6
block/block-backend.c BlockBackend *blk_next(BlockBackend *blk) · line 472
Quoted 5 lines · Max Reitz
if (bs) {block/block-backend.c +2
-0+bdrv_ref (bs); +bdrv_unref (old_bs);return bs; }
Maybe instead goto...
block/block-backend.c BlockDriverState *bdrv_next(BdrvNextIterator *it) · line 484
Quoted 9 lines
it->phase = BDRV_NEXT_MONITOR_OWNED;block/block-backend.c +2
-0+ }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));
... here?
Paolo
Quoted 6 lines
block/block-backend.c +5
-0+if (bs) { +bdrv_ref (bs); + } +bdrv_unref (old_bs); +return bs;
Quoted 14 lines · Paolo Bonzini
block/block-backend.c BlockBackend *blk_next(BlockBackend *blk) · line 472
Quoted 5 lines · Max Reitz
if (bs) {block/block-backend.c +2
-0+bdrv_ref (bs); +bdrv_unref (old_bs);return bs; }Maybe instead goto...
Quoted 5 lines
it->phase = BDRV_NEXT_MONITOR_OWNED;block/block-backend.c +2
-0+ }else { + old_bs = it->bs; }
Quoted 6 lines
block/block-backend.c BlockDriverState *bdrv_next(BdrvNextIterator *it) · line 484
Quoted 4 lines
/* Then return the monitor-owned BDSes without a BB attached. Ignore allblock/block-backend.c +0
-0@@ -467,18 +483,46 @@ BlockDriverState *bdrv_next(BdrvNextIterator *it) bs = it->bs; }while (bs &&bdrv_has_blk (bs));... here?
I wouldn't mind too much, but I don't think a goto there makes the code easier to read.
Max
Quoted 9 lines
Paolo
Quoted 6 lines
block/block-backend.c +5
-0+if (bs) { +bdrv_ref (bs); + } +bdrv_unref (old_bs); +return bs;
Quoted 29 lines · Max Reitz
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()andblk_ref()inbdrv_next(). This means we have to assert thatbdrv_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 <[email protected]>Signed-off-by Max Reitz <[email protected]>---
v2: Instead of keeping the strong reference in bdrv_drain_all_*() only,
have them for all callers ofbdrv_next()[Fam, Kevin] (Completely different patch now, so no git-backport-diff included
here)
Quoted 7 lines
--- include/block/block.h | 1 + block.c | 3 +++ block/block-backend.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- block/snapshot.c | 6 ++++++ migration/block.c | 1 + 5 files changed, 57 insertions(+), 2 deletions(-)
Quoted 29 lines · Max Reitz
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()andblk_ref()inbdrv_next(). This means we have to assert thatbdrv_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 <[email protected]>Signed-off-by Max Reitz <[email protected]>---
v2: Instead of keeping the strong reference in bdrv_drain_all_*() only,
have them for all callers ofbdrv_next()[Fam, Kevin] (Completely different patch now, so no git-backport-diff included
here)
Quoted 7 lines
--- include/block/block.h | 1 + block.c | 3 +++ block/block-backend.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- block/snapshot.c | 6 ++++++ migration/block.c | 1 + 5 files changed, 57 insertions(+), 2 deletions(-)
Due to one supporter and otherwise lack of resistance: Applied to my block branch (https://github.com/XanClic/qemu/commits/block).
Max