| Message ID | 20201127144522.29991-4-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > We have too much comments for this feature. It seems better just don't > do it. Most of real users (tests don't count) have to create additional > reference. > > Drop also comment in external_snapshot_prepare: > - bdrv_append doesn't "remove" old bs in common sense, it sounds > strange > - the fact that bdrv_append can fail is obvious from the context > - the fact that we must rollback all changes in transaction abort is > known (it's the direct role of abort) > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block.c | 19 +++---------------- > block/backup-top.c | 1 - > block/commit.c | 1 + > block/mirror.c | 3 --- > blockdev.c | 4 ---- > tests/test-bdrv-drain.c | 2 +- > tests/test-bdrv-graph-mod.c | 2 ++ > 7 files changed, 7 insertions(+), 25 deletions(-) > > diff --git a/block.c b/block.c > index 0dd28f0902..55efef3c9d 100644 > --- a/block.c > +++ b/block.c > @@ -3145,11 +3145,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > goto out; > } > > - /* bdrv_append() consumes a strong reference to bs_snapshot > - * (i.e. it will call bdrv_unref() on it) even on error, 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, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -4608,10 +4603,8 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, > * > * This function does not create any image files. > * > - * bdrv_append() takes ownership of a bs_new reference and unrefs it because > - * that's what the callers commonly need. bs_new will be referenced by the old > - * parents of bs_top after bdrv_append() returns. If the caller needs to keep a > - * reference of its own, it must call bdrv_ref(). > + * Recent update: bdrv_append does NOT eat bs_new reference for now. Drop this > + * comment several moths later. A comment like this is unusual. Do you think there is a high risk of somebody introducing a new bdrv_append() in parallel and that they would read this comment when rebasing their existing patches? If we do keep the comment: s/for now/now/ (it has recently changed, we're not intending to change it later) and s/moths/months/. > */ > void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > Error **errp) > @@ -4621,20 +4614,14 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > bdrv_set_backing_hd(bs_new, bs_top, &local_err); > if (local_err) { > error_propagate(errp, local_err); > - goto out; > + return; > } > > bdrv_replace_node(bs_top, bs_new, &local_err); > if (local_err) { > error_propagate(errp, local_err); > bdrv_set_backing_hd(bs_new, NULL, &error_abort); > - goto out; Can we leave a return here just in case that new code will be added at the end of the function? > } > - > - /* bs_new is now referenced by its new parents, we don't need the > - * additional reference any more. */ > -out: > - bdrv_unref(bs_new); > } > > static void bdrv_delete(BlockDriverState *bs) > diff --git a/block/backup-top.c b/block/backup-top.c > index fe6883cc97..650ed6195c 100644 > --- a/block/backup-top.c > +++ b/block/backup-top.c > @@ -222,7 +222,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > > bdrv_drained_begin(source); > > - bdrv_ref(top); > bdrv_append(top, source, &local_err); > if (local_err) { > error_prepend(&local_err, "Cannot append backup-top filter: "); > diff --git a/block/commit.c b/block/commit.c > index 71db7ba747..61924bcf66 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -313,6 +313,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, > commit_top_bs->total_sectors = top->total_sectors; > > bdrv_append(commit_top_bs, top, &local_err); > + bdrv_unref(commit_top_bs); /* referenced by new parents or failed */ > if (local_err) { > commit_top_bs = NULL; > error_propagate(errp, local_err); > diff --git a/block/mirror.c b/block/mirror.c > index 8e1ad6eceb..13f7ecc998 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1605,9 +1605,6 @@ static BlockJob *mirror_start_job( > bs_opaque = g_new0(MirrorBDSOpaque, 1); > mirror_top_bs->opaque = bs_opaque; > > - /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep > - * it alive until block_job_create() succeeds even if bs has no parent. */ > - bdrv_ref(mirror_top_bs); > bdrv_drained_begin(bs); > bdrv_append(mirror_top_bs, bs, &local_err); > bdrv_drained_end(bs); > diff --git a/blockdev.c b/blockdev.c > index b5f11c524b..96c96f8ba6 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1587,10 +1587,6 @@ static void external_snapshot_prepare(BlkActionState *common, > goto out; > } > > - /* This removes our old bs and adds the new bs. This is an operation that > - * can fail, so we need to do it in .prepare; undoing it for abort is > - * always possible. */ This comment is still relevant, it's unrelated to the bdrv_ref(). > - bdrv_ref(state->new_bs); > bdrv_append(state->new_bs, state->old_bs, &local_err); > if (local_err) { > error_propagate(errp, local_err); Kevin
18.01.2021 17:18, Kevin Wolf wrote: > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: >> We have too much comments for this feature. It seems better just don't >> do it. Most of real users (tests don't count) have to create additional >> reference. >> >> Drop also comment in external_snapshot_prepare: >> - bdrv_append doesn't "remove" old bs in common sense, it sounds >> strange >> - the fact that bdrv_append can fail is obvious from the context >> - the fact that we must rollback all changes in transaction abort is >> known (it's the direct role of abort) >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block.c | 19 +++---------------- >> block/backup-top.c | 1 - >> block/commit.c | 1 + >> block/mirror.c | 3 --- >> blockdev.c | 4 ---- >> tests/test-bdrv-drain.c | 2 +- >> tests/test-bdrv-graph-mod.c | 2 ++ >> 7 files changed, 7 insertions(+), 25 deletions(-) >> >> diff --git a/block.c b/block.c >> index 0dd28f0902..55efef3c9d 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3145,11 +3145,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, >> goto out; >> } >> >> - /* bdrv_append() consumes a strong reference to bs_snapshot >> - * (i.e. it will call bdrv_unref() on it) even on error, 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, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> @@ -4608,10 +4603,8 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, >> * >> * This function does not create any image files. >> * >> - * bdrv_append() takes ownership of a bs_new reference and unrefs it because >> - * that's what the callers commonly need. bs_new will be referenced by the old >> - * parents of bs_top after bdrv_append() returns. If the caller needs to keep a >> - * reference of its own, it must call bdrv_ref(). >> + * Recent update: bdrv_append does NOT eat bs_new reference for now. Drop this >> + * comment several moths later. > > A comment like this is unusual. Do you think there is a high risk of > somebody introducing a new bdrv_append() in parallel and that they would > read this comment when rebasing their existing patches? Or even later, someone may remember that bdrv_append() eat the reference, and then face some strange behavior with a new call of bdrv_append(), finally go to check the function code and see the new comment.. I don't insist, we can drop the comment > > If we do keep the comment: s/for now/now/ (it has recently changed, > we're not intending to change it later) and s/moths/months/. > >> */ >> void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, >> Error **errp) >> @@ -4621,20 +4614,14 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, >> bdrv_set_backing_hd(bs_new, bs_top, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> - goto out; >> + return; >> } >> >> bdrv_replace_node(bs_top, bs_new, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> bdrv_set_backing_hd(bs_new, NULL, &error_abort); >> - goto out; > > Can we leave a return here just in case that new code will be added at > the end of the function? sure > >> } >> - >> - /* bs_new is now referenced by its new parents, we don't need the >> - * additional reference any more. */ >> -out: >> - bdrv_unref(bs_new); >> } >> >> static void bdrv_delete(BlockDriverState *bs) >> diff --git a/block/backup-top.c b/block/backup-top.c >> index fe6883cc97..650ed6195c 100644 >> --- a/block/backup-top.c >> +++ b/block/backup-top.c >> @@ -222,7 +222,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, >> >> bdrv_drained_begin(source); >> >> - bdrv_ref(top); >> bdrv_append(top, source, &local_err); >> if (local_err) { >> error_prepend(&local_err, "Cannot append backup-top filter: "); >> diff --git a/block/commit.c b/block/commit.c >> index 71db7ba747..61924bcf66 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -313,6 +313,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, >> commit_top_bs->total_sectors = top->total_sectors; >> >> bdrv_append(commit_top_bs, top, &local_err); >> + bdrv_unref(commit_top_bs); /* referenced by new parents or failed */ >> if (local_err) { >> commit_top_bs = NULL; >> error_propagate(errp, local_err); >> diff --git a/block/mirror.c b/block/mirror.c >> index 8e1ad6eceb..13f7ecc998 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -1605,9 +1605,6 @@ static BlockJob *mirror_start_job( >> bs_opaque = g_new0(MirrorBDSOpaque, 1); >> mirror_top_bs->opaque = bs_opaque; >> >> - /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep >> - * it alive until block_job_create() succeeds even if bs has no parent. */ >> - bdrv_ref(mirror_top_bs); >> bdrv_drained_begin(bs); >> bdrv_append(mirror_top_bs, bs, &local_err); >> bdrv_drained_end(bs); >> diff --git a/blockdev.c b/blockdev.c >> index b5f11c524b..96c96f8ba6 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1587,10 +1587,6 @@ static void external_snapshot_prepare(BlkActionState *common, >> goto out; >> } >> >> - /* This removes our old bs and adds the new bs. This is an operation that >> - * can fail, so we need to do it in .prepare; undoing it for abort is >> - * always possible. */ > > This comment is still relevant, it's unrelated to the bdrv_ref(). I described in commit message, why I dislike this comment :) I can keep it of course, it's not critical > >> - bdrv_ref(state->new_bs); >> bdrv_append(state->new_bs, state->old_bs, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); > > Kevin >
Am 18.01.2021 um 18:21 hat Vladimir Sementsov-Ogievskiy geschrieben: > 18.01.2021 17:18, Kevin Wolf wrote: > > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > We have too much comments for this feature. It seems better just don't > > > do it. Most of real users (tests don't count) have to create additional > > > reference. > > > > > > Drop also comment in external_snapshot_prepare: > > > - bdrv_append doesn't "remove" old bs in common sense, it sounds > > > strange > > > - the fact that bdrv_append can fail is obvious from the context > > > - the fact that we must rollback all changes in transaction abort is > > > known (it's the direct role of abort) > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > diff --git a/blockdev.c b/blockdev.c > > > index b5f11c524b..96c96f8ba6 100644 > > > --- a/blockdev.c > > > +++ b/blockdev.c > > > @@ -1587,10 +1587,6 @@ static void external_snapshot_prepare(BlkActionState *common, > > > goto out; > > > } > > > - /* This removes our old bs and adds the new bs. This is an operation that > > > - * can fail, so we need to do it in .prepare; undoing it for abort is > > > - * always possible. */ > > > > This comment is still relevant, it's unrelated to the bdrv_ref(). > > I described in commit message, why I dislike this comment :) I can > keep it of course, it's not critical Ah, right, I missed this bit in the commit message (or forgot it until I reached this hunk) and thought it was an accidental removal. If it's intentional, no reason to change the patch. Kevin
diff --git a/block.c b/block.c index 0dd28f0902..55efef3c9d 100644 --- a/block.c +++ b/block.c @@ -3145,11 +3145,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, goto out; } - /* bdrv_append() consumes a strong reference to bs_snapshot - * (i.e. it will call bdrv_unref() on it) even on error, 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, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -4608,10 +4603,8 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, * * This function does not create any image files. * - * bdrv_append() takes ownership of a bs_new reference and unrefs it because - * that's what the callers commonly need. bs_new will be referenced by the old - * parents of bs_top after bdrv_append() returns. If the caller needs to keep a - * reference of its own, it must call bdrv_ref(). + * Recent update: bdrv_append does NOT eat bs_new reference for now. Drop this + * comment several moths later. */ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp) @@ -4621,20 +4614,14 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, bdrv_set_backing_hd(bs_new, bs_top, &local_err); if (local_err) { error_propagate(errp, local_err); - goto out; + return; } bdrv_replace_node(bs_top, bs_new, &local_err); if (local_err) { error_propagate(errp, local_err); bdrv_set_backing_hd(bs_new, NULL, &error_abort); - goto out; } - - /* bs_new is now referenced by its new parents, we don't need the - * additional reference any more. */ -out: - bdrv_unref(bs_new); } static void bdrv_delete(BlockDriverState *bs) diff --git a/block/backup-top.c b/block/backup-top.c index fe6883cc97..650ed6195c 100644 --- a/block/backup-top.c +++ b/block/backup-top.c @@ -222,7 +222,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, bdrv_drained_begin(source); - bdrv_ref(top); bdrv_append(top, source, &local_err); if (local_err) { error_prepend(&local_err, "Cannot append backup-top filter: "); diff --git a/block/commit.c b/block/commit.c index 71db7ba747..61924bcf66 100644 --- a/block/commit.c +++ b/block/commit.c @@ -313,6 +313,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, commit_top_bs->total_sectors = top->total_sectors; bdrv_append(commit_top_bs, top, &local_err); + bdrv_unref(commit_top_bs); /* referenced by new parents or failed */ if (local_err) { commit_top_bs = NULL; error_propagate(errp, local_err); diff --git a/block/mirror.c b/block/mirror.c index 8e1ad6eceb..13f7ecc998 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1605,9 +1605,6 @@ static BlockJob *mirror_start_job( bs_opaque = g_new0(MirrorBDSOpaque, 1); mirror_top_bs->opaque = bs_opaque; - /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep - * it alive until block_job_create() succeeds even if bs has no parent. */ - bdrv_ref(mirror_top_bs); bdrv_drained_begin(bs); bdrv_append(mirror_top_bs, bs, &local_err); bdrv_drained_end(bs); diff --git a/blockdev.c b/blockdev.c index b5f11c524b..96c96f8ba6 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1587,10 +1587,6 @@ static void external_snapshot_prepare(BlkActionState *common, goto out; } - /* This removes our old bs and adds the new bs. This is an operation that - * can fail, so we need to do it in .prepare; undoing it for abort is - * always possible. */ - bdrv_ref(state->new_bs); bdrv_append(state->new_bs, state->old_bs, &local_err); if (local_err) { error_propagate(errp, local_err); diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index 8a29e33e00..892f7f47d8 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -1478,7 +1478,6 @@ static void test_append_to_drained(void) g_assert_cmpint(base_s->drain_count, ==, 1); g_assert_cmpint(base->in_flight, ==, 0); - /* Takes ownership of overlay, so we don't have to unref it later */ bdrv_append(overlay, base, &error_abort); g_assert_cmpint(base->in_flight, ==, 0); g_assert_cmpint(overlay->in_flight, ==, 0); @@ -1495,6 +1494,7 @@ static void test_append_to_drained(void) g_assert_cmpint(overlay->quiesce_counter, ==, 0); g_assert_cmpint(overlay_s->drain_count, ==, 0); + bdrv_unref(overlay); bdrv_unref(base); blk_unref(blk); } diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c index 27e3361a60..cfe096c9af 100644 --- a/tests/test-bdrv-graph-mod.c +++ b/tests/test-bdrv-graph-mod.c @@ -138,6 +138,7 @@ static void test_update_perm_tree(void) bdrv_append(filter, bs, &local_err); error_free_or_abort(&local_err); + bdrv_unref(filter); blk_unref(root); } @@ -202,6 +203,7 @@ static void test_should_update_child(void) bdrv_append(filter, bs, &error_abort); g_assert(target->backing->bs == bs); + bdrv_unref(filter); bdrv_unref(bs); blk_unref(root); }
We have too much comments for this feature. It seems better just don't do it. Most of real users (tests don't count) have to create additional reference. Drop also comment in external_snapshot_prepare: - bdrv_append doesn't "remove" old bs in common sense, it sounds strange - the fact that bdrv_append can fail is obvious from the context - the fact that we must rollback all changes in transaction abort is known (it's the direct role of abort) Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block.c | 19 +++---------------- block/backup-top.c | 1 - block/commit.c | 1 + block/mirror.c | 3 --- blockdev.c | 4 ---- tests/test-bdrv-drain.c | 2 +- tests/test-bdrv-graph-mod.c | 2 ++ 7 files changed, 7 insertions(+), 25 deletions(-)