[Qemu-devel] [PATCH v2 0/2] Allow x-blockdev-del on a BB with a monitor-owned BDS

[Qemu-devel] [PATCH v2 0/2] Allow x-blockdev-del on a BB with a monitor-owned BDS

[Alberto Garcia]

This series allow x-blockdev-del to be peformed on a BlockBackend that
has a BDS inserted with an additional reference if that reference is
from the monitor.

v2 squashes two of the patches together and fixes the commit message,
but is otherwise the same as v1.

Regards,

Berto

v2:
- Squash patch 2 (with the tests) into the first one with the code [Eric]
- s/splitted/split/ in the description of the tests [Eric]

v1: https://lists.gnu.org/archive/html/qemu-block/2016-02/msg00247.html
- Initial version

Alberto Garcia (2):
  block: Allow x-blockdev-del on a BB with a monitor-owned BDS
  block: Update the x-blockdev-del documentation

 blockdev.c                 | 13 ++++++++++---
 qapi/block-core.json       |  5 ++++-
 qmp-commands.hx            |  5 ++++-
 tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
 tests/qemu-iotests/139.out |  4 ++--
 5 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH v2 1/2] 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.

This patch also updates iotest 139 to reflect this change. Both
testAttachMedia() and testSnapshot() are split 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>
---
 blockdev.c                 | 13 ++++++++++---
 tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
 tests/qemu-iotests/139.out |  4 ++--
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1f73478..499a96b 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;
+            }
         }
     }
 
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

[Qemu-devel] [PATCH v2 2/2] 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>
Reviewed-by: Eric Blake <eblake@redhat.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 v2 0/2] Allow x-blockdev-del on a BB with a monitor-owned BDS

[Alberto Garcia]

On Tue 09 Feb 2016 04:57:34 PM CET, Alberto Garcia wrote:
> This series allow x-blockdev-del to be peformed on a BlockBackend that
> has a BDS inserted with an additional reference if that reference is
> from the monitor.
>
> v2 squashes two of the patches together and fixes the commit message,
> but is otherwise the same as v1.

Ping

Berto

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

[Max Reitz]

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

On 09.02.2016 16:57, Alberto Garcia 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.

Is it? In order to get into this situation, you need to execute:
blockdev-add (BB/BDS), blockdev-add (BDS/BB), x-blockdev-insert-medium

Now, in order to unravel it, you currently need:
x-blockdev-remove-medium (or eject), x-blockdev-del (BB/BDS),
x-blockdev-del (BDS/BB)

So you need to execute the x-blockdev-remove-medium because you did an
x-blockdev-insert-medium before. That seems reasonable to me, and not
very superfluous.

In fact, the behavior allowed by this patch appears a bit inconsistent
to me. Why is it OK for x-blockdev-del to automatically eject a BDS from
a BB if the BB is deleted, but not if the BDS is deleted? (which is what
your modifications to test 139 verify)

> 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.
> 
> This patch also updates iotest 139 to reflect this change. Both
> testAttachMedia() and testSnapshot() are split 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>
> ---
>  blockdev.c                 | 13 ++++++++++---
>  tests/qemu-iotests/139     | 30 +++++++++++++++++++++++++-----
>  tests/qemu-iotests/139.out |  4 ++--
>  3 files changed, 37 insertions(+), 10 deletions(-)

The patch itself looks fine to me, but I'm not so sure about the idea
behind it.

Max


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

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

[Alberto Garcia]

On Wed 24 Feb 2016 05:15:11 PM CET, Max Reitz 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.
>
> Is it? In order to get into this situation, you need to execute:
> blockdev-add (BB/BDS), blockdev-add (BDS/BB), x-blockdev-insert-medium
>
> Now, in order to unravel it, you currently need:
> x-blockdev-remove-medium (or eject), x-blockdev-del (BB/BDS),
> x-blockdev-del (BDS/BB)
>
> So you need to execute the x-blockdev-remove-medium because you did an
> x-blockdev-insert-medium before. That seems reasonable to me, and not
> very superfluous.

I think your case is reasonable, but it's not the only way to get into
this situation. See for example this one:

blockdev-add 'drive0', 'node0'
blockdev-add 'node1'
blockdev-snapshot node='node0' overlay='node1'

Now you have 'drive0' with 'node0' <- 'node1'.

You cannot simply remove 'drive0', you need to eject 'node1' first and
then you can remove 'drive0' and 'node1'.

Berto

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

[Kevin Wolf]

Am 25.02.2016 um 12:11 hat Alberto Garcia geschrieben:
> On Wed 24 Feb 2016 05:15:11 PM CET, Max Reitz 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.
> >
> > Is it? In order to get into this situation, you need to execute:
> > blockdev-add (BB/BDS), blockdev-add (BDS/BB), x-blockdev-insert-medium
> >
> > Now, in order to unravel it, you currently need:
> > x-blockdev-remove-medium (or eject), x-blockdev-del (BB/BDS),
> > x-blockdev-del (BDS/BB)
> >
> > So you need to execute the x-blockdev-remove-medium because you did an
> > x-blockdev-insert-medium before. That seems reasonable to me, and not
> > very superfluous.
> 
> I think your case is reasonable, but it's not the only way to get into
> this situation. See for example this one:
> 
> blockdev-add 'drive0', 'node0'
> blockdev-add 'node1'
> blockdev-snapshot node='node0' overlay='node1'
> 
> Now you have 'drive0' with 'node0' <- 'node1'.
> 
> You cannot simply remove 'drive0', you need to eject 'node1' first and
> then you can remove 'drive0' and 'node1'.

I think this is even more a reason not to introduce any magic but to
require that node and BB be removed separately. Otherwise it will become
really confusing to track for management software which node is supposed
to automatically go away and which isn't.

That said, I'm still hoping to somehow find a compatible way to
completely hide BBs so that they are automatically created and deleted
whenever a new user attaches to a BDS.

Kevin

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

[Alberto Garcia]

On Fri 26 Feb 2016 09:58:54 AM CET, Kevin Wolf wrote:
>> I think your case is reasonable, but it's not the only way to get
>> into this situation. See for example this one:
>> 
>> blockdev-add 'drive0', 'node0'
>> blockdev-add 'node1'
>> blockdev-snapshot node='node0' overlay='node1'
>> 
>> Now you have 'drive0' with 'node0' <- 'node1'.
>> 
>> You cannot simply remove 'drive0', you need to eject 'node1' first
>> and then you can remove 'drive0' and 'node1'.
>
> I think this is even more a reason not to introduce any magic but to
> require that node and BB be removed separately. Otherwise it will
> become really confusing to track for management software which node is
> supposed to automatically go away and which isn't.

Note that with this patch you still need to remove the node and the BB
separately, it only spares you to eject a node that was not explicitly
inserted in the first place.

That said, I think Max's example convinced me that this is not as clear
as I initially thought, so if you're not convinced either I have no
problem to withdraw the patch.

Thanks,

Berto