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

[Alberto Garcia]

On Thu 22 Oct 2015 12:38:24 PM CEST, Kevin Wolf  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=drive0overlay0 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

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

[Kevin Wolf]

Am 22.10.2015 um 13:31 hat Alberto Garcia geschrieben:
> On Thu 22 Oct 2015 01:25:05 PM CEST, Kevin Wolf wrote:
> >> 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=drive0overlay0 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?
> >
> > Well, it's consistent with what you described above.
> >
> > The confusing part might be that you could blockdev-del id=drive0
> > originally, but after taking the snapshot it doesn't work any
> > more. The only way I can see to remove this effect is that you always
> > need to eject the BDS first, even if its only reference is from the BB
> > that is going to be deleted.
> >
> > I guess that would be even clearer rules, but of course it also means
> > that it's a bit more cumbersome to use. If it helps avoiding bugs in
> > management tools, it might be worth it.
> 
> That would be a good reason to force the user to eject the media
> first. Note however that in this case you would still need to delete
> overlay0 manually, as it would still have the monitor reference.

Yes, but I think that's expected because you had a separate blockdev-add.

> However if the snapshot is created using blockdev-snapshot-sync that
> problem does not exist because that extra reference is not there.

Hm... Actually I see a good question here. It's not clear to me that a
BDS created with blockdev-snapshot-sync shouldn't be considered
explicit, especially if a node-name was passed. I guess you can bring up
good arguments for either behaviour.

Kevin

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

[Kevin Wolf]

Am 22.10.2015 um 13:08 hat Alberto Garcia geschrieben:
> >> 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.

Awesome. As you know, I love test cases.

> 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=drive0overlay0 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?

Well, it's consistent with what you described above.

The confusing part might be that you could blockdev-del id=drive0
originally, but after taking the snapshot it doesn't work any more. The
only way I can see to remove this effect is that you always need to
eject the BDS first, even if its only reference is from the BB that is
going to be deleted.

I guess that would be even clearer rules, but of course it also means
that it's a bit more cumbersome to use. If it helps avoiding bugs in
management tools, it might be worth it.

But again, I'd like to hear a libvirt opinion from Eric here.

Kevin

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

[Alberto Garcia]

On Thu 22 Oct 2015 01:25:05 PM CEST, Kevin Wolf wrote:
>> 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=drive0overlay0 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?
>
> Well, it's consistent with what you described above.
>
> The confusing part might be that you could blockdev-del id=drive0
> originally, but after taking the snapshot it doesn't work any
> more. The only way I can see to remove this effect is that you always
> need to eject the BDS first, even if its only reference is from the BB
> that is going to be deleted.
>
> I guess that would be even clearer rules, but of course it also means
> that it's a bit more cumbersome to use. If it helps avoiding bugs in
> management tools, it might be worth it.

That would be a good reason to force the user to eject the media
first. Note however that in this case you would still need to delete
overlay0 manually, as it would still have the monitor reference.

However if the snapshot is created using blockdev-snapshot-sync that
problem does not exist because that extra reference is not there.

> But again, I'd like to hear a libvirt opinion from Eric here.

Sure, me too.

Berto

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

[Kevin Wolf]

Am 20.10.2015 um 17:02 hat Alberto Garcia geschrieben:
> On Mon 19 Oct 2015 05:04:50 PM CEST, Kevin Wolf  wrote:
> >> > 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.
> 
> Can you have a BlockBackend on a BDS that is being used as a backing
> file? What is that for? And can you even write to it?

I think you can't create that currently, but it would be useful indeed.
You need it for cases like taking a snapshot and then making the
snapshot available through the NBD server.

Writing to backing files, though... Probably not a good idea.

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

> I think this is what best mirrors blockdev-add. It however has the
> following consequence:
> 
> blockdev-add id=drive0 node-name=node0 ...
> 
> blockdev-del node-name=node0 <-- This fails, node0 is used by drive0
> 
> blockdev-del id=drive0   <-- This works and removes drive0 and node0

I think this is okay. As you said, it mirrors blockdev-add.

Eric, would this work for you?

Kevin

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

[Alberto Garcia]

On Mon 19 Oct 2015 05:04:50 PM CEST, Kevin Wolf  wrote:
>> > 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.

Can you have a BlockBackend on a BDS that is being used as a backing
file? What is that for? And can you even write to it?

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

I think this is what best mirrors blockdev-add. It however has the
following consequence:

blockdev-add id=drive0 node-name=node0 ...

blockdev-del node-name=node0 <-- This fails, node0 is used by drive0

blockdev-del id=drive0   <-- This works and removes drive0 and node0

Berto

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

[Kevin Wolf]

