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

[Hanna Reitz]

On 03.03.22 12:05, Hanna Reitz wrote:
> 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.

Considering softfreeze is next week, I’d propose I just add the 
following the patch 10, would that be OK for you?

(In case it is, I’ll hold off on applying patch 16 for now; it’s a test, 
so we can easily add it during freeze)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index c43315ae6e..5c8ad9ed78 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -385,6 +385,12 @@ struct BlockDriver {
      * in generic block-layer: no serializing, no alignment, no tracked
      * requests. So, block-driver that realizes these APIs is fully 
responsible
      * for synchronization between snapshot-access API and normal IO 
requests.
+     *
+     * TODO: To be able to support qcow2's internal snapshots, this API 
will
+     * need to be extended to:
+     * - be able to select a specific snapshot
+     * - receive the snapshot's actual length (which may differ from bs's
+     *   length)
      */
     int coroutine_fn (*bdrv_co_preadv_snapshot)(BlockDriverState *bs,
         int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t 
qiov_offset);