Kevin Wolf <kw...@redhat.com> writes:
> Am 22.09.2014 um 18:34 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> 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.
