Re: [PATCH v8 4/7] copy-on-read: pass base file name to COR driver
04.09.2020 15:17, Max Reitz wrote:
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.
Agree, it should be node-name. Filename-based interfaces is a headache.
driver. The rest of the functionality will be implemented in the patch
that follows.
Signed-off-by: Andrey Shinkevich
---
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
--
Best regards,
Vladimir
Re: [PATCH v8 4/7] copy-on-read: pass base file name to COR driver
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
> ---
> 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
[PATCH v8 4/7] copy-on-read: pass base file name to COR driver
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
driver. The rest of the functionality will be implemented in the patch
that follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 41 -
block/copy-on-read.h | 1 +
2 files changed, 41 insertions(+), 1 deletion(-)
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);
+
+return base_bs;
+}
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BlockDriverState *base_bs = NULL;
BDRVStateCOR *state = bs->opaque;
+const char *base_name = qdict_get_try_str(options, "base");
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +78,15 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+if (base_name) {
+qdict_del(options, "base");
+base_bs = get_base_by_name(bs, base_name, errp);
+if (!base_bs) {
+return -EINVAL;
+}
+}
state->active = true;
+state->base_bs = base_bs;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
@@ -190,6 +224,7 @@ static void bdrv_copy_on_read_init(void)
static BlockDriverState *create_filter_node(BlockDriverState *bs,
+const char *base_name,
const char *filter_node_name,
Error **errp)
{
@@ -197,6 +232,9 @@ static BlockDriverState
*create_filter_node(BlockDriverState *bs,
qdict_put_str(opts, "driver", "copy-on-read");
qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+if (base_name) {
+qdict_put_str(opts, "base", base_name);
+}
if (filter_node_name) {
qdict_put_str(opts, "node-name", filter_node_name);
}
@@ -206,13 +244,14 @@ static BlockDriverState
*create_filter_node(BlockDriverState *bs,
BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *base_name,
const char *filter_node_name,
Error **errp)
{
BlockDriverState *cor_filter_bs;
Error *local_err = NULL;
-cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+cor_filter_bs = create_filter_node(bs, base_name, filter_node_name, errp);
if (cor_filter_bs == NULL) {
error_prepend(errp, "Could not create filter node: ");
return NULL;
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
index 1686e4e..6a7c8bb 100644
--- a/block/copy-on-read.h
+++ b/block/copy-on-read.h
@@ -28,6 +28,7 @@
#include "block/block_int.h"
BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *base_name,
const char *filter_node_name,
Error **errp);
void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
--
1.8.3.1
