From 4e4a54d41ca8801f35d9988d0b8742bf354df049 Mon Sep 17 00:00:00 2001 From: Mitsuhiro Tanino Date: Fri, 8 May 2015 15:00:03 -0400 Subject: [PATCH] LVM: Support efficient data copy using "dd" for create_cloned_volume The create_cloned_volume uses dd command for data copy, but the copy always copy full blocks even if the source data contains many null or zero blocks. When we use thin provisioned LVM, blocks are not pre-allocated, so unused region returns zero. If we copy full block for destination volume, unnecessary blocks will be allocated and the usage will be 100%. The dd command has conv=sparse option in order to copy data more efficiently. This patch enables conv=sparse option as an argument of dd command for create_cloned_volume when we use thin provisioned LVM. [NOTE] This patch only enables conv=sparse parameter of dd command for create_cloned_volume() path of LVM driver. There are some places using dd in Cinder, but we should carefully consider to apply this parameter for other places because misuse of this parameter causes security issues or data corruptions. Also we DO NOT use this parameter for volume wiping case because the volume is not cleared at all. Here are some results for this option. - Without conv=sparse option LV VG Attr LSize Pool Origin Data% vg1-pool vg1 twi-a-tz-- 3.80g 31.45 volume-clone vg1 Vwi-a-tz-- 1.00g vg1-pool 100.00 volume-source vg1 Vwi-a-tz-- 1.00g vg1-pool 19.53 - With conv=sparse option LV VG Attr LSize Pool Origin Data% vg1-pool vg1 twi-a-tz-- 3.80g 10.28 volume-clone vg1 Vwi-a-tz-- 1.00g vg1-pool 19.53 volume-source vg1 Vwi-a-tz-- 1.00g vg1-pool 19.53 Change-Id: I743f823ca38529b12301a89308d1d406aa3fa45f Closes-bug: #1224671 --- cinder/tests/unit/test_volume_utils.py | 37 ++++++++++++++++++++++++-- cinder/volume/drivers/lvm.py | 4 ++- cinder/volume/utils.py | 19 +++++++++---- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py index cd49bf6b9..62500acf3 100644 --- a/cinder/tests/unit/test_volume_utils.py +++ b/cinder/tests/unit/test_volume_utils.py @@ -382,7 +382,7 @@ class ClearVolumeTestCase(test.TestCase): mock_copy.assert_called_once_with('/dev/zero', 'volume_path', 1024, '1M', sync=True, execute=utils.execute, ionice='-c3', - throttle=None) + throttle=None, sparse=False) @mock.patch('cinder.volume.utils.copy_volume', return_value=None) @mock.patch('cinder.volume.utils.CONF') @@ -397,7 +397,7 @@ class ClearVolumeTestCase(test.TestCase): mock_copy.assert_called_once_with('/dev/zero', 'volume_path', 1, '1M', sync=True, execute=utils.execute, ionice='-c0', - throttle=None) + throttle=None, sparse=False) @mock.patch('cinder.utils.execute') @mock.patch('cinder.volume.utils.CONF') @@ -523,6 +523,39 @@ class CopyVolumeTestCase(test.TestCase): 'count=5678', 'bs=1234', 'conv=fdatasync', run_as_root=True) + @mock.patch('cinder.volume.utils._calculate_count', + return_value=(1234, 5678)) + @mock.patch('cinder.volume.utils.check_for_odirect_support', + return_value=False) + @mock.patch('cinder.utils.execute') + def test_copy_volume_dd_with_sparse(self, mock_exec, + mock_support, mock_count): + output = volume_utils.copy_volume('/dev/zero', '/dev/null', 1024, 1, + sync=True, execute=utils.execute, + sparse=True) + self.assertIsNone(output) + mock_exec.assert_called_once_with('dd', 'if=/dev/zero', 'of=/dev/null', + 'count=5678', 'bs=1234', + 'conv=fdatasync,sparse', + run_as_root=True) + + @mock.patch('cinder.volume.utils._calculate_count', + return_value=(1234, 5678)) + @mock.patch('cinder.volume.utils.check_for_odirect_support', + return_value=True) + @mock.patch('cinder.utils.execute') + def test_copy_volume_dd_with_sparse_iflag_and_oflag(self, mock_exec, + mock_support, + mock_count): + output = volume_utils.copy_volume('/dev/zero', '/dev/null', 1024, 1, + sync=True, execute=utils.execute, + sparse=True) + self.assertIsNone(output) + mock_exec.assert_called_once_with('dd', 'if=/dev/zero', 'of=/dev/null', + 'count=5678', 'bs=1234', + 'iflag=direct', 'oflag=direct', + 'conv=sparse', run_as_root=True) + class VolumeUtilsTestCase(test.TestCase): def test_null_safe_str(self): diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 855a2df6d..2a6096842 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -412,12 +412,14 @@ class LVMVolumeDriver(driver.VolumeDriver): mirror_count) self.vg.activate_lv(temp_snapshot['name'], is_snapshot=True) + sparse = True if self.configuration.lvm_type == 'thin' else False volutils.copy_volume( self.local_path(temp_snapshot), self.local_path(volume), src_vref['size'] * units.Ki, self.configuration.volume_dd_blocksize, - execute=self._execute) + execute=self._execute, + sparse=sparse) finally: self.delete_snapshot(temp_snapshot) diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py index 405c8ef16..85fb84a79 100644 --- a/cinder/volume/utils.py +++ b/cinder/volume/utils.py @@ -279,7 +279,7 @@ def check_for_odirect_support(src, dest, flag='oflag=direct'): def _copy_volume(prefix, srcstr, deststr, size_in_m, blocksize, sync=False, - execute=utils.execute, ionice=None): + execute=utils.execute, ionice=None, sparse=False): # Use O_DIRECT to avoid thrashing the system buffer cache extra_flags = [] if check_for_odirect_support(srcstr, deststr, 'iflag=direct'): @@ -291,8 +291,14 @@ def _copy_volume(prefix, srcstr, deststr, size_in_m, blocksize, sync=False, # If the volume is being unprovisioned then # request the data is persisted before returning, # so that it's not discarded from the cache. + conv = [] if sync and not extra_flags: - extra_flags.append('conv=fdatasync') + conv.append('fdatasync') + if sparse: + conv.append('sparse') + if conv: + conv_options = 'conv=' + ",".join(conv) + extra_flags.append(conv_options) blocksize, count = _calculate_count(size_in_m, blocksize) @@ -326,13 +332,14 @@ def _copy_volume(prefix, srcstr, deststr, size_in_m, blocksize, sync=False, def copy_volume(srcstr, deststr, size_in_m, blocksize, sync=False, - execute=utils.execute, ionice=None, throttle=None): + execute=utils.execute, ionice=None, throttle=None, + sparse=False): if not throttle: throttle = throttling.Throttle.get_default() with throttle.subcommand(srcstr, deststr) as throttle_cmd: _copy_volume(throttle_cmd['prefix'], srcstr, deststr, size_in_m, blocksize, sync=sync, - execute=execute, ionice=ionice) + execute=execute, ionice=ionice, sparse=sparse) def clear_volume(volume_size, volume_path, volume_clear=None, @@ -353,12 +360,14 @@ def clear_volume(volume_size, volume_path, volume_clear=None, LOG.info(_LI("Performing secure delete on volume: %s") % volume_path) + # We pass sparse=False explicitly here so that zero blocks are not + # skipped in order to clear the volume. if volume_clear == 'zero': return copy_volume('/dev/zero', volume_path, volume_clear_size, CONF.volume_dd_blocksize, sync=True, execute=utils.execute, ionice=volume_clear_ionice, - throttle=throttle) + throttle=throttle, sparse=False) elif volume_clear == 'shred': clear_cmd = ['shred', '-n3'] if volume_clear_size: -- 2.45.2