From 1b3322d45fe2c5ed72cc7f8674e5e319928065ad Mon Sep 17 00:00:00 2001 From: John Griffith Date: Wed, 5 Sep 2012 19:27:48 +0000 Subject: [PATCH] Don't zero out snapshot volume on snapshot_delete When trying to zero out an LVM snapshot on precise the kernel sometimes hangs when performing the dd. Also the dd process itself can take an extremely long time even when it does succesfully complete. This can be up to 30 minutes for a 1 Gig volume/snapshot. I believe this is a kernel specific issue with LVM snapshots. The zeroing process is unreliable and can cause kernel hangs to let's remove it. In order to protect against data leakage we'll implement the zeroing process on volume creation. This doesn't seem to have a significant impact and doesn't suffer from the same isues tha zeroing out an LVM snapshot does. No reason to continue zero on delete, the zero on creation should probably be sufficient. Doesn't seem to cause any timing issues but need to keep this in mind. Fixes bug 1023755 Change-Id: I56209d8e5973cffa997b4ec3e51c3361838386de --- cinder/tests/test_volume.py | 6 +++--- cinder/volume/driver.py | 22 +++++++++++++--------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index ef0715636..4d752b108 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -767,17 +767,17 @@ class VolumeDriverTestCase(DriverTestCase): self.stubs.Set(self.volume.driver, '_volume_not_present', lambda x: False) self.stubs.Set(self.volume.driver, '_delete_volume', - lambda x, y: False) + lambda x: False) # Want DriverTestCase._fake_execute to return 'o' so that # volume.driver.delete_volume() raises the VolumeIsBusy exception. self.output = 'o' self.assertRaises(exception.VolumeIsBusy, self.volume.driver.delete_volume, - {'name': 'test1', 'size': 1024}) + {'name': 'test1'}) # when DriverTestCase._fake_execute returns something other than # 'o' volume.driver.delete_volume() does not raise an exception. self.output = 'x' - self.volume.driver.delete_volume({'name': 'test1', 'size': 1024}) + self.volume.driver.delete_volume({'name': 'test1'}) class ISCSITestCase(DriverTestCase): diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 435e023e4..cdae2a18d 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -143,11 +143,8 @@ class VolumeDriver(object): return True return False - def _delete_volume(self, volume, size_in_g): + def _delete_volume(self, volume): """Deletes a logical volume.""" - # zero out old volumes to prevent data leaking between users - # TODO(ja): reclaiming space should be done lazy and low priority - self._copy_volume('/dev/zero', self.local_path(volume), size_in_g) self._try_execute('lvremove', '-f', "%s/%s" % (FLAGS.volume_group, self._escape_snapshot(volume['name'])), @@ -170,6 +167,15 @@ class VolumeDriver(object): changes to the volume object to be persisted.""" self._create_volume(volume['name'], self._sizestr(volume['size'])) + # NOTE(jdg): As per BUG 1023755 zeroing out snapshot volume + # is extremely slow and on 12.04 has a tendency to hang the kernel + # We're going to work around this by dropping the zero out process + # on snapshot delete, but we need to protect form data leakage still + # so we'll zero out newly created volumes to hack around this. + self._copy_volume('/dev/zero', + self.local_path(volume), + volume['size']) + def create_volume_from_snapshot(self, volume, snapshot): """Creates a volume from a snapshot.""" self._create_volume(volume['name'], self._sizestr(volume['size'])) @@ -195,7 +201,7 @@ class VolumeDriver(object): if (out[0] == 'o') or (out[0] == 'O'): raise exception.VolumeIsBusy(volume_name=volume['name']) - self._delete_volume(volume, volume['size']) + self._delete_volume(volume) def create_snapshot(self, snapshot): """Creates a snapshot.""" @@ -211,9 +217,7 @@ class VolumeDriver(object): # If the snapshot isn't present, then don't attempt to delete return True - # TODO(yamahata): zeroing out the whole snapshot triggers COW. - # it's quite slow. - self._delete_volume(snapshot, snapshot['volume_size']) + self._delete_volume(snapshot) def local_path(self, volume): # NOTE(vish): stops deprecation warning @@ -350,7 +354,6 @@ class ISCSIDriver(VolumeDriver): def create_export(self, context, volume): """Creates an export for a logical volume.""" - #BOOKMARK(jdg) iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name']) volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name']) @@ -380,6 +383,7 @@ class ISCSIDriver(VolumeDriver): def remove_export(self, context, volume): """Removes an export for a logical volume.""" + # NOTE(jdg): tgtadm doesn't use the iscsi_targets table # TODO(jdg): In the future move all of the dependent stuff into the # cooresponding target admin class -- 2.45.2