Re: [Qemu-devel] [PATCH 10/18] block/mirror: Make source the file child

[Max Reitz]

On 2017-10-10 11:47, Kevin Wolf wrote:
> Am 13.09.2017 um 20:19 hat Max Reitz geschrieben:
>> Regarding the source BDS, the mirror BDS is arguably a filter node.
>> Therefore, the source BDS should be its "file" child.
>>
>> Signed-off-by: Max Reitz 
> 
> TODO: Justification why this doesn't break things like
> bdrv_is_allocated_above() that iterate through the backing chain.

Er, well, yes.

>>  block/mirror.c | 127 
>> ++---
>>  block/qapi.c   |  25 ++---
>>  tests/qemu-iotests/141.out |   4 +-
>>  3 files changed, 119 insertions(+), 37 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 7fa2437923..ee792d0cbc 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend 
>> *blk,
>>  
>>  /* Skip automatically inserted nodes that the user isn't aware of 
>> for
>>   * query-block (blk != NULL), but not for query-named-block-nodes */
>> -while (blk && bs0->drv && bs0->implicit) {
>> -bs0 = backing_bs(bs0);
>> -assert(bs0);
>> +while (blk && bs0 && bs0->drv && bs0->implicit) {
>> +if (bs0->backing) {
>> +bs0 = backing_bs(bs0);
>> +} else {
>> +assert(bs0->file);
>> +bs0 = bs0->file->bs;
>> +}
>>  }
>>  }
> 
> Maybe backing_bs() should skip filters? If so, need to show that all
> existing users of backing_bs() really want to skip filters. If not,
> explain why the missing backing_bs() callers don't need it (I'm quite
> sure that some do).

Arguably any BDS is a filter BDS regarding its backing BDS.  So maybe I
could add a new function filter_child_bs().

> Also, if we attack this at the backing_bs() level, we need to audit
> code that it doesn't directly use bs->backing.

Yup.

Maybe the best idea is to leave this patch for a follow-up...

>> @@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver 
>> = {
>>  .drain  = mirror_drain,
>>  };
>>  
>> +static void source_child_inherit_fmt_options(int *child_flags,
>> + QDict *child_options,
>> + int parent_flags,
>> + QDict *parent_options)
>> +{
>> +child_backing.inherit_options(child_flags, child_options,
>> +  parent_flags, parent_options);
>> +}
>> +
>> +static char *source_child_get_parent_desc(BdrvChild *c)
>> +{
>> +return child_backing.get_parent_desc(c);
>> +}
>> +
>> +static void source_child_cb_drained_begin(BdrvChild *c)
>> +{
>> +BlockDriverState *bs = c->opaque;
>> +MirrorBDSOpaque *s = bs->opaque;
>> +
>> +if (s && s->job) {
>> +block_job_drained_begin(>job->common);
>> +}
>> +bdrv_drained_begin(bs);
>> +}
>> +
>> +static void source_child_cb_drained_end(BdrvChild *c)
>> +{
>> +BlockDriverState *bs = c->opaque;
>> +MirrorBDSOpaque *s = bs->opaque;
>> +
>> +if (s && s->job) {
>> +block_job_drained_end(>job->common);
>> +}
>> +bdrv_drained_end(bs);
>> +}
>> +
>> +static BdrvChildRole source_child_role = {
>> +.inherit_options= source_child_inherit_fmt_options,
>> +.get_parent_desc= source_child_get_parent_desc,
>> +.drained_begin  = source_child_cb_drained_begin,
>> +.drained_end= source_child_cb_drained_end,
>> +};
> 
> Wouldn't it make much more sense to use a standard child role and just
> implement BlockDriver callbacks for .bdrv_drained_begin/end? It seems
> that master still only has .bdrv_co_drain (which is begin), but one of
> Manos' pending series adds the missing end callback.

OK then. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 10/18] block/mirror: Make source the file child