Am 13.10.2015 um 15:48 hat Alberto Garcia geschrieben:
> Here's my first attempt at the 'blockdev-del' command.
> 
> This series goes on top of Max's "BlockBackend and media" v6:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html
> 
> With Max's code, 'blockdev-add' can now create a BlockDriverState with
> or without a BlockBackend (depending on whether the 'id' parameter is
> passed).
> 
> Therefore, 'blockdev-del' can be used to delete a backend and/or a
> BDS.
> 
> The way it works is simple: it receives a single 'device' parameter
> which can refer to a block backend or a node name.
> 
>  - If it's a block backend, it will delete it along with its attached
>BDS (if any).
> 
>  - If it's a node name, it will delete the BDS along with the backend
>it is attached to (if any).
> 
>  - If you're wondering whether it's possible to delete the BDS but not
>the backend it is attached to, there is already eject or
>blockdev-remove-medium for that.
> 
> If either the backend or the BDS are being used (refcount > 1, or if
> the BDS has any parents) the command fails.

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.

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.

2. Has/Hasn't been created explicitly by the user.
   Only if it has, the monitor owns a reference.

In addition, we also have BlockBackends without a BDS attached. All BBs
are explicitly created by the user. This leads us to the following
cases:

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

The question I've been thinking about for each of the cases is: Can we
create this with blockdev-add today, and what should blockdev-del do?
Another interesting question is: Can we move between the cases, e.g. by
adding a monitor reference retroactively, or do we even want to do that?

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.

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?

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.

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.

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.

If I compare this with the semantics described in the cover letter, I
must say that I see some problems, especially with case 3.

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

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

[Max Reitz]

On 19.10.2015 13:27, Kevin Wolf wrote:
> Am 13.10.2015 um 15:48 hat Alberto Garcia geschrieben:
>> Here's my first attempt at the 'blockdev-del' command.
>>
>> This series goes on top of Max's "BlockBackend and media" v6:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html
>>
>> With Max's code, 'blockdev-add' can now create a BlockDriverState with
>> or without a BlockBackend (depending on whether the 'id' parameter is
>> passed).
>>
>> Therefore, 'blockdev-del' can be used to delete a backend and/or a
>> BDS.
>>
>> The way it works is simple: it receives a single 'device' parameter
>> which can refer to a block backend or a node name.
>>
>>  - If it's a block backend, it will delete it along with its attached
>>BDS (if any).
>>
>>  - If it's a node name, it will delete the BDS along with the backend
>>it is attached to (if any).
>>
>>  - If you're wondering whether it's possible to delete the BDS but not
>>the backend it is attached to, there is already eject or
>>blockdev-remove-medium for that.
>>
>> If either the backend or the BDS are being used (refcount > 1, or if
>> the BDS has any parents) the command fails.
> 
> 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.
> 
> 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.
> 
> 2. Has/Hasn't been created explicitly by the user.
>Only if it has, the monitor owns a reference.
> 
> In addition, we also have BlockBackends without a BDS attached. All BBs
> are explicitly created by the user. This leads us to the following
> cases:
> 
> 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
> 
> The question I've been thinking about for each of the cases is: Can we
> create this with blockdev-add today, and what should blockdev-del do?
> Another interesting question is: Can we move between the cases, e.g. by
> adding a monitor reference retroactively, or do we even want to do that?
> 
> 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.
> 
> 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?
> 
> 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.

And keep in mind that in this case the monitor has two references, one
to the BB and one to the BDS, so in case these are the only "external"
references, the BB will have a refcount of 1 and the BDS a refcount of
2. Therefore, without any magic, you'll need two blockdev-del
invocations anyway, one for the BB and one for the BDS.

As Berto said, with patch 2 as it is, either blockdev-del will fail, and
you need to call remove-medium/eject first, which seems fine to me.

Max

> 4. That's the typical bs->file. 

Re: [Qemu-block] [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 

[Qemu-block] [PATCH 0/3] Add 'blockdev-del' command

[Alberto Garcia]

Here's my first attempt at the 'blockdev-del' command.

This series goes on top of Max's "BlockBackend and media" v6:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02810.html

With Max's code, 'blockdev-add' can now create a BlockDriverState with
or without a BlockBackend (depending on whether the 'id' parameter is
passed).

Therefore, 'blockdev-del' can be used to delete a backend and/or a
BDS.

The way it works is simple: it receives a single 'device' parameter
which can refer to a block backend or a node name.

 - If it's a block backend, it will delete it along with its attached
   BDS (if any).

 - If it's a node name, it will delete the BDS along with the backend
   it is attached to (if any).

 - If you're wondering whether it's possible to delete the BDS but not
   the backend it is attached to, there is already eject or
   blockdev-remove-medium for that.

If either the backend or the BDS are being used (refcount > 1, or if
the BDS has any parents) the command fails.

The series includes bunch of test cases with different scenarios
(nodes with and without backend, empty backend, backing images, block
jobs, Quorum).

Regards,

Berto

Alberto Garcia (3):
  block: Add blk_get_refcnt()
  block: Add 'blockdev-del' QMP command
  iotests: Add test for the blockdev-del command

 block/block-backend.c  |   5 ++
 blockdev.c |  42 +
 include/sysemu/block-backend.h |   1 +
 qapi/block-core.json   |  21 +
 qmp-commands.hx|  36 
 tests/qemu-iotests/139 | 189 +
 tests/qemu-iotests/139.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 8 files changed, 300 insertions(+)
 create mode 100644 tests/qemu-iotests/139
 create mode 100644 tests/qemu-iotests/139.out

-- 
2.6.1