Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
>> The pointer from BlockBackend to BlockDriverState is a strong
>> reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is
>> a weak one.
>> 
>> Convenience function blk_new_with_bs() creates a BlockBackend with its
>> BlockDriverState.  Callers have to unref both.  The commit after next
>> will relieve them of the need to unref the BlockDriverState.
>> 
>> Complication: due to the silly way drive_del works, we need a way to
>> hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
>> "special" status, give the function a suitably off-putting name:
>> blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
>> BlockBackend's name into the empty string.  Can't avoid that without
>> breaking the blk->bs->device_name equals blk->name invariant.
>> 
>> The patch adds a memory leak: drive_del while a device model is
>> connected leaks the BlockBackend.  Avoiding the leak here is rather
>> hairy, but it'll become straightforward in a few commits, so I mark it
>> FIXME in the code now, and plug it when it's easy.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> +/*
>> + * Hide @blk.
>> + * @blk must not have been hidden already.
>> + * Make attached BlockDriverState, if any, anonymous.
>> + * Once hidden, @blk is invisible to all functions that don't receive
>> + * it as argument.  For example, blk_by_name() won't return it.
>> + * Strictly for use by do_drive_del().
>> + * TODO get rid of it!
>> + */
>> +void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
>> +{
>> +    QTAILQ_REMOVE(&blk_backends, blk, link);
>> +    blk->name[0] = 0;
>
> Style nit: I prefer '\0' when dealing with strings.

I don't, but if you feel strongly about it, I'll do it your way.

>> +    if (blk->bs) {
>> +        bdrv_make_anon(blk->bs);
>> +    }
>> +}
>> diff --git a/blockdev.c b/blockdev.c
>> index 583235a..5da6028 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -228,6 +228,7 @@ void drive_info_del(DriveInfo *dinfo)
>>      if (dinfo->opts) {
>>          qemu_opts_del(dinfo->opts);
>>      }
>> +
>>      g_free(dinfo->id);
>>      QTAILQ_REMOVE(&drives, dinfo, next);
>>      g_free(dinfo->serial);
>
> This hunk is a rebasing artifact, I guess?

Consider it gone.

>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 8d86a6c..14e0b7c 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -324,6 +324,8 @@ struct BlockDriverState {
>>      BlockDriver *drv; /* NULL means no media */
>>      void *opaque;
>>  
>> +    BlockBackend *blk;          /* owning backend, if any */
>> +
>>      void *dev;                  /* attached device model, if any */
>>      /* TODO change to DeviceState when all users are qdevified */
>>      const BlockDevOps *dev_ops;
>
> Just to make sure that we agree on where we're going: This makes the
> assumption that a BDS has at most one BB that owns it.

Yes.

>                                                        Which is not the
> final state that we want to have, so this will have to go away later.

I don't know.  Can you explain why you think we're going to want
multiple BBs?

> (Where "later" isn't necessarily part of this series.)
>
> For now, the use of the field is limited to callbacks and
> bdrv_get_device_name(). Callbacks could always only serve a single
> device, so nothing became worse here.

In *this* patch, member blk is only read in bdrv_swap(), which asserts
it's null.  Later on in the series, it gets indeed used as you describe.

PATCH 22 puts it to use for BlockDevOps callbacks.  The patch moves the
callbacks from BDS to BB.  I hope you'll agree that's where they belong.

Naturally, the *calls* of the callbacks remain where they are, in
block.c.  They get updated like this:

-       bdrv_dev_FOO(bs, ARGS)
+       if (bs->blk) {
+           blk_dev_FOO(bs->blk ARGS)
+       }

PATCH 08 uses it to eliminate BDS member device_name[].

> I'm not entirely sure about bdrv_get_device_name(), whether it needs to
> go or to be rewritten to get the name of any BB pointing to it (I
> suspect for most callers we want to replace it by something that uses
> node-name by default if there is one and only fall back to BB names if
> there isn't), but that's not an issue to block this patch.

I agree users of bdrv_get_device_name() need to be examined, and the
ones that really want a BDS name should probably be changed to use the
BDS name (a.k.a. node-name) and fall back to the BB name.

This series makes this need more visible, by emphasizing the
distinctness of the two names.

Aside: which one to fall back to if we have multiple BBs?

> What I would consider, however, is adding a TODO comment that tells
> people that this field needs to go and if you need to use it, something
> is wrong with your design (which happens to be true for the existing
> design of some code).

For the device callbacks, we need a way to find the BB.  If multiple BBs
can sit on top of the same BDS, we need to find the one with a device
models attached.  Ot even the ones, if we permit that.

Let's discuss this a bit, and depending on what we learn, add a suitable
comment.  Possibly on top.

> Nothing critical in this patch, so with or without addressing the
> comments:
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!