[Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Alberto Garcia]

When x-blockdev-del is performed on a BlockBackend that has inserted
media it will only succeed if the BDS doesn't have any additional
references.

The only problem with this is that if the BDS was created separately
using blockdev-add then the backend won't be able to be destroyed
unless the BDS is ejected first. This is an unnecessary restriction.

Now that we have a list of monitor-owned BDSs we can allow
x-blockdev-del to work in this scenario if the BDS has exactly one
extra reference and that reference is from the monitor.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e1b6b0f..847058d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3966,9 +3966,16 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
         }
 
         if (bs->refcnt > 1) {
-            error_setg(errp, "Block device %s is in use",
-                       bdrv_get_device_or_node_name(bs));
-            goto out;
+            /* We allow deleting a BlockBackend that has a BDS with an
+             * extra reference if that extra reference is from the
+             * monitor. */
+            bool bs_has_only_monitor_ref =
+                blk && bs->monitor_list.tqe_prev && bs->refcnt == 2;
+            if (!bs_has_only_monitor_ref) {
+                error_setg(errp, "Block device %s is in use",
+                           bdrv_get_device_or_node_name(bs));
+                goto out;
+            }
         }
     }
 
-- 
2.7.0

[Qemu-devel] [PATCH 2/3] iotests: Update test 139 after the changes in x-blockdev-del

[Alberto Garcia]

Now that we allow x-blockdev-del to work in a new scenario that
previously forbidden we must update the iotests in order to reflect
this change.

Both testAttachMedia() and testSnapshot() are splitted in two: one
version keeps the previous behavior, and a second version checks that
the new functionality works as expected.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
 tests/qemu-iotests/139.out |  4 ++--
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index a4b9694..a6fc299 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -330,21 +330,32 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.delDeviceModel('device0')
         self.delBlockBackend('drive0', 'node0')
 
-    def testAttachMedia(self):
+    def testAttachMedia1(self):
         # This creates a BlockBackend and removes its media
         self.addBlockBackend('drive0', 'node0')
         self.ejectDrive('drive0', 'node0')
         # This creates a new BlockDriverState and inserts it into the backend
         self.addBlockDriverState('node1')
         self.insertDrive('drive0', 'node1')
-        # The backend can't be removed: the new BDS has an extra reference
-        self.delBlockBackend('drive0', 'node1', expect_error = True)
+        # The BDS can't be removed because it's attached to a backend
         self.delBlockDriverState('node1', expect_error = True)
         # The BDS still exists after being ejected, but now it can be removed
         self.ejectDrive('drive0', 'node1', destroys_media = False)
         self.delBlockDriverState('node1')
         self.delBlockBackend('drive0', None)
 
+    def testAttachMedia2(self):
+        # This creates a BlockBackend and removes its media
+        self.addBlockBackend('drive0', 'node0')
+        self.ejectDrive('drive0', 'node0')
+        # This creates a new BlockDriverState and inserts it into the backend
+        self.addBlockDriverState('node1')
+        self.insertDrive('drive0', 'node1')
+        # The BlockDriverState has a monitor reference so we can destroy the backend
+        self.delBlockBackend('drive0', 'node1', destroys_media = False)
+        # The backend has been destroyed, now we can proceed with the BDS
+        self.delBlockDriverState('node1')
+
     def testSnapshotSync(self):
         self.addBlockBackend('drive0', 'node0')
         self.createSnapshotSync('node0', 'overlay0')
@@ -354,11 +365,10 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.delBlockBackend('drive0', 'overlay0')
         self.checkBlockDriverState('node0', False)
 
-    def testSnapshot(self):
+    def testSnapshot1(self):
         self.addBlockBackend('drive0', 'node0')
         self.addBlockDriverStateOverlay('overlay0')
         self.createSnapshot('node0', 'overlay0')
-        self.delBlockBackend('drive0', 'overlay0', expect_error = True)
         self.delBlockDriverState('node0', expect_error = True)
         self.delBlockDriverState('overlay0', expect_error = True)
         self.ejectDrive('drive0', 'overlay0', destroys_media = False)
@@ -367,6 +377,16 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.delBlockDriverState('overlay0')
         self.checkBlockDriverState('node0', False)
 
