[RFC,1/2] snapshot: create helper to test that block drivers supports snapshots

The patch enforces proper locking for this operation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
---
Patches are compile-tested only. Sent to check the approach, naming and
functions placement. Functions are returning bad BlockDriver via
parameter to make clear distinction when I'll have to return BS to write.

 block/snapshot.c         | 35 +++++++++++++++++++++++++++++++++++
 include/block/snapshot.h |  9 +++++++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 48 insertions(+), 13 deletions(-)

"Denis V. Lunev" <den@openvz.org> wrote:
> The patch enforces proper locking for this operation.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> ---
> Patches are compile-tested only. Sent to check the approach, naming and
> functions placement. Functions are returning bad BlockDriver via
> parameter to make clear distinction when I'll have to return BS to write.
>
>  block/snapshot.c         | 35 +++++++++++++++++++++++++++++++++++
>  include/block/snapshot.h |  9 +++++++++
>  migration/savevm.c       | 17 ++++-------------
>  3 files changed, 48 insertions(+), 13 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 89500f2..6b5ce4e 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -25,6 +25,7 @@
>  #include "block/snapshot.h"
>  #include "block/block_int.h"
>  #include "qapi/qmp/qerror.h"
> +#include "monitor/monitor.h"

You don't need the monitor here O:-)

>  
>  QemuOptsList internal_snapshot_opts = {
>      .name = "snapshot",
> @@ -356,3 +357,37 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>  
>      return ret;
>  }
> +
> +
> +/* Group operations. All block drivers are involved.
> + * These functions will properly handle dataplace (take aio_context_acquire
> + * when appropriate for appropriate block drivers
> + *
> + * Returned block driver will be always locked.
> + */
> +
> +bool bdrv_snapshot_all_can_snapshot(BlockDriverState **first_bad_bs)

bdrv_snapshot_is_possible???

> +{
> +    BlockDriverState *bs;
> +
> +    while ((bs = bdrv_next(bs))) {
> +        bool ok;
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +            continue;
> +        }
> +
> +        aio_context_acquire(ctx);

I think that you should get the lock before the bdrv_is_inserted, but
who am I to know for sure O:-)

> +        ok = bdrv_can_snapshot(bs);
> +        aio_context_release(ctx);
> +
> +        if (!ok) {
> +            *first_bad_bs = bs;
> +            return false;
> +        }
> +    }
> +
> +    *first_bad_bs = NULL;
> +    return true;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 770d9bb..61b4b5d 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -75,4 +75,13 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>  int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>                                           const char *id_or_name,
>                                           Error **errp);
> +
> +
> +/* Group operations. All block drivers are involved.
> + * These functions will properly handle dataplace (take aio_context_acquire
> + * when appropriate for appropriate block drivers
> + *
> + */
> +bool bdrv_snapshot_all_can_snapshot(BlockDriverState **first_bad_bs);
> +
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index dbcc39a..91ba0bf 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1290,19 +1290,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      const char *name = qdict_get_try_str(qdict, "name");
>      Error *local_err = NULL;
>  
> -    /* Verify if there is a device that doesn't support snapshots and is writable */
> -    bs = NULL;
> -    while ((bs = bdrv_next(bs))) {
> -
> -        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> -            continue;
> -        }
> -
> -        if (!bdrv_can_snapshot(bs)) {
> -            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
> -                               bdrv_get_device_name(bs));
> -            return;
> -        }
> +    if (bdrv_snapshot_all_can_snapshot(&bs)) {
> +        monitor_printf(mon, "Device '%s' is writable but does not "
> +                       "support snapshots.\n", bdrv_get_device_name(bs));
> +        return;
>      }
>  
>      bs = find_vmstate_bs();

ok with the savevm.c changes.

On Wed, Nov 04, 2015 at 01:07:44PM +0100, Juan Quintela wrote:
> > +{
> > +    BlockDriverState *bs;
> > +
> > +    while ((bs = bdrv_next(bs))) {
> > +        bool ok;
> > +        AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > +        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> > +            continue;
> > +        }
> > +
> > +        aio_context_acquire(ctx);
> 
> I think that you should get the lock before the bdrv_is_inserted, but
> who am I to know for sure O:-)

Yes, please acquire the AioContext before bdrv_is_inserted(bs).

On Wed, Nov 04, 2015 at 02:31:55PM +0300, Denis V. Lunev wrote:
> The patch enforces proper locking for this operation.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> ---
> Patches are compile-tested only. Sent to check the approach, naming and
> functions placement. Functions are returning bad BlockDriver via
> parameter to make clear distinction when I'll have to return BS to write.
> 
>  block/snapshot.c         | 35 +++++++++++++++++++++++++++++++++++
>  include/block/snapshot.h |  9 +++++++++
>  migration/savevm.c       | 17 ++++-------------
>  3 files changed, 48 insertions(+), 13 deletions(-)

Kevin is the maintainer of block/snapshot.c.  Please include him in
future revisions.

Looks fine to me, besides the comments that Juan already raised.

Stefan