openstack
/
diskimage-builder
Code Issues Proposed changes

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 <andreas@florath.net>
This commit is contained in:
Andreas Florath 2017-09-16 09:43:16 +00:00 committed by Ian Wienand
parent dc215674f8
commit fa6c731132
4 changed files with 84 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
diskimage_builder/block_device/level3/mount.py
View File

@ -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):

95
diskimage_builder/block_device/tests/test_mount_order.py
View File

@ -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)

5
diskimage_builder/lib/disk-image-create
View File

@ -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?

11
diskimage_builder/lib/img-functions
View File

@ -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