Re: [Qemu-devel] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

On Thu 22 Oct 2015 12:38:24 PM CEST, Kevin Wolf <address@hidden> wrote:
>> >> > I haven't thought about it enough yet, but it seems to me that we
>> >> > can't do the BDS/BB aliasing with blockdev-del, but need to interpret
>> >> > node-name as BDS and id as BB. Perhaps we also shouldn't use a single
>> >> > 'device' option then, but a union of 'node-name' and 'id' (just like
>> >> > the same devices were created in blockdev-add).
>> >> 
>> >> Having two separate options could make things more clear, indeed.
>> >
>> > Note that it doesn't improve things with your generalised rule (if I got
>> > it right anyway).
>> >
>> > So I think these are the options we have:
>> >
>> > a) blockdev-del only works on a BDS or empty BB without any references.
>> >    Before deleting a BDS, it must be ejected from the BB; before
>> >    deleting a BB, its BDS must be ejected.
>> >
>> > b) Your rule: Same as a), but treating BB and BDS as a unit if the BDS
>> >    is only referenced by the BB.
>> >
>> > c) No aliasing. You explicitly refer to a BB or to a BDS. The difference
>> >    from A is that it works without an explicit eject: When a BDS is
>> >    deleted, it is automatically ejected from the BB; and when the BB is
>> >    deleted, the BDS stays around if it still has other references.
>> 
>> I'm currently thinking about d), which tries to maintain the symmetry
>> with blockdev-add:
>> 
>> - blockdev-del would have two parameters, 'id' and 'node-name', and only
>>   one of them can be set, so you must choose whether you want to delete
>>   a backend or a BDS.
>> 
>> - blockdev-add can either create a backend with a BDS, or a BDS alone,
>>   so:
>> 
>>   - If you created a backend and you try to delete it you can do it
>>     (along with its BDS) as long as neither the backend nor the BDS are
>>     being used (no extra references, no parents). This means that the
>>     operation will fail if there's a BDS that has been created
>>     separately and manually attached to the the backend.
>> 
>>   - If you created a BDS alone and you try to delete it you can do it as
>>     long as no one else is using it. This would delete the BDS and only
>>     the BDS (because that's what you create with blockdev-add). If it's
>>     currently attached to a backend then the operation fails.
>
> So this is essentially c) with the modification that no implicit eject
> happens. Either both BB and BDS go away because the BDS is only
> referenced by the BB or you get an error.

Right, I would say you always get an error.

I'm currently extending the set of tests (I expect to send the updated
series later today or tomorrow) and most are quite straightforward and
hopefully helpful to prevent surprises in the future. It's also an
interesting exercise to test the BlockBackend series by Max.

But there's this case that is not so obvious. It involves the new
'blockdev-snapshot' command I'm working on:

  - blockdev-add id=drive0 node-name=node0 file=hd0.qcow2
  - qemu-img create -f qcow2 -b hd0.qcow2 overlay0.qcow2
  - blockdev-add node-name=overlay0 file=overlay0.qcow2
  - blockdev-snapshot node=hd0 overlay=overlay0

At this point you have drive0 with overlay0 inserted, and hd0 as its
backing image. All these operation will fail:

  - blockdev-del id=drive0         because overlay0 has two references
                                   (monitor and block backend)
  - blockdev-del node=overlay0     for the same reason
  - blockdev-del node=hd0          because it's a backing image

In order to delete all this you need to:

  - eject device=drive0            overlay0 has one reference left
  - blockdev-del id=drive0
  - blockdev-del node=overlay0     this deletes hd0 as well

Does this make sense, or do we need to rethink the semantics a bit more?

Berto