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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

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 <mreitz@redhat.com>
> 
> 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(&s->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(&s->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


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