Re: [Qemu-devel] [PATCH v2 10/11] block: add option 'backing' to -drive options

[Paolo Bonzini]

Il 17/07/2013 17:09, Kevin Wolf ha scritto:
> Am 17.07.2013 um 16:16 hat Paolo Bonzini geschrieben:
>> Il 17/07/2013 15:48, Kevin Wolf ha scritto:
>>>> I understand this is the right thing to do long term, but pre-opening of
>>>> the target is not really needed for fleecing.
>>>
>>> So for how much longer should we plan to procrastinate? (I know, not an
>>> entirely fair question, but we have to make the step at some point)
>>
>> If we bring it up during soft freeze, we will procrastinate it a lot. :)
>>  If we bring it up at the beginning of a release cycle, the wait could
>> be as short as 1 month...
>>
>>> And we'll want to reference existing BDSes as backing/protocol files in
>>> blockdev-add soon anyway, so if we decide against it here, it's just
>>> moving from Fam's to-do list to mine...
>>
>> We can reconsider these very patches in 1 month.  It's just the timing,
>> combined with the fact that this is not necessary for fleecing, that I'm
>> uncomfortable with.
> 
> Okay, I see where this is going. Let me reinforce one fundamental policy
> that you may not like.

No, I like it actually.  In no way I want to force fleecing in 1.6.  And
this is by no means about downstream schedules, I don't care about them
(not even sure which downstream you're referring to :)).

However, this API seems (a) overengineered for the purposes of fleecing;
(b) ignoring what Ian is doing to support sync modes in block/backup.c.

>>> I guess we can give a name to the target, and we can make drive-backup
>>> automatically connect the target with the original as its backing file
>>> (still needs the refcounting, by the way).
>>
>> No, it doesn't need the refcounting (see my reply to the cover letter).
>>  In his next submission of drive-backup sync modes, Ian is already going
>> to handle the automatic connection of the target with the original.
> 
> Okay, I'll have a look, but I can't imagine how it work without
> refcounting.  As soon as it has a name, you can attach the target to a
> guest, nbd server, start block jobs and do all kinds of fun with it so
> that taking it away when the source goes away becomes problematic.

- Attach the target to a guest: yes, though I wouldn't do it because the
backup job can bdrv_close it under the guest's feet.  Deletion would be
protected by DriveInfo's refcount.

- NBD server: handles closing just fine

- start block jobs: job will be cancelled as soon as the backup job
bdrv_close's the target

- all kinds of fun with it so that taking it away when the source goes
away becomes problematic: taking away the source takes away the target
too, so it is in the same ballpark as attaching the target to a guest
(i.e. be prepared to have I/O that starts to fail, and remember to use
drive_get/put_ref).

>> Yes, but this time we have to bite the bullet on that one at least,
>> because we have no other choice (we want to do at least one
>> "interesting" thing, namely connect to it with the NBD server).
> 
> Yes, like I said, we might not feel comfortable with enabling these
> cases, but not enabling them isn't an option either. So now is the time
> to do the real thing.

I agree and understand.  At the same time, we already have an (ugly)
mechanism for "soft" reference counts, namely DriveInfo.  So I don't
care about the order between "move refcnt from DriveInfo to BDS" and
"support fleecing with the backup job", but they should be two
completely separate series.  Also, the right API shouldn't be influenced
by which part goes in first.

Paolo