diff mbox

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

Message ID 87083486787f4d4b9752f4b29209e6d0d1d31626.1455033112.git.berto@igalia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alberto Garcia Feb. 9, 2016, 3:57 p.m. UTC
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(-)

Comments

Max Reitz Feb. 24, 2016, 4:15 p.m. UTC | #1
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
Alberto Garcia Feb. 25, 2016, 11:11 a.m. UTC | #2
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
Kevin Wolf Feb. 26, 2016, 8:58 a.m. UTC | #3
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
Alberto Garcia Feb. 26, 2016, 9:09 a.m. UTC | #4
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
diff mbox

Patch

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