Re: [PATCH v4 12/18] block: copy-before-write: realize snapshot-access API

[Hanna Reitz <hreitz@redhat.com>]

On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote:
> Current scheme of image fleecing looks like this:
>
> [guest]                    [NBD export]
>    |                              |
>    |root                          | root
>    v                              v
> [copy-before-write] -----> [temp.qcow2]
>    |                 target  |
>    |file                     |backing
>    v                         |
> [active disk] <-------------+
>
>   - On guest writes copy-before-write filter copies old data from active
>     disk to temp.qcow2. So fleecing client (NBD export) when reads
>     changed regions from temp.qcow2 image and unchanged from active disk
>     through backing link.
>
> This patch makes possible new image fleecing scheme:
>
> [guest]                   [NBD export]
>     |                            |
>     | root                       | root
>     v                 file       v
> [copy-before-write]<------[x-snapshot-access]
>     |           |
>     | file      | target
>     v           v
> [active-disk] [temp.img]
>
>   - copy-before-write does CBW operations and also provides
>     snapshot-access API. The API may be accessed through
>     x-snapshot-access driver.

The “x-” prefix seems like a relic from an earlier version.

(I agree with what I assume is your opinion now, that we don’t need an 
x- prefix.  I can’t imagine why we’d need to change the snapshot-access 
interface in an incompatible way.)

> Benefits of new scheme:
>
> 1. Access control: if remote client try to read data that not covered
>     by original dirty bitmap used on copy-before-write open, client gets
>     -EACCES.
>
> 2. Discard support: if remote client do DISCARD, this additionally to
>     discarding data in temp.img informs block-copy process to not copy
>     these clusters. Next read from discarded area will return -EACCES.
>     This is significant thing: when fleecing user reads data that was
>     not yet copied to temp.img, we can avoid copying it on further guest
>     write.
>
> 3. Synchronisation between client reads and block-copy write is more
>     efficient. In old scheme we just rely on BDRV_REQ_SERIALISING flag
>     used for writes to temp.qcow2. New scheme is less blocking:
>       - fleecing reads are never blocked: if data region is untouched or
>         in-flight, we just read from active-disk, otherwise we read from
>         temp.img
>       - writes to temp.img are not blocked by fleecing reads
>       - still, guest writes of-course are blocked by in-flight fleecing
>         reads, that currently read from active-disk - it's the minimum
>         necessary blocking
>
> 4. Temporary image may be of any format, as we don't rely on backing
>     feature.
>
> 5. Permission relation are simplified. With old scheme we have to share
>     write permission on target child of copy-before-write, otherwise
>     backing link conflicts with copy-before-write file child write
>     permissions. With new scheme we don't have backing link, and
>     copy-before-write node may have unshared access to temporary node.
>     (Not realized in this commit, will be in future).
>
> 6. Having control on fleecing reads we'll be able to implement
>     alternative behavior on failed copy-before-write operations.
>     Currently we just break guest request (that's a historical behavior
>     of backup). But in some scenarios it's a bad behavior: better
>     is to drop the backup as failed but don't break guest request.
>     With new scheme we can simply unset some bits in a bitmap on CBW
>     failure and further fleecing reads will -EACCES, or something like
>     this. (Not implemented in this commit, will be in future)
>     Additional application for this is implementing timeout for CBW
>     operations.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/copy-before-write.c | 212 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 211 insertions(+), 1 deletion(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 91a2288b66..a8c88f64eb 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c

[...]

> +static int coroutine_fn
> +cbw_co_snapshot_block_status(BlockDriverState *bs,
> +                             bool want_zero, int64_t offset, int64_t bytes,
> +                             int64_t *pnum, int64_t *map,
> +                             BlockDriverState **file)
> +{
> +    BDRVCopyBeforeWriteState *s = bs->opaque;
> +    BlockReq *req;
> +    int ret;
> +    int64_t cur_bytes;
> +    BdrvChild *child;
> +
> +    req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &child);
> +    if (!req) {
> +        return -EACCES;
> +    }
> +
> +    ret = bdrv_block_status(bs, offset, cur_bytes, pnum, map, file);

This looks like an infinite recursion.  Shouldn’t this be s/bs/child->bs/?

> +    if (child == s->target) {
> +        /*
> +         * We refer to s->target only for areas that we've written to it.
> +         * And we can not report unallocated blocks in s->target: this will
> +         * break generic block-status-above logic, that will go to
> +         * copy-before-write filtered child in this case.
> +         */
> +        assert(ret & BDRV_BLOCK_ALLOCATED);
> +    }
> +
> +    cbw_snapshot_read_unlock(bs, req);
> +
> +    return ret;
> +}

[...]

> @@ -225,6 +407,27 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>           return -EINVAL;
>       }
>   
> +    cluster_size = block_copy_cluster_size(s->bcs);
> +
> +    s->done_bitmap = bdrv_create_dirty_bitmap(bs, cluster_size, NULL, errp);
> +    if (!s->done_bitmap) {
> +        return -EINVAL;

Hmm, similarly to my question on patch 4, I assume cbw_close() will free 
s->bcs (and also s->done_bitmap in the error case below)?

> +    }
> +    bdrv_disable_dirty_bitmap(s->done_bitmap);
> +
> +    /* s->access_bitmap starts equal to bcs bitmap */
> +    s->access_bitmap = bdrv_create_dirty_bitmap(bs, cluster_size, NULL, errp);
> +    if (!s->access_bitmap) {
> +        return -EINVAL;
> +    }
> +    bdrv_disable_dirty_bitmap(s->access_bitmap);
> +    bdrv_dirty_bitmap_merge_internal(s->access_bitmap,
> +                                     block_copy_dirty_bitmap(s->bcs), NULL,
> +                                     true);
> +
> +    qemu_co_mutex_init(&s->lock);
> +    QLIST_INIT(&s->frozen_read_reqs);
> +
>       return 0;
>   }