[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 10/18] block/mirror: Make source the file child
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 10/18] block/mirror: Make source the file child |
Date: |
Wed, 11 Oct 2017 14:02:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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 <address@hidden>
>
> 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
signature.asc
Description: OpenPGP digital signature