Re: [Qemu-devel] [PATCH v4] savevm: Really verify if a drive supports snapshots

[Kevin Wolf]

Am 08.06.2010 06:39, schrieb MORITA Kazutaka:
> At Fri,  4 Jun 2010 16:35:59 -0300,
> Miguel Di Ciurcio Filho wrote:
>>
>> Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.
>>
>> First issue: Their names implies different porpouses, but they do the same 
>> thing
>> and have exactly the same code. Maybe copied and pasted and forgotten?
>> bdrv_has_snapshot() is called in various places for actually checking if 
>> there
>> is snapshots or not.
>>
>> Second issue: the way bdrv_can_snapshot() verifies if a block driver 
>> supports or
>> not snapshots does not catch all cases. E.g.: a raw image.
>>
>> So when do_savevm() is called, first thing it does is to set a global
>> BlockDriverState to save the VM memory state calling get_bs_snapshots().
>>
>> static BlockDriverState *get_bs_snapshots(void)
>> {
>>     BlockDriverState *bs;
>>     DriveInfo *dinfo;
>>
>>     if (bs_snapshots)
>>         return bs_snapshots;
>>     QTAILQ_FOREACH(dinfo, &drives, next) {
>>         bs = dinfo->bdrv;
>>         if (bdrv_can_snapshot(bs))
>>             goto ok;
>>     }
>>     return NULL;
>>  ok:
>>     bs_snapshots = bs;
>>     return bs;
>> }
>>
>> bdrv_can_snapshot() may return a BlockDriverState that does not support
>> snapshots and do_savevm() goes on.
>>
>> Later on in do_savevm(), we find:
>>
>>     QTAILQ_FOREACH(dinfo, &drives, next) {
>>         bs1 = dinfo->bdrv;
>>         if (bdrv_has_snapshot(bs1)) {
>>             /* Write VM state size only to the image that contains the state 
>> */
>>             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>>             ret = bdrv_snapshot_create(bs1, sn);
>>             if (ret < 0) {
>>                 monitor_printf(mon, "Error while creating snapshot on 
>> '%s'\n",
>>                                bdrv_get_device_name(bs1));
>>             }
>>         }
>>     }
>>
>> bdrv_has_snapshot(bs1) is not checking if the device does support or has
>> snapshots as explained above. Only in bdrv_snapshot_create() the device is
>> actually checked for snapshot support.
>>
>> So, in cases where the first device supports snapshots, and the second does 
>> not,
>> the snapshot on the first will happen anyways. I believe this is not a good
>> behavior. It should be an all or nothing process.
>>
>> This patch addresses these issues by making bdrv_can_snapshot() actually do
>> what it must do and enforces better tests to avoid errors in the middle of
>> do_savevm(). bdrv_has_snapshot() is removed and replaced by 
>> bdrv_can_snapshot()
>> where appropriate.
>>
>> bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense 
>> to me.
>>
>> The loadvm_state() function was updated too to enforce that when loading a 
>> VM at
>> least all writable devices must support snapshots too.
>>
>> Signed-off-by: Miguel Di Ciurcio Filho <address@hidden>
>> ---
>>  block.c  |   11 +++++++++++
>>  block.h  |    1 +
>>  savevm.c |   58 ++++++++++++++++++++++++++++++++++++----------------------
>>  3 files changed, 48 insertions(+), 22 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index cd70730..ace3cdb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1720,6 +1720,17 @@ void bdrv_debug_event(BlockDriverState *bs, 
>> BlkDebugEvent event)
>>  /**************************************************************/
>>  /* handling of snapshots */
>>  
>> +int bdrv_can_snapshot(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
>> +        bdrv_is_read_only(bs)) {
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>> +}
>> +
> 
> The underlying protocol could support snapshots, so I think we should
> check against bs->file too.
> 
> --- a/block.c
> +++ b/block.c
> @@ -1671,6 +1671,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>      BlockDriver *drv = bs->drv;
>      if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
>          bdrv_is_read_only(bs)) {
> +        if (bs->file) {
> +            return bdrv_can_snapshot(bs->file);
> +        }
>          return 0;
>      }

You're right that we need to consider protocols, but I think your fix
isn't completely correct either. We should check bs->file only if
!drv->bdrv_snapshot_create, the other options still mean that we can't
snapshot.

Miguel, looks like this needs a v5...

Kevin