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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 22.09.2014 um 18:34 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
>> >> 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?
>
> We already agreed that we'll have multiple parents for a BDS, for
> scenarios like having an NBD server on a snapshot or sharing backing
> files, potentially also some block jobs.

We certainly want to provide for multiple "users" (intentionally vague
language here), such NBD server, block jobs, device models.  Should they
share a BB, or does each one need its own BB?

> The question is whether among these multiple parents we want to have a
> limitation to one BlockBackend, forbidding e.g. an NBD server on the
> active layer. This would be a problem for live storage migration if we
> don't want the NBD server to reuse the same BB as the guest device.
>
> More generally, if we can indirectly have multiple BBs on a single
> BDS by putting a filter in between, do we have good reasons to forbid
> having them attached directly?

Keeping code simple?

Not a valid argument when we *need* multiple BBs, i.e. when the answer
to my prior question is "each one needs its own BB".

>> > (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.
>
> Yes, my "now" depends on context and either refers to the patch I'm
> commenting on or the end of the series. In most cases when I see
> something that I feel is worth having a closer look, the first thing I
> do is looking at the fully applied series.
>
>> 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)
>> +       }
>
> Yes, as I said, this is fine for now. When we allow multiple BBs, we'll
> have to turn it into something like notifier lists, but that can wait.

Okay.

>> 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?
>
> My first attempt would be "any", and in cases where this isn't good
> enough, you can't use a fallback at all.

This is going to be fun :)

>> > 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.
>
> Are you sure that nothing else than device models can be interested in
> callbacks? I expect that whatever block layer user we have, they will
> always be interested in resizes, for example. Media change might also
> not be entirely uninteresting, though in most cases what other users
> want is probably a blocker.

I designed BlockDevOps for device models only.  If other users emerge,
it needs a rename, and possibly a rethink.