Merge lp:~raharper/curtin/set_bcache_cache_mode into lp:~curtin-dev/curtin/trunk

Proposed by Ryan Harper
Status: Merged
Merged at revision: 292
Proposed branch: lp:~raharper/curtin/set_bcache_cache_mode
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 104 lines (+44/-1)
3 files modified
curtin/commands/block_meta.py (+37/-0)
examples/tests/mdadm_bcache.yaml (+1/-0)
tests/vmtests/test_mdadm_bcache.py (+6/-1)
To merge this branch: bzr merge lp:~raharper/curtin/set_bcache_cache_mode
Reviewer Review Type Date Requested Status
Scott Moser (community) Approve
Review via email: mp+276194@code.launchpad.net

Description of the change

Update curtin to write out cache mode for bcache devices if 'cache_mode' is present in storage config for bcache devices.
Update vmtest for bcache to confirm that we can set cache_mode; note it also confirms that the setting is persistent since we reboot between curtin install and vmtest validation.

Fixes: lp:1510334

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

one suggestion in line. other than that, looks good.
and if i'm wrong on that, then go ahead and commit anyway.

review: Approve
296. By Ryan Harper

Find bcache device name via backing device holders

Add function get_holders which reads /sys/class/block/<dev>/holders
Use get_holders to list the bcache backing device holders to find the
assigned bcache device name

297. By Ryan Harper

vmtest: switch bcache test to use check_file_stripped_line

Use common check function in bcache testcase

298. By Ryan Harper

make check fixes for bcache cache_mode changes

Apply cleanups as found by make check

299. By Ryan Harper

vmtest: switch bcache test to use regex

After looking at how stripped line output works, using a
regex is more flexible in the face of bcache adding/removing
cache modes.

Revision history for this message
Ryan Harper (raharper) wrote :

I've addressed two things:

1) Blake highlighted that relying the the bcache storage config id isn't robust; it could be any string though in practice it tends to the the correct value. Instead, use the backing device to look up the bcache device name via sysfs holders dir.

2) Switch testcase to use check_file_regex

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py 2015-10-26 10:02:13 +0000
+++ curtin/commands/block_meta.py 2015-10-30 13:40:20 +0000
@@ -155,6 +155,22 @@
155 util.subp(cmd, rcs=[0, 1, 2, 5], capture=True)155 util.subp(cmd, rcs=[0, 1, 2, 5], capture=True)
156156
157157
158def get_holders(devname):
159 if not devname:
160 return []
161
162 devname_sysfs = \
163 '/sys/class/block/{}/holders'.format(os.path.basename(devname))
164 if not os.path.exists(devname_sysfs):
165 err = ('No sysfs path to device holders:'
166 ' {}'.format(devname_sysfs))
167 LOG.error(err)
168 raise ValueError(err)
169
170 LOG.debug('Getting blockdev holders: {}'.format(devname_sysfs))
171 return os.listdir(devname_sysfs)
172
173
158def clear_holders(sys_block_path):174def clear_holders(sys_block_path):
159 holders = os.listdir(os.path.join(sys_block_path, "holders"))175 holders = os.listdir(os.path.join(sys_block_path, "holders"))
160 LOG.info("clear_holders running on '%s', with holders '%s'" %176 LOG.info("clear_holders running on '%s', with holders '%s'" %
@@ -882,6 +898,8 @@
882 storage_config)898 storage_config)
883 cache_device = get_path_to_storage_volume(info.get('cache_device'),899 cache_device = get_path_to_storage_volume(info.get('cache_device'),
884 storage_config)900 storage_config)
901 cache_mode = info.get('cache_mode', None)
902
885 if not backing_device or not cache_device:903 if not backing_device or not cache_device:
886 raise ValueError("backing device and cache device for bcache must be \904 raise ValueError("backing device and cache device for bcache must be \
887 specified")905 specified")
@@ -909,6 +927,25 @@
909 fp.write(path)927 fp.write(path)
910 fp.close()928 fp.close()
911929
930 if cache_mode:
931 # find the actual bcache device name via sysfs using the
932 # backing device's holders directory.
933 holders = get_holders(backing_device)
934
935 if len(holders) != 1:
936 err = ('Invalid number of holding devices:'
937 ' {}'.format(holders))
938 LOG.error(err)
939 raise ValueError(err)
940
941 [bcache_dev] = holders
942 LOG.info("Setting cache_mode on {} to {}".format(bcache_dev,
943 cache_mode))
944 cache_mode_file = \
945 '/sys/block/{}/bcache/cache_mode'.format(info.get('id'))
946 with open(cache_mode_file, "w") as fp:
947 fp.write(cache_mode)
948
912 if info.get('name'):949 if info.get('name'):
913 # Make dname rule for this dev950 # Make dname rule for this dev
914 make_dname(info.get('id'), storage_config)951 make_dname(info.get('id'), storage_config)
915952
=== modified file 'examples/tests/mdadm_bcache.yaml'
--- examples/tests/mdadm_bcache.yaml 2015-08-12 14:26:23 +0000
+++ examples/tests/mdadm_bcache.yaml 2015-10-30 13:40:20 +0000
@@ -46,6 +46,7 @@
46 name: cached_array46 name: cached_array
47 backing_device: mddevice47 backing_device: mddevice
48 cache_device: sda548 cache_device: sda5
49 cache_mode: writeback
49 - id: sda1_root50 - id: sda1_root
50 type: format51 type: format
51 fstype: ext452 fstype: ext4
5253
=== modified file 'tests/vmtests/test_mdadm_bcache.py'
--- tests/vmtests/test_mdadm_bcache.py 2015-10-26 10:39:59 +0000
+++ tests/vmtests/test_mdadm_bcache.py 2015-10-30 13:40:20 +0000
@@ -50,13 +50,15 @@
50 cd OUTPUT_COLLECT_D50 cd OUTPUT_COLLECT_D
51 bcache-super-show /dev/vda6 > bcache_super_vda651 bcache-super-show /dev/vda6 > bcache_super_vda6
52 ls /sys/fs/bcache > bcache_ls52 ls /sys/fs/bcache > bcache_ls
53 cat /sys/block/bcache0/bcache/cache_mode > bcache_cache_mode
53 """)]54 """)]
54 fstab_expected = {55 fstab_expected = {
55 '/dev/bcache0': '/media/data'56 '/dev/bcache0': '/media/data'
56 }57 }
5758
58 def test_bcache_output_files_exist(self):59 def test_bcache_output_files_exist(self):
59 self.output_files_exist(["bcache_super_vda6", "bcache_ls"])60 self.output_files_exist(["bcache_super_vda6", "bcache_ls",
61 "bcache_cache_mode"])
6062
61 def test_bcache_status(self):63 def test_bcache_status(self):
62 bcache_cset_uuid = None64 bcache_cset_uuid = None
@@ -68,6 +70,9 @@
68 with open(os.path.join(self.td.mnt, "bcache_ls"), "r") as fp:70 with open(os.path.join(self.td.mnt, "bcache_ls"), "r") as fp:
69 self.assertTrue(bcache_cset_uuid in fp.read().splitlines())71 self.assertTrue(bcache_cset_uuid in fp.read().splitlines())
7072
73 def test_bcache_cachemode(self):
74 self.check_file_regex("bcache_cache_mode", r"\[writeback\]")
75
7176
72class WilyTestMdadmBcache(TestMdadmBcacheAbs):77class WilyTestMdadmBcache(TestMdadmBcacheAbs):
73 __test__ = True78 __test__ = True

Subscribers

People subscribed via source and target branches