BCF-7410: Backup and restore Btrfs options - #99
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for backing up and restoring Btrfs filesystem creation parameters (nodesize, sectorsize, and enabled filesystem features) so recovered systems can recreate Btrfs filesystems more faithfully.
Changes:
- Added helper functions to detect Btrfs version, available/enabled features, and to construct appropriate
mkfs.btrfsfeature flags. - Extended filesystem layout saving to record
nodesize=,sectorsize=, andfeatures=for Btrfs filesystems, and updated filesystem recreation to apply those parameters. - Added unit tests for the new filesystem helper functions and added
commas a required binary.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| usr/share/rear/lib/filesystems-functions.sh | Adds Btrfs feature/version detection and mkfs option construction helpers. |
| usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh | Persists Btrfs nodesize/sectorsize/features into the saved layout. |
| usr/share/rear/layout/prepare/GNU/Linux/131_include_filesystem_code.sh | Applies saved Btrfs nodesize/sectorsize/features during filesystem creation. |
| usr/share/rear/conf/default.conf | Includes comm to support feature list comparisons. |
| tests/COVE/005_filesystems_functions.bats | Adds unit tests covering the new Btrfs helper logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e8ead5f to
9d9897f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
usr/share/rear/lib/filesystems-functions.sh:171
- Runtime feature extraction uses
grep -Pwith a negative lookbehind. This adds a hard dependency on PCRE grep; using ERE withgrep -Eis sufficient here and avoids portability issues.
for runtime_feature in free-space-tree quota; do
local found
if found=$(echo "$features" | grep -oP '(?<![a-z])\^?'"$runtime_feature"',?'); then
features="${features/$found/}"
runtime_features+=",${found%,}"
fi
usr/share/rear/lib/filesystems-functions.sh:51
get_btrfs_versionrelies ongrep -P(PCRE lookbehind).grep -Pis not guaranteed to be available (e.g., BusyBox grep) and can cause version detection to fail even whenbtrfsis present. Consider using POSIX tools (awk) to extract the version, and also capture stderr to handle implementations that write version info there.
This issue also appears on line 166 of the same file.
function get_btrfs_version() {
if ! has_binary btrfs; then
return 127
fi
btrfs version | grep -oP '(?<=btrfs-progs v)[\d.]+'
}
9d02dc0 to
dc9038c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
usr/share/rear/lib/filesystems-functions.sh:152
- The error message here refers to “mkfs.btrfs”, but the new unit test expects “btrfs-progs” and (more importantly) the feature support depends on the btrfs-progs version as a whole. This mismatch will fail the added Bats test and is inconsistent terminology.
LogPrintError "'$unsupported_feature' is an unsupported Btrfs filesystem feature in the version of mkfs.btrfs used by the rescue system and cannot be recovered."
usr/share/rear/layout/save/GNU/Linux/230_filesystem_layout.sh:268
- If get_enabled_btrfs_features returns an empty string (but exits 0), the layout will still emit
features=. During restore,create_fs()treats an empty features value as “disable all features” (see get_btrfs_features_option_for_mkfs), which can create a filesystem with unintended feature flags. Only persist the features option when it is non-empty.
if features=$(get_enabled_btrfs_features "$uuid" ); then
echo -n " features=$features"
else
usr/share/rear/lib/filesystems-functions.sh:50
- This uses
grep -P(PCRE) with lookbehind, which is a GNU grep feature that is not consistently available across all environments (and is not used elsewhere in this repo). Parsing the version with awk avoids introducing a PCRE dependency while preserving the current “exit 1 on unexpected output” behavior.
btrfs version | grep -oP '(?<=btrfs-progs v)[\d.]+'
960e645 to
6d6cda7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
usr/share/rear/layout/save/default/950_verify_disklayout_file.sh:230
broken_btrfs_errorsis appended to in this loop and later iterated, but it is never initialized like the otherbroken_*_errorsarrays. Ifnounsetis enabled or the variable is set in the environment, this can cause failures or leakage between runs. Initialize it to an empty local array before the loop.
Log "Verifying that the Btrfs filesystem entries in $DISKLAYOUT_FILE are correct"
while read -r _ device _ _ _ _ options; do
usr/share/rear/layout/prepare/GNU/Linux/131_include_filesystem_code.sh:287
- This backward-compat fallback hard-codes
-R ^free-space-tree, but your own logic elsewhere notes that-Ris deprecated since btrfs-progs 6.3. On newer rescue media where-Ris removed/unsupported, filesystem creation would fail for older sessions. Prefer selecting-Ronly for the 5.7–6.2 window and use-Ootherwise.
if [ -z "$features" ] && [ "$BACKUP" = "COVE" ] && [[ ! "$options" == *"space_cache=v2"* ]]; then
features=" -R ^free-space-tree"
fi
tests/COVE/005_filesystems_functions.bats:540
- This test case is named as a nodesize validation test, but it calls
is_btrfs_sectorsize_valid, so it doesn't actually test the nodesize validator against non-numeric input.
@test "Is Btrfs nodesize valid: nodesize containing non-numeric characters is invalid" {
run -1 is_btrfs_sectorsize_valid "16384;rm -rf /"
}
c72feeb to
4bbac32
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
usr/share/rear/lib/filesystems-functions.sh:234
is_btrfs_sectorsize_validhas the same arithmetic-parsing issue asis_btrfs_nodesize_valid: non-numeric input triggers a bash arithmetic syntax error to stderr. Guarding for numeric-only input avoids noisy error output during disklayout verification/recovery.
function is_btrfs_sectorsize_valid() {
local sectorsize=$1
(( sectorsize > 0 && (sectorsize & (sectorsize - 1)) == 0 ))
}
usr/share/rear/lib/filesystems-functions.sh:50
get_btrfs_versionrelies ongrep -Pwith a lookbehind.grep -P(PCRE) is not guaranteed to be available in all rescue environments, which can cause version detection to fail and then break the feature handling logic that depends on it. Consider parsing the version usingawk/sedwithout PCRE so it works with non-GNU grep implementations too.
if ! has_binary btrfs; then
return 127
fi
btrfs version | grep -oP '(?<=btrfs-progs v)[\d.]+'
usr/share/rear/lib/filesystems-functions.sh:177
get_btrfs_features_option_for_mkfsusesgrep -Pwith a negative lookbehind to extract runtime features. This makes the feature split dependent on PCRE support, even though the same parsing can be done reliably in bash by splitting on commas. Avoidinggrep -Phere reduces portability risk and removes a relatively complex regex.
local runtime_features=""
for runtime_feature in free-space-tree quota; do
local found
if found=$(echo "$features" | grep -oP '(?<![a-z])\^?'"$runtime_feature"',?'); then
features="${features/$found/}"
usr/share/rear/lib/filesystems-functions.sh:78
get_available_btrfs_featuressilently skips adding runtime features whenget_btrfs_versionfails (e.g., unexpectedbtrfs versionoutput). On btrfs-progs 5.7–6.2 this can lead to an incomplete feature list (missing e.g.quota/free-space-tree) without any error, which then affects restore-time mkfs options.
# We need to get runtime features using mkfs.btrfs -R list-all for versions
# between 5.7 and 6.2. Since 6.3, the -R option has been deprecated,
# and all features have been merged into the -O option.
if printf '%s\n' "5.7" "$(get_btrfs_version)" "6.2" | sort -V -C; then
if ! buffer="$(mkfs.btrfs -R list-all 2>&1)"; then
usr/share/rear/lib/filesystems-functions.sh:228
is_btrfs_nodesize_validuses bash arithmetic directly on the input. When the value contains non-numeric characters, bash emits an arithmetic syntax error to stderr, which can leak confusing messages into logs. Adding a simple numeric guard avoids that noise and keeps validation behavior predictable.
This issue also appears on line 231 of the same file.
function is_btrfs_nodesize_valid() {
local nodesize=$1
# The nodesize must be not larger than 64KiB and a power of 2
(( nodesize > 0 && nodesize <= 65536 && (nodesize & (nodesize - 1)) == 0 ))
}
tests/COVE/005_filesystems_functions.bats:550
- Test title says "nodesize" but the test actually validates sectorsize (calls
is_btrfs_sectorsize_valid). Updating the description avoids confusion when reading test output.
@test "Is Btrfs nodesize valid: sectorsize must be a power of 2" {
run -1 is_btrfs_sectorsize_valid 1023
4bbac32 to
974f88c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
usr/share/rear/lib/filesystems-functions.sh:133
get_enabled_btrfs_featurescurrently errors out ifget_available_btrfs_featuresfails. That means a system withoutmkfs.btrfs(or with a version-detection issue) will not save any Btrfs feature list, even though the enabled features can still be read from sysfs. It’s safer to keep the sysfs-derived list and only filter it when the available-features list is actually obtainable.
# Filter out filesystem features that can't be passed to the mkfs.btrfs -O option
local available_features
if ! available_features=$(get_available_btrfs_features); then
LogPrintError "Failed to get the list of available Btrfs features to filter out enabled features for the filesystem with UUID $uuid."
return 1
usr/share/rear/lib/filesystems-functions.sh:51
get_btrfs_versioncurrently hard-requires thebtrfsbinary, but the feature detection code primarily depends onmkfs.btrfs. Ifmkfs.btrfsis present butbtrfsis not, version detection fails and feature handling degrades unnecessarily. Consider falling back tomkfs.btrfs --versionwhenbtrfsis missing.
This issue also appears on line 129 of the same file.
function get_btrfs_version() {
if ! has_binary btrfs; then
return 127
fi
btrfs version | grep -oP '(?<=btrfs-progs v)[\d.]+'
}
Backup and restore Btrfs nodesize, sectorsize and filesystem features Jira-Ref: BCF-7410: Backup and restore Btrfs options
19547dd to
fcc4242
Compare
Jira-Ref: BCF-7410: Backup and restore Btrfs options
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
usr/share/rear/lib/filesystems-functions.sh:77
- The version range check for deciding when to use
mkfs.btrfs -R list-allincorrectly excludes patch releases like6.2.1(because it compares against the literal upper bound6.2). For6.2.x, this will skip the-Rquery and miss runtime features, leading to incomplete feature detection.
# We need to get runtime features using mkfs.btrfs -R list-all for versions
# between 5.7 and 6.2. Since 6.3, the -R option has been deprecated,
# and all features have been merged into the -O option.
local btrfs_version
if ! btrfs_version=$(get_btrfs_version); then
LogPrintError "Failed to determine the Btrfs version to check whether mkfs.btrfs -R list-all must be used."
return 1
fi
if printf '%s\n' "5.7" "$btrfs_version" "6.2" | sort -V -C; then
if ! buffer="$(mkfs.btrfs -R list-all 2>&1)"; then
LogPrintError "Failed to get the list of available Btrfs runtime features using mkfs.btrfs -R list-all."
return 1
fi
features+=$'\n'
features+="$(echo "$buffer" | awk 'NR>1 {print $1}')"
fi
tests/COVE/005_filesystems_functions.bats:29
- This test uses
run -127 get_btrfs_version. In bats-core,rundoes not take an expected-exit-code argument; passing-127is treated as an option and will make the test fail to execute. Other tests intests/COVE/*.batsuserun <cmd>and then assert on$status.
@test "Check Btrfs version: btrfs is missing" {
function has_binary() {
[ "$1" != "btrfs" ]
}
run -127 get_btrfs_version
}
usr/share/rear/lib/filesystems-functions.sh:180
- The version range check for splitting runtime features into
-Roptions has the same issue as above:6.2.1and other6.2.xversions will not be treated as needing-R, even though the-Rdeprecation starts at6.3. This can generate the wrong mkfs option set for5.7 <= version < 6.3.
# For versions 5.7 through 6.2, runtime features must be controlled using the -R option.
local btrfs_version
if ! btrfs_version=$(get_btrfs_version); then
LogPrintError "Failed to determine the Btrfs version to check whether mkfs.btrfs -R list-all must be used."
return 1
fi
if printf '%s\n' "5.7" "$btrfs_version" "6.2" | sort -V -C; then
local runtime_features=""
for runtime_feature in free-space-tree quota; do
local found
if found=$(echo "$features" | grep -oP '(?<![a-z])\^?'"$runtime_feature"',?'); then
features="${features/$found/}"
runtime_features+=",${found%,}"
fi
done
if [ -n "$runtime_features" ]; then
result+=" -R ${runtime_features#,}"
fi
features=${features%,}
fi
Jira-Ref: BCF-7410: Backup and restore Btrfs options
b2b4898 to
84b42b8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
usr/share/rear/lib/filesystems-functions.sh:161
- When removing unsupported features, the sed substitution can leave consecutive commas when the unsupported feature is in the middle of the comma-separated list (e.g. "extref,mixed-bg,no-holes" -> "extref,,no-holes"). That can later result in an invalid mkfs.btrfs feature list being emitted (empty feature between commas). Clean up duplicate/leading/trailing commas after the removal.
for unsupported_feature in $unsupported_features; do
LogPrintError "'$unsupported_feature' is an unsupported Btrfs filesystem feature in the version of mkfs.btrfs used by the rescue system and cannot be recovered."
# Remove unsupported features caused by using an older mkfs.btrfs on the rescue system than mkfs.btrfs used to create a filesystem.
features=$(echo "$features" | sed -E "s/(^|,)${unsupported_feature}(,|$)/\1/;s/,$//")
done
Backup and restore Btrfs nodesize, sectorsize and filesystem features