Merge "Make sure device support Direct before setting"

cinder/image/image_utils.py

@@ -63,13 +63,28 @@ def qemu_img_info(path):
 
 def convert_image(source, dest, out_format, bps_limit=None):
     """Convert image to other format."""
-    start_time = timeutils.utcnow()
+
-    # Always set -t none. First it is needed for cgroup io/limiting
+    cmd = ('qemu-img', 'convert',
-    # and it is needed to ensure that all data hit the device before
+           '-O', out_format, source, dest)
-    # it gets unmapped remotely from the host
+
+    # Check whether O_DIRECT is supported and set '-t none' if it is
+    # This is needed to ensure that all data hit the device before
+    # it gets unmapped remotely from the host for some backends
+    # Reference Bug: #1363016
+
+    # NOTE(jdg): In the case of file devices qemu does the
+    # flush properly and more efficiently than would be done
+    # setting O_DIRECT, so check for that and skip the
+    # setting for non BLK devs
+    if (utils.is_blk_device(dest) and
+            volume_utils.check_for_odirect_support(source,
+                                                   dest,
+                                                   'oflag=direct')):
         cmd = ('qemu-img', 'convert',
                '-t', 'none',
                '-O', out_format, source, dest)
+
+    start_time = timeutils.utcnow()
     cgcmd = volume_utils.setup_blkio_cgroup(source, dest, bps_limit)
     if cgcmd:
         cmd = tuple(cgcmd) + cmd

cinder/tests/test_image_utils.py

@@ -84,11 +84,16 @@ class TestUtils(test.TestCase):
 
         mox = self._mox
         mox.StubOutWithMock(utils, 'execute')
+        mox.StubOutWithMock(utils, 'is_blk_device')
 
         TEST_OUT_FORMAT = 'vmdk'
         TEST_SOURCE = 'img/qemu.img'
         TEST_DEST = '/img/vmware.vmdk'
 
+        utils.is_blk_device(TEST_DEST).AndReturn(True)
+        utils.execute('dd', 'count=0', 'if=img/qemu.img',
+                      'of=/img/vmware.vmdk', 'oflag=direct',
+                      run_as_root=True)
         utils.execute(
             'qemu-img', 'convert', '-t', 'none', '-O', TEST_OUT_FORMAT,
             TEST_SOURCE, TEST_DEST, run_as_root=True)
@@ -207,12 +212,14 @@ class TestUtils(test.TestCase):
         mox.StubOutWithMock(utils, 'execute')
         mox.StubOutWithMock(image_utils, 'fetch')
         mox.StubOutWithMock(volume_utils, 'setup_blkio_cgroup')
+        mox.StubOutWithMock(utils, 'is_blk_device')
 
         TEST_INFO = ("image: qemu.qcow2\n"
                      "file format: raw\n"
                      "virtual size: 0 (0 bytes)\n"
                      "disk size: 0")
 
+        utils.is_blk_device(self.TEST_DEV_PATH).AndReturn(True)
         CONF.set_override('volume_copy_bps_limit', bps_limit)
 
         image_utils.create_temporary_file().AndReturn(self.TEST_DEV_PATH)
@@ -239,6 +246,11 @@ class TestUtils(test.TestCase):
                 prefix = ('cgexec', '-g', 'blkio:test')
             else:
                 prefix = ()
+
+            utils.execute('dd', 'count=0', 'if=/dev/ether/fake_dev',
+                          'of=/dev/ether/fake_dev', 'oflag=direct',
+                          run_as_root=True)
+
             cmd = prefix + ('qemu-img', 'convert', '-t', 'none', '-O', 'raw',
                             self.TEST_DEV_PATH, self.TEST_DEV_PATH)
 
@@ -306,8 +318,6 @@ class TestUtils(test.TestCase):
                           self.TEST_IMAGE_ID, self.TEST_DEV_PATH,
                           mox.IgnoreArg())
 
