| Message ID | 1447165535-31263-6-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote: > +int bdrv_all_find_snapshot(const char *name, bool read_only, > + BlockDriverState **first_bad_bs) > +{ > + QEMUSnapshotInfo sn; > + int err = 0; > + BlockDriverState *bs = NULL; > + > + while (err == 0 && (bs = bdrv_next(bs))) { > + AioContext *ctx = bdrv_get_aio_context(bs); > + > + aio_context_acquire(ctx); > + if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) { > + err = bdrv_snapshot_find(bs, &sn, name); > + } > + aio_context_release(ctx); > + } > + > + *first_bad_bs = bs; > + return err; > +} It's difficult to see how bdrv_all_find_snapshot(read_only=true) is equivalent to what you replaced below: > @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > available_snapshots = g_new0(int, nb_sns); > total = 0; > for (i = 0; i < nb_sns; i++) { > - sn = &sn_tab[i]; > - available = 1; > - bs1 = NULL; > - > - while ((bs1 = bdrv_next(bs1))) { > - if (bdrv_can_snapshot(bs1) && bs1 != bs) { > - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); > - if (ret < 0) { > - available = 0; > - break; > - } > - } > - } > - > - if (available) { > + if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) { The original loop skips drives that cannot snapshot and the vmstate drive. The new code tries to find a snapshot all devices. To me it seems the semantics are changed. Can you explain why this change is safe?
On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote: > On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote: >> +int bdrv_all_find_snapshot(const char *name, bool read_only, >> + BlockDriverState **first_bad_bs) >> +{ >> + QEMUSnapshotInfo sn; >> + int err = 0; >> + BlockDriverState *bs = NULL; >> + >> + while (err == 0 && (bs = bdrv_next(bs))) { >> + AioContext *ctx = bdrv_get_aio_context(bs); >> + >> + aio_context_acquire(ctx); >> + if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) { >> + err = bdrv_snapshot_find(bs, &sn, name); >> + } >> + aio_context_release(ctx); >> + } >> + >> + *first_bad_bs = bs; >> + return err; >> +} > It's difficult to see how bdrv_all_find_snapshot(read_only=true) is > equivalent to what you replaced below: > >> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) >> available_snapshots = g_new0(int, nb_sns); >> total = 0; >> for (i = 0; i < nb_sns; i++) { >> - sn = &sn_tab[i]; >> - available = 1; >> - bs1 = NULL; >> - >> - while ((bs1 = bdrv_next(bs1))) { >> - if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); >> - if (ret < 0) { >> - available = 0; >> - break; >> - } >> - } >> - } >> - >> - if (available) { >> + if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) { > The original loop skips drives that cannot snapshot and the vmstate > drive. The new code tries to find a snapshot all devices. > > To me it seems the semantics are changed. Can you explain why this > change is safe? yep. you are once again right... OK.
On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote: > On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote: >> +int bdrv_all_find_snapshot(const char *name, bool read_only, >> + BlockDriverState **first_bad_bs) >> +{ >> + QEMUSnapshotInfo sn; >> + int err = 0; >> + BlockDriverState *bs = NULL; >> + >> + while (err == 0 && (bs = bdrv_next(bs))) { >> + AioContext *ctx = bdrv_get_aio_context(bs); >> + >> + aio_context_acquire(ctx); >> + if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) { >> + err = bdrv_snapshot_find(bs, &sn, name); >> + } >> + aio_context_release(ctx); >> + } >> + >> + *first_bad_bs = bs; >> + return err; >> +} > It's difficult to see how bdrv_all_find_snapshot(read_only=true) is > equivalent to what you replaced below: > >> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) >> available_snapshots = g_new0(int, nb_sns); >> total = 0; >> for (i = 0; i < nb_sns; i++) { >> - sn = &sn_tab[i]; >> - available = 1; >> - bs1 = NULL; >> - >> - while ((bs1 = bdrv_next(bs1))) { >> - if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); >> - if (ret < 0) { >> - available = 0; >> - break; >> - } >> - } >> - } >> - >> - if (available) { >> + if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) { > The original loop skips drives that cannot snapshot and the vmstate > drive. The new code tries to find a snapshot all devices. > > To me it seems the semantics are changed. Can you explain why this > change is safe? argh... This code is used for 'virsh qemu-monitor-command --hmp rhel7 info snapshots' and NOT used for 'virsh snapshot-list rhel7' or used in a very different way. This is the root of the problem. You are right, the command is broken completely. Some refactoring is necessary here. Den
diff --git a/block/snapshot.c b/block/snapshot.c index 9f07a63..97dc315 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -423,3 +423,24 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs) *first_bad_bs = bs; return err; } + +int bdrv_all_find_snapshot(const char *name, bool read_only, + BlockDriverState **first_bad_bs) +{ + QEMUSnapshotInfo sn; + int err = 0; + BlockDriverState *bs = NULL; + + while (err == 0 && (bs = bdrv_next(bs))) { + AioContext *ctx = bdrv_get_aio_context(bs); + + aio_context_acquire(ctx); + if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) { + err = bdrv_snapshot_find(bs, &sn, name); + } + aio_context_release(ctx); + } + + *first_bad_bs = bs; + return err; +} diff --git a/include/block/snapshot.h b/include/block/snapshot.h index 0a176c7..0fae32b 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -85,5 +85,7 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs); int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs, Error **err); int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs); +int bdrv_all_find_snapshot(const char *name, bool read_only, + BlockDriverState **first_bad_bs); #endif diff --git a/migration/savevm.c b/migration/savevm.c index d18ff13..90aa565 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1383,6 +1383,18 @@ int load_vmstate(const char *name) QEMUFile *f; int ret; + if (!bdrv_all_can_snapshot(&bs)) { + error_report("Device '%s' is writable but does not support snapshots.", + bdrv_get_device_name(bs)); + return -ENOTSUP; + } + ret = bdrv_all_find_snapshot(name, false, &bs); + if (ret < 0) { + error_report("Device '%s' does not have the requested snapshot '%s'", + bdrv_get_device_name(bs), name); + return ret; + } + bs_vm_state = find_vmstate_bs(); if (!bs_vm_state) { error_report("No block device supports snapshots"); @@ -1399,29 +1411,6 @@ int load_vmstate(const char *name) return -EINVAL; } - /* Verify if there is any device that doesn't support snapshots and is - writable and check if the requested snapshot is available too. */ - bs = NULL; - while ((bs = bdrv_next(bs))) { - - if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) { - continue; - } - - if (!bdrv_can_snapshot(bs)) { - error_report("Device '%s' is writable but does not support snapshots.", - bdrv_get_device_name(bs)); - return -ENOTSUP; - } - - ret = bdrv_snapshot_find(bs, &sn, name); - if (ret < 0) { - error_report("Device '%s' does not have the requested snapshot '%s'", - bdrv_get_device_name(bs), name); - return ret; - } - } - /* Flush all IO requests so they don't interfere with the new state. */ bdrv_drain_all(); @@ -1475,8 +1464,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) void hmp_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; - QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; - int nb_sns, i, ret, available; + QEMUSnapshotInfo *sn_tab, *sn; + int nb_sns, i; int total; int *available_snapshots; @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) available_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { - sn = &sn_tab[i]; - available = 1; - bs1 = NULL; - - while ((bs1 = bdrv_next(bs1))) { - if (bdrv_can_snapshot(bs1) && bs1 != bs) { - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); - if (ret < 0) { - available = 0; - break; - } - } - } - - if (available) { + if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) { available_snapshots[total] = i; total++; }
to check that snapshot is available for all loaded block drivers. The ability to switch to snapshot is verified separately using bdrv_all_can_snapshot. The patch also ensures proper locking. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Juan Quintela <quintela@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> --- block/snapshot.c | 21 ++++++++++++++++++ include/block/snapshot.h | 2 ++ migration/savevm.c | 55 +++++++++++++----------------------------------- 3 files changed, 38 insertions(+), 40 deletions(-)