[Kevin Wolf]

Am 13.09.2017 um 20:19 hat Max Reitz geschrieben:
> Regarding the source BDS, the mirror BDS is arguably a filter node.
> Therefore, the source BDS should be its "file" child.
> 
> Signed-off-by: Max Reitz 

TODO: Justification why this doesn't break things like
bdrv_is_allocated_above() that iterate through the backing chain.

>  block/mirror.c | 127 
> ++---
>  block/qapi.c   |  25 ++---
>  tests/qemu-iotests/141.out |   4 +-
>  3 files changed, 119 insertions(+), 37 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 7fa2437923..ee792d0cbc 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend 
> *blk,
>  
>  /* Skip automatically inserted nodes that the user isn't aware of for
>   * query-block (blk != NULL), but not for query-named-block-nodes */
> -while (blk && bs0->drv && bs0->implicit) {
> -bs0 = backing_bs(bs0);
> -assert(bs0);
> +while (blk && bs0 && bs0->drv && bs0->implicit) {
> +if (bs0->backing) {
> +bs0 = backing_bs(bs0);
> +} else {
> +assert(bs0->file);
> +bs0 = bs0->file->bs;
> +}
>  }
>  }

Maybe backing_bs() should skip filters? If so, need to show that all
existing users of backing_bs() really want to skip filters. If not,
explain why the missing backing_bs() callers don't need it (I'm quite
sure that some do).

Also, if we attack this at the backing_bs() level, we need to audit
code that it doesn't directly use bs->backing.

> @@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver 
> = {
>  .drain  = mirror_drain,
>  };
>  
> +static void source_child_inherit_fmt_options(int *child_flags,
> + QDict *child_options,
> + int parent_flags,
> + QDict *parent_options)
> +{
> +child_backing.inherit_options(child_flags, child_options,
> +  parent_flags, parent_options);
> +}
> +
> +static char *source_child_get_parent_desc(BdrvChild *c)
> +{
> +return child_backing.get_parent_desc(c);
> +}
> +
> +static void source_child_cb_drained_begin(BdrvChild *c)
> +{
> +BlockDriverState *bs = c->opaque;
> +MirrorBDSOpaque *s = bs->opaque;
> +
> +if (s && s->job) {
> +block_job_drained_begin(>job->common);
> +}
> +bdrv_drained_begin(bs);
> +}
> +
> +static void source_child_cb_drained_end(BdrvChild *c)
> +{
> +BlockDriverState *bs = c->opaque;
> +MirrorBDSOpaque *s = bs->opaque;
> +
> +if (s && s->job) {
> +block_job_drained_end(>job->common);
> +}
> +bdrv_drained_end(bs);
> +}
> +
> +static BdrvChildRole source_child_role = {
> +.inherit_options= source_child_inherit_fmt_options,
> +.get_parent_desc= source_child_get_parent_desc,
> +.drained_begin  = source_child_cb_drained_begin,
> +.drained_end= source_child_cb_drained_end,
> +};

Wouldn't it make much more sense to use a standard child role and just
implement BlockDriver callbacks for .bdrv_drained_begin/end? It seems
that master still only has .bdrv_co_drain (which is begin), but one of
Manos' pending series adds the missing end callback.

Kevin

[Qemu-devel] [PATCH 10/18] block/mirror: Make source the file child

[Max Reitz]

Regarding the source BDS, the mirror BDS is arguably a filter node.
Therefore, the source BDS should be its "file" child.

Signed-off-by: Max Reitz 
---
 block/mirror.c | 127 ++---
 block/qapi.c   |  25 ++---
 tests/qemu-iotests/141.out |   4 +-
 3 files changed, 119 insertions(+), 37 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9df4157511..05410c94ca 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -77,8 +77,16 @@ typedef struct MirrorBlockJob {
 int target_cluster_size;
 int max_iov;
 bool initial_zeroing_ongoing;
+
+/* Signals that we are no longer accessing source and target and the mirror
+ * BDS should thus relinquish all permissions */
+bool exiting;
 } MirrorBlockJob;
 
