diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index c7a0e3df2c0..361e36fc1a0 100644 --- 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 150762d82c1..86168c012c9 100644 --- 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)" - 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', diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index f99238b10be..b8acbf35690 100644 --- 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 6179ee7f0f5..04d0fc58f39 100644 --- 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 1073a20fa99..8f27701c556 100644 --- 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 d59dce8068e..56f5b0c6208 100644 --- 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,