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.shinkev...@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
signature.asc
Description: OpenPGP digital signature
