| Message ID | 87083486787f4d4b9752f4b29209e6d0d1d31626.1455033112.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 --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
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(-)