Re: [PATCH v8 5/7] copy-on-read: limit guest writes to base in COR driver

[Max Reitz]

On 28.08.20 18:52, Andrey Shinkevich wrote:
> Limit the guest's COR operations by the base node in the backing chain
> during a stream job.

I don’t understand.   Shouldn’t we limit the areas where we set the COR
flag?

> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  block/copy-on-read.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 1f858bb..ecbd1f8 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -57,6 +57,37 @@ static BlockDriverState *get_base_by_name(BlockDriverState 
> *bs,
>      return base_bs;
>  }
>  
> +/*
> + * Returns 1 if the block is allocated in a node between 
> cor_filter_bs->file->bs
> + * and the base_bs in the backing chain. Otherwise, returns 0.
> + * The COR operation is allowed if the base_bs is not specified - return 1.
> + * Returns negative errno on failure.
> + */
> +static int node_above_base(BlockDriverState *cor_filter_bs, uint64_t offset,
> +                           uint64_t bytes)
> +{
> +    int ret;
> +    int64_t dummy;
> +    BlockDriverState *file = NULL;
> +    BDRVStateCOR *state = cor_filter_bs->opaque;
> +
> +    if (!state->base_bs) {
> +        return 1;
> +    }
> +
> +    ret = bdrv_block_status_above(cor_filter_bs->file->bs, state->base_bs,
> +                                  offset, bytes, &dummy, NULL, &file);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (file) {

Why check file and not the return value?

> +        return 1;
> +    }
> +
> +    return 0;

“dummy” should really not be called that way, it should be evaluated
whether it’s smaller than bytes.

First, [offset, offset + dummy) may not be allocated above the base –
but [offset + dummy, offset + bytes) may be.  Then this function returns
0 here, even though there is something in that range that’s allocated.

Second, in that case we still shouldn’t return 1, but return the
shrunken offset instead.  Or, if there are multiple distinct allocated
areas, they should probably even all be copied separately.


(But all of that of course only if this function is intended to be used
to limit where we set the COR flag, because I don’t understand why we’d
want to limit where something can be written.)

Max

> +}
> +
>  static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>                      Error **errp)
>  {
> @@ -153,6 +184,12 @@ static int coroutine_fn 
> cor_co_pwritev_part(BlockDriverState *bs,
>                                              QEMUIOVector *qiov,
>                                              size_t qiov_offset, int flags)
>  {
> +    int ret = node_above_base(bs, offset, bytes);
> +
> +    if (!ret || ret < 0) {
> +        return ret;
> +    }
> +
>      return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
>                                  flags);
>  }
> @@ -162,6 +199,12 @@ static int coroutine_fn 
> cor_co_pwrite_zeroes(BlockDriverState *bs,
>                                               int64_t offset, int bytes,
>                                               BdrvRequestFlags flags)
>  {
> +    int ret = node_above_base(bs, offset, bytes);
> +
> +    if (!ret || ret < 0) {
> +        return ret;
> +    }
> +
>      return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
>  }
>  
> @@ -178,6 +221,12 @@ static int coroutine_fn 
> cor_co_pwritev_compressed(BlockDriverState *bs,
>                                                    uint64_t bytes,
>                                                    QEMUIOVector *qiov)
>  {
> +    int ret = node_above_base(bs, offset, bytes);
> +
> +    if (!ret || ret < 0) {
> +        return ret;
> +    }
> +
>      return bdrv_co_pwritev(bs->file, offset, bytes, qiov,
>                             BDRV_REQ_WRITE_COMPRESSED);
>  }
>

signature.asc
Description: OpenPGP digital signature