From e9722f09dfd4b58e40afbf285ab9a7738a641dca Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Thu, 23 Jan 2014 11:57:17 -0500 Subject: [PATCH] GlusterFS: Fix deadlock in volume clone The create_cloned_volume path could deadlock due to create_cloned_volume and create/delete_snapshot using the same lock for synchronization. Refactor the calls to create/delete snapshot to call the inner method which does not use a lock. Introduced by "06999f6 GlusterFS: Synchronize additional op..." Related-Bug: 1267983 Closes-Bug: 1272092 Change-Id: I84ca34b201c10644faa047f1c9274c14bcdd0359 --- cinder/tests/test_glusterfs.py | 16 ++++++++-------- cinder/volume/drivers/glusterfs.py | 18 ++++++++++++++---- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index 9fa60efcb70..ed81dbb828a 100644 --- a/cinder/tests/test_glusterfs.py +++ b/cinder/tests/test_glusterfs.py @@ -596,8 +596,8 @@ class GlusterFsDriverTestCase(test.TestCase): def test_create_cloned_volume(self): (mox, drv) = self._mox, self._driver - mox.StubOutWithMock(drv, 'create_snapshot') - mox.StubOutWithMock(drv, 'delete_snapshot') + mox.StubOutWithMock(drv, '_create_snapshot') + mox.StubOutWithMock(drv, '_delete_snapshot') mox.StubOutWithMock(drv, '_read_info_file') mox.StubOutWithMock(image_utils, 'convert_image') mox.StubOutWithMock(drv, '_copy_volume_from_snapshot') @@ -630,7 +630,7 @@ class GlusterFsDriverTestCase(test.TestCase): 'id': 'tmp-snap-%s' % src_vref['id'], 'volume': src_vref} - drv.create_snapshot(snap_ref) + drv._create_snapshot(snap_ref) snap_info = {'active': volume_file, snap_ref['id']: volume_path + '-clone'} @@ -639,7 +639,7 @@ class GlusterFsDriverTestCase(test.TestCase): drv._copy_volume_from_snapshot(snap_ref, volume_ref, volume['size']) - drv.delete_snapshot(mox_lib.IgnoreArg()) + drv._delete_snapshot(mox_lib.IgnoreArg()) mox.ReplayAll() @@ -1590,9 +1590,9 @@ class GlusterFsDriverTestCase(test.TestCase): volume = self._simple_volume('c1073000-0000-0000-0000-0000000c1073') src_volume = self._simple_volume() - mox.StubOutWithMock(drv, 'create_snapshot') + mox.StubOutWithMock(drv, '_create_snapshot') mox.StubOutWithMock(drv, '_copy_volume_from_snapshot') - mox.StubOutWithMock(drv, 'delete_snapshot') + mox.StubOutWithMock(drv, '_delete_snapshot') snap_ref = {'volume_name': src_volume['name'], 'name': 'clone-snap-%s' % src_volume['id'], @@ -1608,11 +1608,11 @@ class GlusterFsDriverTestCase(test.TestCase): 'provider_location': volume['provider_location'], 'name': 'volume-' + volume['id']} - drv.create_snapshot(snap_ref) + drv._create_snapshot(snap_ref) drv._copy_volume_from_snapshot(snap_ref, volume_ref, src_volume['size']) - drv.delete_snapshot(snap_ref) + drv._delete_snapshot(snap_ref) mox.ReplayAll() diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py index 71ac852a047..c738be5a7b0 100644 --- a/cinder/volume/drivers/glusterfs.py +++ b/cinder/volume/drivers/glusterfs.py @@ -180,14 +180,14 @@ class GlusterfsDriver(nfs.RemoteFsDriver): 'volume_id': src_vref['id'], 'id': 'tmp-snap-%s' % src_vref['id'], 'volume': src_vref} - self.create_snapshot(temp_snapshot) + self._create_snapshot(temp_snapshot) try: self._copy_volume_from_snapshot(temp_snapshot, volume_info, src_vref['size']) finally: - self.delete_snapshot(temp_snapshot) + self._delete_snapshot(temp_snapshot) return {'provider_location': src_vref['provider_location']} @@ -205,6 +205,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): return {'provider_location': volume['provider_location']} + @utils.synchronized('glusterfs', external=False) def create_volume_from_snapshot(self, volume, snapshot): """Creates a volume from a snapshot. @@ -283,6 +284,11 @@ class GlusterfsDriver(nfs.RemoteFsDriver): @utils.synchronized('glusterfs', external=False) def create_snapshot(self, snapshot): + """Apply locking to the create snapshot operation.""" + + return self._create_snapshot(snapshot) + + def _create_snapshot(self, snapshot): """Create a snapshot. If volume is attached, call to Nova to create snapshot, @@ -451,7 +457,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): LOG.debug(_('volume id: %s') % snapshot['volume_id']) path_to_disk = self._local_path_volume(snapshot['volume']) - self._create_snapshot(snapshot, path_to_disk) + self._create_snapshot_offline(snapshot, path_to_disk) def _create_qcow2_snap_file(self, snapshot, backing_filename, new_snap_path): @@ -480,7 +486,7 @@ class GlusterfsDriver(nfs.RemoteFsDriver): new_snap_path] self._execute(*command, run_as_root=True) - def _create_snapshot(self, snapshot, path_to_disk): + def _create_snapshot_offline(self, snapshot, path_to_disk): """Create snapshot (offline case).""" # Requires volume status = 'available' @@ -535,6 +541,10 @@ class GlusterfsDriver(nfs.RemoteFsDriver): @utils.synchronized('glusterfs', external=False) def delete_snapshot(self, snapshot): + """Apply locking to the delete snapshot operation.""" + self._delete_snapshot(snapshot) + + def _delete_snapshot(self, snapshot): """Delete a snapshot. If volume status is 'available', delete snapshot here in Cinder