From fa6c731132f15acdeb1d180f813387281082a10c Mon Sep 17 00:00:00 2001 From: Andreas Florath Date: Sat, 16 Sep 2017 09:43:16 +0000 Subject: [PATCH] Move fstrim to block device layer The call to fstrim in disk-image-create is currently useless, because at the time this is called, the file systems were already umounted by the block device layer. The current implementation of the block-device mount plugin does not call fstrim at all: resulting in larger image sizes. This patch removes the useless fstrim call from the disk-image-create script and moves this into the block-device mount.py. The resulting image might be much smaller. Example: Ubuntu Xenial with some elements; once with and once without this patch: -rw-r--r-- 1 dib dib 475661824 Sep 16 06:43 ubuntu-xenial-without-fstrim.qcow2 -rw-r--r-- 1 dib dib 364249088 Sep 16 09:30 ubuntu-xenial-with-fstrim.qcow2 Change-Id: I4e21ae50c5e6e26dc9f50f004ed6413132c81047 Signed-off-by: Andreas Florath --- .../block_device/level3/mount.py | 11 +++ .../block_device/tests/test_mount_order.py | 95 ++++++++++++++----- diskimage_builder/lib/disk-image-create | 5 - diskimage_builder/lib/img-functions | 11 --- 4 files changed, 84 insertions(+), 38 deletions(-) diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py index 47fa785d..1d653d02 100644 --- a/diskimage_builder/block_device/level3/mount.py +++ b/diskimage_builder/block_device/level3/mount.py @@ -95,6 +95,17 @@ class MountPointNode(NodeBase): def umount(self): logger.info("Called for [%s]", self.name) + # Before calling umount call 'fstrim' on the mounted file + # system. This discards unused blocks from the mounted + # file system and therefore decreases the resulting image + # size. + # A race condition can occur when trying to fstrim immediately after + # deleting a file resulting in that free space not being reclaimed. + # Calling sync before fstrim is a workaround for this behaviour. + # https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02978.html + exec_sudo(["sync"]) + exec_sudo(["fstrim", "--verbose", + self.state['mount'][self.mount_point]['path']]) exec_sudo(["umount", self.state['mount'][self.mount_point]['path']]) def delete(self): diff --git a/diskimage_builder/block_device/tests/test_mount_order.py b/diskimage_builder/block_device/tests/test_mount_order.py index bd7c97ae..20403941 100644 --- a/diskimage_builder/block_device/tests/test_mount_order.py +++ b/diskimage_builder/block_device/tests/test_mount_order.py @@ -13,6 +13,7 @@ import functools import logging import mock +import os import diskimage_builder.block_device.tests.test_config as tc @@ -51,39 +52,67 @@ class TestMountComparator(TestBase): class TestMountOrder(tc.TestGraphGeneration): - @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo') + def _exec_sudo_log(*args, **kwargs): + # Used as a side-effect from exec_sudo mocking so we can see + # the call in-place in logs + logger.debug("exec_sudo: %s", " ".join(args[0])) + + @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo', + side_effect=_exec_sudo_log) def test_mount_order(self, mock_exec_sudo): + # XXX: better mocking for the os.path.exists calls to avoid + # failing if this exists. + self.assertFalse(os.path.exists('/fake/')) + # This is probably in order after graph creation, so ensure it - # remains stable + # remains stable. We test the mount and umount call sequences config = self.load_config_file('multiple_partitions_graph.yaml') - state = {} - graph, call_order = create_graph(config, self.fake_default_config, state) # build up some fake state so that we don't have to mock out # all the parent calls that would really make these values, as # we just want to test MountPointNode - state['filesys'] = {} - state['filesys']['mkfs_root'] = {} - state['filesys']['mkfs_root']['device'] = 'fake' - state['filesys']['mkfs_var'] = {} - state['filesys']['mkfs_var']['device'] = 'fake' - state['filesys']['mkfs_var_log'] = {} - state['filesys']['mkfs_var_log']['device'] = 'fake' + state['filesys'] = { + 'mkfs_root': {'device': 'fake_root_device'}, + 'mkfs_var': {'device': 'fake_var_device'}, + 'mkfs_var_log': {'device': 'fake_var_log_device'} + } for node in call_order: if isinstance(node, MountPointNode): - # XXX: do we even need to create? We could test the - # sudo arguments from the mock in the below asserts - # too node.create() + for node in reversed(call_order): + if isinstance(node, MountPointNode): + node.umount() # ensure that partitions are mounted in order root->var->var/log self.assertListEqual(state['mount_order'], ['/', '/var', '/var/log']) - @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo') + cmd_sequence = [ + # mount sequence + mock.call(['mkdir', '-p', '/fake/']), + mock.call(['mount', 'fake_root_device', '/fake/']), + mock.call(['mkdir', '-p', '/fake/var']), + mock.call(['mount', 'fake_var_device', '/fake/var']), + mock.call(['mkdir', '-p', '/fake/var/log']), + mock.call(['mount', 'fake_var_log_device', '/fake/var/log']), + # umount sequence + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/var/log']), + mock.call(['umount', '/fake/var/log']), + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/var']), + mock.call(['umount', '/fake/var']), + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/']), + mock.call(['umount', '/fake/']) + ] + self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence) + + @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo', + side_effect=_exec_sudo_log) def test_mount_order_unsorted(self, mock_exec_sudo): # As above, but this is out of order and gets sorted # so that root is mounted first. @@ -94,17 +123,39 @@ class TestMountOrder(tc.TestGraphGeneration): graph, call_order = create_graph(parsed_graph, self.fake_default_config, state) - state['filesys'] = {} - state['filesys']['mkfs_root'] = {} - state['filesys']['mkfs_root']['device'] = 'fake' - state['filesys']['mkfs_var'] = {} - state['filesys']['mkfs_var']['device'] = 'fake' - state['filesys']['mkfs_boot'] = {} - state['filesys']['mkfs_boot']['device'] = 'fake' + state['filesys'] = { + 'mkfs_root': {'device': 'fake_root_device'}, + 'mkfs_var': {'device': 'fake_var_device'}, + 'mkfs_boot': {'device': 'fake_boot_device'} + } for node in call_order: if isinstance(node, MountPointNode): node.create() + for node in reversed(call_order): + if isinstance(node, MountPointNode): + node.umount() # ensure that partitions are mounted in order / -> /boot -> /var self.assertListEqual(state['mount_order'], ['/', '/boot', '/var']) + + cmd_sequence = [ + # mount sequence + mock.call(['mkdir', '-p', '/fake/']), + mock.call(['mount', 'fake_root_device', '/fake/']), + mock.call(['mkdir', '-p', '/fake/boot']), + mock.call(['mount', 'fake_boot_device', '/fake/boot']), + mock.call(['mkdir', '-p', '/fake/var']), + mock.call(['mount', 'fake_var_device', '/fake/var']), + # umount sequence + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/var']), + mock.call(['umount', '/fake/var']), + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/boot']), + mock.call(['umount', '/fake/boot']), + mock.call(['sync']), + mock.call(['fstrim', '--verbose', '/fake/']), + mock.call(['umount', '/fake/']) + ] + self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence) diff --git a/diskimage_builder/lib/disk-image-create b/diskimage_builder/lib/disk-image-create index cb36d3f5..b624a51d 100644 --- a/diskimage_builder/lib/disk-image-create +++ b/diskimage_builder/lib/disk-image-create @@ -480,11 +480,6 @@ for X in ${!IMAGE_TYPES[@]} ; do fi done -if [[ ! $IMAGE_ELEMENT =~ no-final-image ]]; then - # Prep filesystem by discarding all unused space - fstrim_image -fi - # Unmount and cleanup the /mnt and /build subdirectories, to save # space before converting the image to some other format. # XXX ? needed? diff --git a/diskimage_builder/lib/img-functions b/diskimage_builder/lib/img-functions index e08d4a4d..e43f7304 100644 --- a/diskimage_builder/lib/img-functions +++ b/diskimage_builder/lib/img-functions @@ -32,17 +32,6 @@ function unmount_image () { fi } -function fstrim_image () { - # A race condition can occur when trying to fstrim immediately after - # deleting a file resulting in that free space not being reclaimed. - # Calling sync before fstrim is a workaround for this behaviour. - # https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02978.html - sync - - # Discard all unused bytes - sudo fstrim "${TMP_BUILD_DIR}/mnt" -} - function trap_cleanup() { exitval=$? cleanup