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

[Alberto Garcia]

On Mon 19 Oct 2015 01:27:45 PM CEST, Kevin Wolf wrote:
> I've been thinking a bit about the creation and deletion of
> BlockBackends a bit last week, and honestly it still feels a bit messy
> (or maybe I just don't fully understand it yet). Anyway, the cover
> letter of this series seems to be a good place for trying to write it
> down.

Thanks for the summary!

> So we seem to have two criteria to distinguish BDSes:
>
> 1. Does/Doesn't have a BlockBackend on top.
>    In the future, multiple BlockBackends are possible, too.

One single BDS attached to multiple BlockBackends at the same time?
What's the use case?

> 1. BB without BDS
> 2. BB attached to BDS without monitor reference
> 3. BB attached to BDS with monitor reference
> 4. BDS without BB and without monitor reference
> 5. BDS without BB and with monitor reference

I would say that the rule that we should follow is: if the outcome is
not clear for a particular case, then the command fails. We should try
not to guess what the user wants. blockdev-del should only delete
something when there's only one sane alternative.

> Let me start with the first two questions for all cases:
>
> 1. As far as I know, it's currently not possible to directly create a BB
>    without a BDS (or before Max' series with an empty BDS). You have to
>    create a BB with an opened BDS and then eject that. Oops.
>
>    blockdev-del is easy: It deletes the BB and nothing else.

I agree.

> 2. This is the normal case, easy to produce with blockdev-add.
>    The two options for deleting something are removing the BDS while
>    keeping the BB (this is already covered by 'eject') and dropping the
>    BB (probably makes sense to use 'blockdev-del' here). There is no
>    monitor reference to the BDS anyway, so blockdev-dev can't delete
>    that.
>
>    Dropping the BB automatically also drops the BDS in simple cases. In
>    more complicated cases where the BDS has got more references later,
>    it might still exist. Then the blockdev-del wasn't the exact opposite
>    operation of blockdev-add.
>
>    Is this a problem for a user/management tool that wants to keep track
>    of which image files are still in use?

I would say that if the BDS has additional references then
'blockdev-del' should fail and do nothing (what the current patch does).

> 3. A BDS with a monitor reference can be created (with some patches) by
>    using blockdev-add with a node-name, but no id. Directly attaching it
>    to a BB is tricky today, even though I'm sure you could do it with
>    some clever use of block jobs... With 1. fixed (i.e. you can create
>    an empty BB), insert-medium should do the job.
>
>    Here we have even more choices for deleting something: Delete the BB,
>    but leave the BDS alone; delete the BDS and leave an empty BB behind
>    (this is different from eject because eject would drop the connection
>    between BB and BDS, but both would continue to exist!); delete both
>    at once.
>
>    We had two separate blockdev-add commands for the BDS and the BB, so
>    it makes sense to require two separate blockdev-del commands as well.
>    In order to keep blockdev-add/del symmetrical, we would have to make
>    blockdev-del of a node-name delete the BDS and blockdev-del of an id
>    delete the BB.

This is a tricky case, thanks for pointing it out. I would also go for
the conservative option here: if you create a BDS with blockdev-add and
then attach it to a BlockBackend, you're adding one additional reference
(via blk_insert_bs()). Therefore blockdev-del would fail like in case 2.

You need to eject the BDS first in order to decide which one you want to
delete.

> 4. That's the typical bs->file. Created by nested blockdev-add
>    descriptions. blockdev-del errors out, there is no monitor reference
>    to drop, and there is also no BB that the request could be used
>    for.

Correct.

> 5. Created by a top-level blockdev-add with node-name and no id (like 3.
>    but without attaching it to a BB afterwards). blockdev-del can only
>    mean deleting the BDS.

Correct.

> 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.

Berto