+    def testSnapshot2(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.addBlockDriverStateOverlay('overlay0')
+        self.createSnapshot('node0', 'overlay0')
+        # The BlockDriverState has a monitor reference so we can destroy the backend
+        self.delBlockBackend('drive0', 'overlay0', destroys_media = False)
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('overlay0')
+        self.checkBlockDriverState('node0', False)
+
     def testMirror(self):
         self.addBlockBackend('drive0', 'node0')
         self.createMirror('drive0', 'node0', 'mirror0')
diff --git a/tests/qemu-iotests/139.out b/tests/qemu-iotests/139.out
index 281b69e..6323079 100644
--- a/tests/qemu-iotests/139.out
+++ b/tests/qemu-iotests/139.out
@@ -1,5 +1,5 @@
-............
+..............
 ----------------------------------------------------------------------
-Ran 12 tests
+Ran 14 tests
 
 OK
-- 
2.7.0

Re: [Qemu-devel] [PATCH 2/3] iotests: Update test 139 after the changes in x-blockdev-del

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On 02/08/2016 07:14 AM, Alberto Garcia wrote:
> Now that we allow x-blockdev-del to work in a new scenario that
> previously forbidden we must update the iotests in order to reflect
> this change.
> 
> Both testAttachMedia() and testSnapshot() are splitted in two: one

s/splitted/split/ (one of those weird English words whose past tense is
identical to present tense)

> version keeps the previous behavior, and a second version checks that
> the new functionality works as expected.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
>  tests/qemu-iotests/139.out |  4 ++--
>  2 files changed, 27 insertions(+), 7 deletions(-)

Does the test fail if 1/3 is applied in isolation? If so, then this
patch must be squashed with that one.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] iotests: Update test 139 after the changes in x-blockdev-del

[Alberto Garcia]

On Tue 09 Feb 2016 04:36:31 PM CET, Eric Blake wrote:

>> Both testAttachMedia() and testSnapshot() are splitted in two: one
>
> s/splitted/split/ (one of those weird English words whose past tense
> is identical to present tense)

:)

>>  tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
>>  tests/qemu-iotests/139.out |  4 ++--
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> Does the test fail if 1/3 is applied in isolation? If so, then this
> patch must be squashed with that one.

Yes, it fails.

Berto

[Qemu-devel] [PATCH 3/3] block: Update the x-blockdev-del documentation

[Alberto Garcia]

Explain what happens if the user tries to delete a BlockBackend that
contains media that was added separately using blockdev-add.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 qapi/block-core.json | 5 ++++-
 qmp-commands.hx      | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 33012b8..cc59ab9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2089,7 +2089,10 @@
 #
 # In the former case the backend will be destroyed, along with its
 # inserted medium if there's any. The command will fail if the backend
-# or its medium are in use.
+# or its medium are in use. If the medium was also created with
+# blockdev-add then the command will succeed, the backend will be
+# destroyed but the medium will remain until it is deleted with
+# x-blockdev-del.
 #
 # In the latter case the node will be destroyed. The command will fail
 # if the node is attached to a block backend or is otherwise being
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 020e5ee..9710686 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4151,7 +4151,10 @@ The selected device can be either a block backend or a graph node.
 
 In the former case the backend will be destroyed, along with its
 inserted medium if there's any. The command will fail if the backend
-or its medium are in use.
+or its medium are in use. If the medium was also created with
+blockdev-add then the command will succeed, the backend will be
+destroyed but the medium will remain until it is deleted with
+x-blockdev-del.
 
 In the latter case the node will be destroyed. The command will fail
 if the node is attached to a block backend or is otherwise being
-- 
2.7.0

Re: [Qemu-devel] [PATCH 3/3] block: Update the x-blockdev-del documentation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On 02/08/2016 07:14 AM, Alberto Garcia wrote:
> Explain what happens if the user tries to delete a BlockBackend that
> contains media that was added separately using blockdev-add.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  qapi/block-core.json | 5 ++++-
>  qmp-commands.hx      | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Kevin Wolf]

Am 08.02.2016 um 15:14 hat Alberto Garcia geschrieben:
> When x-blockdev-del is performed on a BlockBackend that has inserted
> media it will only succeed if the BDS doesn't have any additional
> references.
> 
> The only problem with this is that if the BDS was created separately
> using blockdev-add then the backend won't be able to be destroyed
> unless the BDS is ejected first. This is an unnecessary restriction.
> 
> Now that we have a list of monitor-owned BDSs we can allow
> x-blockdev-del to work in this scenario if the BDS has exactly one
> extra reference and that reference is from the monitor.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

This means that what you created with two blockdev-add commands would be
destroyed with a single blockdev-del command. I think we all agreed that
blockdev-add and blockdev-del should always come in pairs.

Kevin

Re: [Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Alberto Garcia]

On Tue 09 Feb 2016 11:32:20 AM CET, Kevin Wolf <kwolf@redhat.com> wrote:
>> When x-blockdev-del is performed on a BlockBackend that has inserted
>> media it will only succeed if the BDS doesn't have any additional
>> references.
>> 
>> The only problem with this is that if the BDS was created separately
>> using blockdev-add then the backend won't be able to be destroyed
>> unless the BDS is ejected first. This is an unnecessary restriction.
>> 
>> Now that we have a list of monitor-owned BDSs we can allow
>> x-blockdev-del to work in this scenario if the BDS has exactly one
>> extra reference and that reference is from the monitor.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> This means that what you created with two blockdev-add commands would be
> destroyed with a single blockdev-del command. I think we all agreed that
> blockdev-add and blockdev-del should always come in pairs.

