From: Eric Harney <eharney@redhat.com>
Date: Thu, 23 Jan 2014 16:57:17 +0000 (-0500)
Subject: GlusterFS: Fix deadlock in volume clone
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=e9722f09dfd4b58e40afbf285ab9a7738a641dca;p=openstack-build%2Fcinder-build.git

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
---

diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py
index 9fa60efcb..ed81dbb82 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 71ac852a0..c738be5a7 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