Re: [PATCH v5 11/16] block: introduce snapshot-access block driver

[Hanna Reitz <hreitz@redhat.com>]

On 28.02.22 12:39, Vladimir Sementsov-Ogievskiy wrote:
> The new block driver simply utilizes snapshot-access API of underlying
> block node.
>
> In further patches we want to use it like this:
>
> [guest]                   [NBD export]
>     |                            |
>     | root                       | root
>     v                 file       v
> [copy-before-write]<------[snapshot-access]
>     |           |
>     | file      | target
>     v           v
> [active-disk] [temp.img]
>
> This way, NBD client will be able to read snapshotted state of active
> disk, when active disk is continued to be written by guest. This is
> known as "fleecing", and currently uses another scheme based on qcow2
> temporary image which backing file is active-disk. New scheme comes
> with benefits - see next commit.
>
> The other possible application is exporting internal snapshots of
> qcow2, like this:
>
> [guest]          [NBD export]
>     |                  |
>     | root             | root
>     v       file       v
> [qcow2]<---------[snapshot-access]
>
> For this, we'll need to implement snapshot-access API handlers in
> qcow2 driver, and improve snapshot-access block driver (and API) to
> make it possible to select snapshot by name. Another thing to improve
> is size of snapshot. Now for simplicity we just use size of bs->file,
> which is OK for backup, but for qcow2 snapshots export we'll need to
> imporve snapshot-access API to get size of snapshot.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json    |   4 +-
>   block/snapshot-access.c | 132 ++++++++++++++++++++++++++++++++++++++++
>   MAINTAINERS             |   1 +
>   block/meson.build       |   1 +
>   4 files changed, 137 insertions(+), 1 deletion(-)
>   create mode 100644 block/snapshot-access.c

[...]

> diff --git a/block/snapshot-access.c b/block/snapshot-access.c
> new file mode 100644
> index 0000000000..77b87c1946
> --- /dev/null
> +++ b/block/snapshot-access.c

[...]

> +static int snapshot_access_open(BlockDriverState *bs, QDict *options, int flags,
> +                                Error **errp)
> +{
> +    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
> +                               BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
> +                               false, errp);
> +    if (!bs->file) {
> +        return -EINVAL;
> +    }
> +
> +    bs->total_sectors = bs->file->bs->total_sectors;

(If I hadn’t commented on patch 16, I wouldn’t’ve here, but now I might 
as well...)

Instead of just a comment in the commit message (which noone will really 
read later on), I prefer a TODO or FIXME comment directly here in the 
code, or even better in the API added in the previous patch (i.e. as 
part of the comment in the BlockDriver struct), that this will not work 
for qcow2, i.e. that we will need to inquire the snapshot size from the 
snapshot-providing node.

It’s OK not to implement that now, but I don’t think having a note just 
in the commit message will help us remember.

> +
> +    return 0;
> +}