No, no. I think you are misunderstanding the use case.

Consider this example (testAttachMedia() from iotest 139):

1) blockdev-add creates 'drive0' with a BDS called 'node0'
2) 'node0' is removed from 'drive0' (and therefore destroyed)
3) 'node1' is added using blockdev-add
4) 'node1' is inserted into 'drive0'

Now we want to destroy both 'drive0' and 'node1'. With the current code
we can only do it like this:

5) 'node1' is removed from 'drive0', but not destroyed because it still
    has the monitor reference.
6) 'drive0' is destroyed with x-blockdev-del
7) 'node1' is destroyed with x-blockdev-del

The order of 6) and 7) can be changed without problems. However, 5) is
necessary because otherwise 'x-blockdev-del drive0' won't work ('node1'
has 2 references at that point).

I claim that step 5) should be unnecessary.

If we can guarantee that 'node1' has only two references: the one held
by 'drive0' and the monitor reference, then it should be possible to do
'x-blockdev-del drive0' when 'node1' is inserted.

This would destroy 'drive0' but 'node1' would still remain because of
its monitor reference. Now we can do 'x-blockdev-del node1' and complete
the operation.

Berto

Re: [Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

On 02/08/2016 07:14 AM, Alberto Garcia wrote:

When sending a multi-patch series, you should always include a 0/3 cover
letter.  The cover letter is optional only for a lone patch.

> When x-blockdev-del is performed on a BlockBackend that has inserted
> media it will only succeed if the BDS doesn't have any additional
> references.
> 
> The only problem with this is that if the BDS was created separately
> using blockdev-add then the backend won't be able to be destroyed
> unless the BDS is ejected first. This is an unnecessary restriction.
> 
> Now that we have a list of monitor-owned BDSs we can allow
> x-blockdev-del to work in this scenario if the BDS has exactly one
> extra reference and that reference is from the monitor.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index e1b6b0f..847058d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3966,9 +3966,16 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
>          }
>  
>          if (bs->refcnt > 1) {
> -            error_setg(errp, "Block device %s is in use",
> -                       bdrv_get_device_or_node_name(bs));
> -            goto out;
> +            /* We allow deleting a BlockBackend that has a BDS with an
> +             * extra reference if that extra reference is from the
> +             * monitor. */
> +            bool bs_has_only_monitor_ref =
> +                blk && bs->monitor_list.tqe_prev && bs->refcnt == 2;
> +            if (!bs_has_only_monitor_ref) {

I don't think the temporary bool or nested 'if' are necessary; but at
the same time, I don't think the following is any more legible:

/* Prohibit deleting a BlockBackend whose BDS is in use by any more than
a single monitor */
if (bs->refcnt > 1 + (blk && bs->monitor_list.tqe_prev)) {
    error_setg(...

so I could live with your patch as-is:
Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block: Allow x-blockdev-del on a BB with a monitor-owned BDS

[Alberto Garcia]

On Tue 09 Feb 2016 04:34:09 PM CET, Eric Blake wrote:
> On 02/08/2016 07:14 AM, Alberto Garcia wrote:
>
> When sending a multi-patch series, you should always include a 0/3 cover
> letter.  The cover letter is optional only for a lone patch.

Sorry, I didn't know. The description of the first patch already
contains everything that would go into the cover letter, that's why I
decided to do it like this. I'll include the cover letter in the future.

>>          if (bs->refcnt > 1) {
>> -            error_setg(errp, "Block device %s is in use",
>> -                       bdrv_get_device_or_node_name(bs));
>> -            goto out;
>> +            /* We allow deleting a BlockBackend that has a BDS with an
>> +             * extra reference if that extra reference is from the
>> +             * monitor. */
>> +            bool bs_has_only_monitor_ref =
>> +                blk && bs->monitor_list.tqe_prev && bs->refcnt == 2;
>> +            if (!bs_has_only_monitor_ref) {
>
> I don't think the temporary bool or nested 'if' are necessary; but at
> the same time, I don't think the following is any more legible:
>
> /* Prohibit deleting a BlockBackend whose BDS is in use by any more than
> a single monitor */
> if (bs->refcnt > 1 + (blk && bs->monitor_list.tqe_prev)) {
>     error_setg(...

Exactly, I considered several options and I thought the one I finally
chose would be the easiest to read.

Thanks!

Berto