Repository navigation
BCF-6879: Support Btrfs on multiple block devices - #100
Anastasiya Kapskaya (NastyaKapskaya) merged 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Relax-and-Recover’s Btrfs handling to support filesystems spanning multiple block devices by persisting the device list in the layout, restoring dependencies accordingly, and using that list during filesystem re-creation. It also refactors SELinux-permissive handling into a shared helper and adjusts GRUB editenv path detection.
Changes:
- Persist and consume a
devices=option for Btrfs filesystems to support multi-device layouts end-to-end. - Update layout dependency generation to account for additional Btrfs devices.
- Centralize SELinux “set permissive” logic into a shared library function and reuse it for RHEL/Fedora COVE finalize steps.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| usr/share/rear/lib/linux-functions.sh | Adds shared set_selinux_permissive helper for COVE finalize workflows. |
| usr/share/rear/lib/layout-functions.sh | Adds dependency tracking for additional Btrfs devices from devices= mount options. |
| usr/share/rear/lib/filesystems-functions.sh | Adds helpers to extract Btrfs device lists and validate them. |
| usr/share/rear/lib/bootloader-functions.sh | Adjusts how grub-editenv path is determined outside recovery mode. |
| usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh | Saves Btrfs device lists into layout output. |
| usr/share/rear/layout/save/default/950_verify_disklayout_file.sh | Validates the new devices= option in disklayout.conf. |
| usr/share/rear/layout/prepare/GNU/Linux/131_include_filesystem_code.sh | Uses the saved Btrfs device list when running mkfs during recovery. |
| usr/share/rear/finalize/COVE/RedHatEnterpriseServer/060_set_selinux_permissive.sh | Switches to the new shared SELinux helper. |
| usr/share/rear/finalize/COVE/Fedora/060_set_selinux_permissive.sh | Adds Fedora SELinux-permissive finalize step using the shared helper. |
| tests/COVE/005_filesystems_functions.bats | Adds tests for the new Btrfs helper functions and validator. |
Suppressed comments (1)
usr/share/rear/lib/filesystems-functions.sh:301
- The device-path validation regex is too restrictive: it rejects common real-world device paths like
/dev/mapper/vg-lvor/dev/disk/by-id/...because it disallows-,., and additional path segments. This will make valid Btrfs multi-device layouts fail verification/creation.
for path in "${paths[@]}"; do
if [[ ! "$path" =~ ^/dev/[/0-9a-zA-Z]+$ ]]; then
return 1
fi
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
usr/share/rear/lib/bootloader-functions.sh:1035
- Using
evalhere executes a shell string and re-parses it, which is unnecessary for this fixed command and makes the code harder to reason about (and riskier if the command string ever becomes influenced by environment/user input). You can avoidevalby runningtype -Pdirectly (it already supports||), keeping behavior identical without re-parsing.
GRUB_EDITENV_PATH="$(eval "$grub_editenv_path")"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
usr/share/rear/lib/layout-functions.sh:200
- The loop uses an undeclared
devicevariable, which becomes function-global in bash and can clobber other variables (or leak out of this function). Use a distinct local loop variable for the Btrfs member devices.
usr/share/rear/lib/linux-functions.sh:359 - This comment says the function changes the SELinux mode, but the implementation only edits
$TARGET_FS_ROOT/etc/selinux/config(it does not runsetenforce). Adjust the comment to reflect what actually happens to avoid misleading future changes.
usr/share/rear/lib/bootloader-functions.sh:1035
- Avoid
evalhere. Even though the string is currently constant,evalexpands and executes arbitrary shell syntax and is a common injection footgun; using the samebash -capproach as the chroot branch is safer and consistent.
GRUB_EDITENV_PATH="$(eval "$grub_editenv_path")"
d20b2d7 to
5a554c0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
usr/share/rear/lib/bootloader-functions.sh:1035
- Using eval here executes shell syntax from a string and is riskier than necessary. Since grub_editenv_path is already a shell command string (it includes
||), execute it via bash -c (consistent with the chroot branch) instead of eval.
GRUB_EDITENV_PATH="$(eval "$grub_editenv_path")"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
usr/share/rear/lib/bootloader-functions.sh:1035
- Avoid using
evalhere. Even thoughgrub_editenv_pathis currently a hardcoded string,evalexecutes in the current shell context (side effects, harder to audit) and is unnecessary for runningtype -P ... || type -P .... Use a subshell viabash -c(matching the recovery-mode branch) or invoketype -Pdirectly.
GRUB_EDITENV_PATH="$(eval "$grub_editenv_path")"
usr/share/rear/lib/linux-functions.sh:373
- The line-continuation indentation inside the quoted
textends up as literal spaces in the warning message (because the escaped newline is removed but the next line's leading spaces remain). This can make the message render with odd spacing. Prefer a single-line string (or concatenate without indentation) so the output is predictable.
local text="During the recovery process of ${os}-based systems, SELinux is set to permissive mode to \
successfully relabel the entire file system. After verifying that your system is functioning \
correctly, set SELINUX=enforcing in /etc/selinux/config to return SELinux to enforcing mode."
2532261 to
642816c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
usr/share/rear/lib/filesystems-functions.sh:290
read -adrops a trailing empty field, so a malformed value such as/dev/sda1,passes this validator.create_fsthen expands it to one device and silently omits the missing member instead of rejecting the layout; validate the complete comma-separated form before splitting.
IFS=',' read -ra paths <<< "$1"
642816c to
eab81f4
Compare
Storing the 'type -P grub-editenv || type -P grub2-editenv' command in the cmd variable does not work because parsing happens before expansion. By the time $cmd is expanded, '||' can no longer be interpreted as a control operator and it's treated as a word instead.
eab81f4 to
3ef1e8b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
usr/share/rear/finalize/COVE/RedHatEnterpriseServer/060_set_selinux_permissive.sh:7
OS_VENDORuses the internal valueRedHatEnterpriseServer, so this refactor changes the existing user-facing warning fromRHEL 10-based systemstoRedHatEnterpriseServer 10-based systems. Pass a display name for this caller so the warning remains understandable to RHEL users.
set_selinux_permissive "${OS_VENDOR} ${OS_VERSION%%.*}"
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:266
- The primary filesystem device is normalized with
get_device_mappingandget_device_namebefore it is written (lines 163-167), but the additional paths returned byget_btrfs_devicesare emitted raw. On a device-mapper or multipath member that Btrfs reports as/dev/dm-N, recovery can use an unstable or wrong member in the generatedmkfscommand. Normalize every returned member with the same mapping/name functions before serializingdevices=.
if devices=$(get_btrfs_devices "$mountpoint"); then
echo -n " devices=$devices"
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:274
- When profile discovery fails, omitting
dprofilemakes recovery use mkfs.btrfs's default data profile rather than the source profile, defeating the new profile-preservation behavior. This should fail the save (or otherwise prevent recovery from silently falling back to a different profile) instead of merely logging the error.
LogPrintError "Failed to get Btrfs data profile for $device"
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:280
- The same non-fatal handling here omits
mprofile, so recovery silently selects mkfs.btrfs's metadata default whenever discovery fails instead of restoring the source metadata profile. Treat this as a layout-save error rather than continuing with an incomplete Btrfs description.
LogPrintError "Failed to get Btrfs metadata profile for $device"
usr/share/rear/lib/filesystems-functions.sh:346
- The allocation profile directory names are not consistently lowercase: for example, metadata is exposed as
DUPand RAID profiles asRAID1/RAID10, while the mkfs profile names here are lowercase. This loop therefore misses normal metadataDUPand multi-device RAID profiles, omitsmprofile/dprofile, and lets recovery silently use mkfs defaults instead of the backed-up profile. Check the sysfs spelling and emit the normalized lowercase profile.
for profile in "${BTRFS_PROFILES[@]}"; do
if [ -d "$sysfs_alloc_path/$type/$profile" ]; then
echo "$profile"
usr/share/rear/lib/filesystems-functions.sh:279
- The primary filesystem device is normalized with
get_device_mapping/get_device_namebefore the fs entry is written (230_filesystem_layout.sh:152-167), but these additional paths are emitted exactly as reported bybtrfs filesystem show. For a device-mapper member reported as/dev/dm-*, the saved dependency and mkfs command will not match the/dev/mapper/*component name used elsewhere and recovery can wait forever or target the wrong path. Normalize each member path using the same mapping before joining the list.
devices="$(echo "$fs_info" | awk '$1 == "devid" && $(NF-1) == "path" {print $NF}')"
devices=${devices//$'\n'/,}
Save the list of Btrfs devices in disklayout.conf and set filesystem dependencies on all used devices. Pass the list to mkfs.btrfs when creating the filesystem.
A profile describes an allocation policy. It is essential to restore the Btrfs data and metadata profiles when a Btrfs filesystem is created on top of multiple block devices, as profiles configure data redundancy and replication, which typically involve multiple devices.
3ef1e8b to
5f829cc
Compare
Warn at backup time that seeding devices will become regular read-write devices at recovery time.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:266
- The primary filesystem device was normalized with
get_device_mapping/get_device_namebefore the layout line is written, butget_btrfs_devicesreturns the raw paths frombtrfs filesystem showand this option writes them unchanged. During disk migration/recovery an additional device can therefore remain a/dev/sdXor/dev/dm-*source name, while the new dependency and generatedmkfscommands need the mapped target name. Normalize every returned path before emittingdevices=(or in the helper, while preserving its library-loading contract).
if devices=$(get_btrfs_devices "$mountpoint"); then
echo -n " devices=$devices"
usr/share/rear/lib/filesystems-functions.sh:371
- This branch detects a mixed-mode filesystem, but only returns its data/metadata profile; no mixed-mode attribute is saved, and
create_fsalways emits separate-dand-moptions. A source created withmkfs.btrfs --mixedwill therefore be recreated with different allocation semantics. Persist the mixed-mode state and emitmkfs.btrfs -Mfor it instead of treating it as two independent profiles.
elif [ -d "$sysfs_alloc_path/mixed" ]; then
# In mixed mode /sys/fs/btrfs/UUID/allocation/mixed/ should be used
type=mixed
usr/share/rear/lib/filesystems-functions.sh:337
- This regex permits
..path components, so a value such as/dev/../etc/passwdpasses validation even though it escapes/dev. The value is later interpolated into the generatedmkfscommand, so malformed or edited layout data can make recovery operate on a path outside the device tree; reject parent-directory components (or canonicalize and enforce the/devprefix).
if [[ ! "$path" =~ ^/dev/[[:alnum:]/._-]+$ ]]; then
There was a problem hiding this comment.
🟡 Changes recommended
Recovery cleanup is skipped for some filesystem types, and older Btrfs layouts fail without the new devices option.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:266
- The member paths returned by
btrfs filesystem showare serialized verbatim, unlike the primary filesystem device which is normalized withget_device_mappingandget_device_namebefore being written. On layouts involving dm, multipath, or other alternate device names,devices=can therefore refer to components that do not match the normalizeddisk/partentries; dependency generation and migration then leave those members unrecreated or unreplaced. Normalize each returned member path before writing the list, as is done for RAID component paths.
if devices=$(get_btrfs_devices "$mountpoint"); then
echo -n " devices=$devices"
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
Multiple block group profiles appear during the Btrfs balance operation and the operation is aborted.
6812aab to
afa82e2
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Device normalization, mixed-mode preservation, and failed device discovery currently allow incorrect Btrfs recovery.
Review details
Suppressed comments (3)
usr/share/rear/lib/filesystems-functions.sh:371
- The
mixedallocation directory identifies a mixed data/metadata filesystem, but this branch returns only a normal profile name. The layout writer stores that asdprofile/mprofile, and the recovery code turns those into-d/-mwithoutmkfs.btrfs -M, so mixed-mode filesystems are recreated with separate data and metadata block groups. Preserve the mixed-mode flag separately and emit-Mduring recovery.
elif [ -d "$sysfs_alloc_path/mixed" ]; then
# In mixed mode /sys/fs/btrfs/UUID/allocation/mixed/ should be used
type=mixed
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:266
- These device paths are copied directly from
btrfs filesystem show, while the primary$deviceis canonicalized withget_device_mapping/get_device_namebefore this code. For device-mapper or multipath members this can preserve an unstable/dev/dm-Xor by-id path indevices=, and recovery will then use the wrong/nonexistent member for both dependencies andmkfs. Normalize every returned member with the same mapping sequence before writing the layout entry.
if devices=$(get_btrfs_devices "$mountpoint"); then
echo -n " devices=$devices"
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:268
- When device discovery fails, this only logs the problem and omits
devices=.create_fsthen initializesdevicesfrom the single primary$device, so a multi-device filesystem can produce a successful backup whose recovery recreates and formats only one member. Treat failure to obtain the member list as fatal (or otherwise make the layout verifier reject the entry) instead of silently falling back.
LogPrintError "Failed to get a list of Btrfs device paths for $mountpoint"
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
0e895d5
into
develop
enforcingtopermissiveon Fedora 44.grub-editenvpath on Fedora 44 where there is/sbin/type.Refs: