Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Max Reitz <mreitz@redhat.com>]

On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
> 03.05.2021 12:54, Max Reitz wrote:
>> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
>> either bs->file or bs->backing. 
> 
>> We close that child,
> 
> Do we?

We *detach it.

>> close the node
>> (with .bdrv_close()), apply the snapshot on the child node, and then
>> re-open the node (with .bdrv_open()).
>>
>> In order for .bdrv_open() to attach the same child node that we had
>> before, we pass "file={child-node}" or "backing={child-node}" to it.
>> Therefore, when .bdrv_open() has returned success, we can assume that
>> bs->file or bs->backing (respectively) points to our original child
>> again.  This is verified by an assertion.
>>
>> All of this is not immediately clear from a quick glance at the code,
>> so add a comment to the assertion what it is for, and why it is valid.
>> It certainly confused Coverity.
>>
>> Reported-by: Coverity (CID 1452774)
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/snapshot.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index e8ae9a28c1..cce5e35b35 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>           qobject_unref(file_options);
>>           g_free(subqdict_prefix);
>> +        /* Force .bdrv_open() below to re-attach fallback_bs on 
>> *fallback_ptr */
>>           qdict_put_str(options, (*fallback_ptr)->name,
>>                         bdrv_get_node_name(fallback_bs));
>> +        /* Now close bs, apply the snapshot on fallback_bs, and 
>> re-open bs */
>>           if (drv->bdrv_close) {
>>               drv->bdrv_close(bs);
>>           }
>> +        /* .bdrv_open() will re-attach it */
>>           bdrv_unref_child(bs, *fallback_ptr);
>>           *fallback_ptr = NULL;
>> @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>               return ret < 0 ? ret : open_ret;
>>           }
>> -        assert(fallback_bs == (*fallback_ptr)->bs);
>> +        /*
>> +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
>> +         * was closed above and set to NULL, but the .bdrv_open() call
>> +         * has opened it again, because we set the respective option
>> +         * (with the qdict_put_str() call above).
>> +         * Assert that .bdrv_open() has attached some child on
>> +         * *fallback_ptr, and that it has attached the one we wanted
>> +         * it to (i.e., fallback_bs).
>> +         */
>> +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>>           bdrv_unref(fallback_bs);
>>           return ret;
>>       }
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> ===
> 
> I still think that this fallback has more problems.. Nothing guarantee 
> that all format drivers (even restricted to those who have only one 
> child) support such logic.
> 
> For example, .bdrv_open() may rely on state structure were zeroed and 
> not initialize some fields. And .bdrv_close() may just g_free() some 
> things, not setting them to zero.. So we probably should call 
> bdrv_open()/bdrv_close() generic functions. But we can't: at least 
> bdrv_close() requires that node has no parents.
> 
> Not saying about that we lose everything on failure (when actually, it's 
> better to restore original state on failure).
> 
> This feature should instead be done with help of bdrv_reopen_multiple(), 
> and even with it it's not obvious how to do it properly.
> 
> The feature were done long ago in 2010 with commit 
> 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
> 
> Note also, that the only protocol driver that support snapshots is rbd, 
> and snapshot support was added to it in 2012 with commit 
> bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
> 
> Other two drivers with support are sheepdog (which is deprecated) and 
> qcow2 (I doubt that it should be used as protocol driver for some other 
> format).
> 
> 
> Do we really need this fallback? Is it used somewhere? May be, we can 
> just deprecate it, and look will someone complain?

Maybe? :)

Max