Re: [PATCH v8 4/7] copy-on-read: pass base file name to COR driver

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2640 bytes --]

On 28.08.20 18:52, Andrey Shinkevich wrote:
> To limit the guest's COR operations by the base node in the backing
> chain during stream job, pass the base file name to the copy-on-read

Does it have to be a filename?  That sounds really bad to me.

> driver. The rest of the functionality will be implemented in the patch
> that follows.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  block/copy-on-read.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  block/copy-on-read.h |  1 +
>  2 files changed, 41 insertions(+), 1 deletion(-)

Furthermore, I believe that this option should become an externally
visible option for the copy-on-read filter (i.e., part of its
BlockdevOptions) – but that definitely won’t be viable if @base contains
a filename.

Can’t we let the stream job invoke bdrv_find_backing_image() to
translate a filename into a node name that’s then passed to the COR filter?

> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 0ede7aa..1f858bb 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -24,19 +24,45 @@
>  #include "block/block_int.h"
>  #include "qemu/module.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qdict.h"
>  #include "block/copy-on-read.h"
>  
>  
>  typedef struct BDRVStateCOR {
>      bool active;
> +    BlockDriverState *base_bs;
>  } BDRVStateCOR;
>  
> +/*
> + * Non-zero pointers are the caller's responsibility.
> + */
> +static BlockDriverState *get_base_by_name(BlockDriverState *bs,
> +                                          const char *base_name, Error **errp)
> +{
> +    BlockDriverState *base_bs = NULL;
> +    AioContext *aio_context;
> +
> +    base_bs = bdrv_find_backing_image(bs, base_name);
> +    if (base_bs == NULL) {
> +        error_setg(errp, QERR_BASE_NOT_FOUND, base_name);
> +        return NULL;
> +    }
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +    assert(bdrv_get_aio_context(base_bs) == aio_context);
> +    aio_context_release(aio_context);

Er.  OK.  But why?  Isn’t this just guaranteed by the block layer?  I
don’t think we need an explicit assertion for this, especially if it
means having to acquire an AioContext.

Furthermore, I don’t even know why we’d need the AioContext.  On one
hand, we don’t need to acquire a context just to get it or compare it;
on the other, this I would have thought that .bdrv_open() runs in the
BDS’s AioContext anyway (or the caller already has it acquired at least).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]