-        self._mox.VerifyAll()
-
     def test_fetch_to_raw_on_error_parsing_failed(self):
         SRC_INFO_NO_FORMAT = ("image: qemu.qcow2\n"
                               "virtual_size: 50M (52428800 bytes)\n"
@@ -321,7 +331,6 @@ class TestUtils(test.TestCase):
                           context, self._image_service,
                           self.TEST_IMAGE_ID, self.TEST_DEV_PATH,
                           mox.IgnoreArg())
-        self._mox.VerifyAll()
 
     def test_fetch_to_raw_on_error_backing_file(self):
         SRC_INFO_BACKING_FILE = ("image: qemu.qcow2\n"
@@ -338,7 +347,6 @@ class TestUtils(test.TestCase):
                           context, self._image_service,
                           self.TEST_IMAGE_ID, self.TEST_DEV_PATH,
                           mox.IgnoreArg())
-        self._mox.VerifyAll()
 
     @mock.patch('os.stat')
     def test_fetch_to_raw_on_error_not_convert_to_raw(self, mock_stat):
@@ -443,7 +451,8 @@ class TestUtils(test.TestCase):
             prefix = ('cgexec', '-g', 'blkio:test')
         else:
             prefix = ()
-        cmd = prefix + ('qemu-img', 'convert', '-t', 'none', '-O', 'qcow2',
+
+        cmd = prefix + ('qemu-img', 'convert', '-O', 'qcow2',
                         mox.IgnoreArg(), mox.IgnoreArg())
 
         m = self._mox
@@ -452,6 +461,7 @@ class TestUtils(test.TestCase):
 
         volume_utils.setup_blkio_cgroup(mox.IgnoreArg(), mox.IgnoreArg(),
                                         bps_limit).AndReturn(prefix)
+
         utils.execute(*cmd, run_as_root=True)
         utils.execute(
             'env', 'LC_ALL=C', 'qemu-img', 'info',
@@ -466,8 +476,37 @@ class TestUtils(test.TestCase):
 
     @mock.patch('os.stat')
     def test_upload_volume_with_bps_limit(self, mock_stat):
+        bps_limit = 1048576
+        image_meta = {'id': 1, 'disk_format': 'qcow2'}
+        TEST_RET = "image: qemu.qcow2\n"\
+                   "file_format: qcow2 \n"\
+                   "virtual_size: 50M (52428800 bytes)\n"\
+                   "cluster_size: 65536\n"\
+                   "disk_size: 196K (200704 bytes)"
 
-        self.test_upload_volume(bps_limit=1048576)
+        CONF.set_override('volume_copy_bps_limit', bps_limit)
+        prefix = ('cgexec', '-g', 'blkio:test')
+
+        cmd = prefix + ('qemu-img', 'convert', '-O', 'qcow2',
+                        mox.IgnoreArg(), mox.IgnoreArg())
+
+        m = self._mox
+        m.StubOutWithMock(utils, 'execute')
+        m.StubOutWithMock(volume_utils, 'setup_blkio_cgroup')
+        m.StubOutWithMock(volume_utils, 'check_for_odirect_support')
+
+        volume_utils.setup_blkio_cgroup(mox.IgnoreArg(), mox.IgnoreArg(),
+                                        bps_limit).AndReturn(prefix)
+        utils.execute(*cmd, run_as_root=True)
+        utils.execute(
+            'env', 'LC_ALL=C', 'qemu-img', 'info',
+            mox.IgnoreArg(), run_as_root=True).AndReturn(
+                (TEST_RET, 'ignored'))
+
+        m.ReplayAll()
+        image_utils.upload_volume(context, FakeImageService(),
+                                  image_meta, '/dev/loop1')
+        m.VerifyAll()
 
     def test_upload_volume_with_raw_image(self):
         image_meta = {'id': 1, 'disk_format': 'raw'}
@@ -493,8 +532,9 @@ class TestUtils(test.TestCase):
 
         m = self._mox
         m.StubOutWithMock(utils, 'execute')
+        m.StubOutWithMock(volume_utils, 'check_for_odirect_support')
 
-        utils.execute('qemu-img', 'convert', '-t', 'none', '-O', 'qcow2',
+        utils.execute('qemu-img', 'convert', '-O', 'qcow2',
                       mox.IgnoreArg(), mox.IgnoreArg(), run_as_root=True)
         utils.execute(
             'env', 'LC_ALL=C', 'qemu-img', 'info',

cinder/tests/test_volume.py

@@ -920,6 +920,9 @@ class VolumeTestCase(BaseVolumeTestCase):
             return inner_sync2
         return inner_sync1
 
+    def _fake_execute(self, *cmd, **kwargs):
+        pass
+
     def test_create_volume_from_snapshot_check_locks(self):
         # mock the synchroniser so we can record events
         self.stubs.Set(utils, 'synchronized', self._mock_synchronized)
@@ -989,6 +992,7 @@ class VolumeTestCase(BaseVolumeTestCase):
     def test_create_volume_from_volume_check_locks(self):
         # mock the synchroniser so we can record events
         self.stubs.Set(utils, 'synchronized', self._mock_synchronized)
+        self.stubs.Set(utils, 'execute', self._fake_execute)
 
         orig_flow = engine.ActionEngine.run
 

cinder/tests/test_volume_utils.py

@@ -213,6 +213,13 @@ class CopyVolumeTestCase(test.TestCase):
             if 'iflag=direct' in cmd and 'oflag=direct' in cmd:
                 raise exception.InvalidInput(message='iflag/oflag error')
 
+        def fake_check_odirect(src, dest, flags='blah'):
+            return False
+
+        self.stubs.Set(volume_utils,
+                       'check_for_odirect_support',
+                       fake_check_odirect)
+
         volume_utils.copy_volume('/dev/zero', '/dev/null', 1024,
                                  CONF.volume_dd_blocksize, sync=True,
                                  ionice=None, execute=fake_utils_execute)

cinder/utils.py

@@ -757,3 +757,13 @@ def remove_invalid_filter_options(context, filters,
     LOG.debug(log_msg)
     for opt in unknown_options:
         del filters[opt]
+
+
+def is_blk_device(dev):
+    try:
+        if stat.S_ISBLK(os.stat(dev).st_mode):
+            return True
+        return False
+    except Exception:
+        LOG.debug('Path %s not found in is_blk_device check' % dev)
+        return False

cinder/volume/utils.py

@@ -285,18 +285,26 @@ def _calculate_count(size_in_m, blocksize):
     return blocksize, int(count)
 
 
+def check_for_odirect_support(src, dest, flag='oflag=direct'):
+
+    # Check whether O_DIRECT is supported
+    try:
+        utils.execute('dd', 'count=0', 'if=%s' % src, 'of=%s' % dest,
+                      flag, run_as_root=True)
+        return True
+    except processutils.ProcessExecutionError:
+        return False
+
+
 def copy_volume(srcstr, deststr, size_in_m, blocksize, sync=False,
                 execute=utils.execute, ionice=None):
     # Use O_DIRECT to avoid thrashing the system buffer cache
     extra_flags = []
-    # Check whether O_DIRECT is supported to iflag and oflag separately
+    if check_for_odirect_support(srcstr, deststr, 'iflag=direct'):
-    for flag in ['iflag=direct', 'oflag=direct']:
+        extra_flags.append('iflag=direct')
-        try:
+
-            execute('dd', 'count=0', 'if=%s' % srcstr, 'of=%s' % deststr,
+    if check_for_odirect_support(srcstr, deststr, 'oflag=direct'):
-                    flag, run_as_root=True)
+        extra_flags.append('oflag=direct')
-            extra_flags.append(flag)
-        except processutils.ProcessExecutionError:
-            pass
 
     # If the volume is being unprovisioned then
     # request the data is persisted before returning,