+typedef struct MirrorBDSOpaque {
+MirrorBlockJob *job;
+} MirrorBDSOpaque;
+
 struct MirrorOp {
 MirrorBlockJob *s;
 QEMUIOVector qiov;
@@ -595,12 +603,15 @@ static void mirror_exit(BlockJob *job, void *opaque)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 MirrorExitData *data = opaque;
+MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
 AioContext *replace_aio_context = NULL;
 BlockDriverState *src = s->source->bs;
 BlockDriverState *target_bs = blk_bs(s->target);
 BlockDriverState *mirror_top_bs = s->mirror_top_bs;
 Error *local_err = NULL;
 
+s->exiting = true;
+
 bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
 /* Make sure that the source BDS doesn't go away before we called
@@ -622,7 +633,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
 /* We don't access the source any more. Dropping any WRITE/RESIZE is
  * required before it could become a backing file of target_bs. */
-bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
+bdrv_child_try_set_perm(mirror_top_bs->file, 0, BLK_PERM_ALL,
 _abort);
 if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
 BlockDriverState *backing = s->is_none_mode ? src : s->base;
@@ -673,12 +684,11 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
 /* Remove the mirror filter driver from the graph. Before this, get rid of
  * the blockers on the intermediate nodes so that the resulting state is
- * valid. Also give up permissions on mirror_top_bs->backing, which might
+ * valid. Also give up permissions on mirror_top_bs->file, which might
  * block the removal. */
 block_job_remove_all_bdrv(job);
-bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-_abort);
-bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), _abort);
+bdrv_child_try_set_perm(mirror_top_bs->file, 0, BLK_PERM_ALL, 
_abort);
+bdrv_replace_node(mirror_top_bs, mirror_top_bs->file->bs, _abort);
 
 /* We just changed the BDS the job BB refers to (with either or both of the
  * bdrv_replace_node() calls), so switch the BB back so the cleanup does
@@ -687,6 +697,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
 blk_set_perm(job->blk, 0, BLK_PERM_ALL, _abort);
 blk_insert_bs(job->blk, mirror_top_bs, _abort);
 
+bs_opaque->job = NULL;
 block_job_completed(>common, data->ret);
 
 g_free(data);
@@ -1102,7 +1113,7 @@ static void mirror_drain(BlockJob *job)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
-/* Need to keep a reference in case blk_drain triggers execution
+/* Need to keep a reference in case bdrv_drain triggers execution
  * of mirror_complete...
  */
 if (s->target) {
@@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver = {
 .drain  = mirror_drain,
 };
 
+static void source_child_inherit_fmt_options(int *child_flags,
+ QDict *child_options,
+ int parent_flags,
+ QDict *parent_options)
+{
+child_backing.inherit_options(child_flags, child_options,
+  parent_flags, parent_options);
+}
+
+static char *source_child_get_parent_desc(BdrvChild *c)
+{
+return child_backing.get_parent_desc(c);
+}
+
+static void source_child_cb_drained_begin(BdrvChild *c)
+{
+BlockDriverState *bs = c->opaque;
+MirrorBDSOpaque *s = bs->opaque;
+
+if (s && s->job) {
+block_job_drained_begin(>job->common);
+}
+bdrv_drained_begin(bs);
+}
+
+static void source_child_cb_drained_end(BdrvChild *c)
+{
+BlockDriverState *bs = c->opaque;
+MirrorBDSOpaque *s = bs->opaque;
+
+if (s && s->job) {
+block_job_drained_end(>job->common);
+}
+bdrv_drained_end(bs);
+}
+
+static BdrvChildRole source_child_role = {
+.inherit_options= source_child_inherit_fmt_options,
+.get_parent_desc= source_child_get_parent_desc,
+