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

[Kevin Wolf]

Am 19.10.2015 um 16:15 hat Alberto Garcia geschrieben:
> 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?

For having multiple users of the BDS, e.g. a guest device and an NBD
server.

Or sometimes maybe just because it's the logical thing to happen:
Imagine an image B with a backing file A. Both have a BlockBackend on
them. Do a live commit of B into A. On completion, the BDS B is deleted
and both BBs point to A.

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

Sorry for not having read the patches yet. I tried to figure out what
the right thing to do is before I could start reviewing whether the
patches do the thing right.

I agree with your solution, which addresses my question.

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

Okay, so is the following your generalised rule?

If device refers to a BDS without a BB or to a BB without a BDS, then
just delete it. Otherwise, try to treat one BDS with one BB as a unit
and remove that unit. They can only be treated as a unit if the BDS is
only referenced by the BB (i.e. not by the monitor either). If they
can't be treated as a unit, error out.

I think that's safe and allows using the usual aliasing (using the BB
name or the node-name of its BDS means the same thing), which is good
for consistency.

My only concern with this is whether it isn't too clever. Management
tools need to be prepared to handle BDSes with more than one reference
differently from BDSes only referenced by a (single) BB. The common case
is the simple one where removing BB and BDS at once works; so it may be
an invitation to ignore the other case, which would then surprisingly
fail.

On the other hand, always requiring an eject before blockdev-del would
break the symmetry of blockdev-add and blockdev-del and isn't very nice
either.

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

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.

Kevin