Make sure device support Direct before setting

We added '-t none' option to the qemu-img convert operation
in image_utils.py a while back to accomodate a couple of
backend devices that didn't flush writes on disconnect.
(Change: I7a04f683add8c23b9125fe837c4048ccc3ac224d)

The only problem here is that some backend devices don't
support Direct mode and raise an exception and fail when
setting this option.

This patch adds a simple check using dd to see if the dest
supports the Direct flag and only sets '-t none' if the device
does in fact support it.

Additionally it was brought up that even yet other backends
are using file devices not blk devices.  In their case setting
Direct will still work, however it's sub-optimal as qemu-convert
has internal mechanisms to make sure flushing etc are done
correctly and efficiently for those devices.  So to accomodate
that particular use case I'm also adding a check if blk dev
that can be used for determining whether to set Direct for the
qemu-convert process.

Change-Id: I34127ac373ceadcfb6fc2662628b1a91eb7b0046
Closes-Bug: 1375487
(cherry picked from commit c42273fbc1983b146180c82b8a34b0d832a6f431)

diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py
index c7a0e3df2c001294acc3b1186cbe54fbd512b565..361e36fc1a02bef50a57a2da4daaca92dd946181 100644 (file)
--- a/cinder/image/image_utils.py
+++ b/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
-    # and it is needed to ensure that all data hit the device before
-    # it gets unmapped remotely from the host
+
     cmd = ('qemu-img', 'convert',
-           '-t', 'none',
            '-O', out_format, source, dest)
+
+    # 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

diff --git a/cinder/tests/test_image_utils.py b/cinder/tests/test_image_utils.py
index 150762d82c1a3e1b516111b6e3070ae04da64c56..86168c012c9082f6cccf4527a21dd6164a79f083 100644 (file)
--- a/cinder/tests/test_image_utils.py
+++ b/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)"
+
+        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')
 
-        self.test_upload_volume(bps_limit=1048576)
+        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',

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index f99238b10be806618dc062e7ed9b17cbd91efda8..b8acbf356907a4eea15cd9378bd734d383ca5771 100644 (file)
--- a/cinder/tests/test_volume.py
+++ b/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
 

diff --git a/cinder/tests/test_volume_utils.py b/cinder/tests/test_volume_utils.py
index 6179ee7f0f510b7f1279b5ac5a8e394017522afd..04d0fc58f39b3aace0920990c1aa48fcf28be390 100644 (file)
--- a/cinder/tests/test_volume_utils.py
+++ b/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)

diff --git a/cinder/utils.py b/cinder/utils.py
index 1073a20fa9935880606e84051498254034a3eb5e..8f27701c556da461c4df1292eda5505b4960f92c 100644 (file)
--- a/cinder/utils.py
+++ b/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

diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py
index d59dce8068e713b7b46a9774b80862c96e003e45..56f5b0c6208bc1dd41c2b67ceb8219e10b5d8f51 100644 (file)
--- a/cinder/volume/utils.py
+++ b/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
-    for flag in ['iflag=direct', 'oflag=direct']:
-        try:
-            execute('dd', 'count=0', 'if=%s' % srcstr, 'of=%s' % deststr,
-                    flag, run_as_root=True)
-            extra_flags.append(flag)
-        except processutils.ProcessExecutionError:
-            pass
+    if check_for_odirect_support(srcstr, deststr, 'iflag=direct'):
+        extra_flags.append('iflag=direct')
+
+    if check_for_odirect_support(srcstr, deststr, 'oflag=direct'):
+        extra_flags.append('oflag=direct')
 
     # If the volume is being unprovisioned then
     # request the data is persisted before returning,