Skip to contentSkip to conversation

btrfs-progs: convert: Fix a bug in rollback check which overwrite return value

PATCH In review Qu Wenruo 2 participants 3 messages Nov 29, 2017 – Dec 7, 2017 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/2 btrfs-progs: convert: Fix a bug in rollback check which overwrite return value

Commit 1170ac307900 ("btrfs-progs: convert: Introduce function to check if convert image is able to be rolled back") reworked rollback check condition, by checking 1:1 mapping of each file extent.

The idea itself has nothing wrong, but error handler is not implemented correctly, which over writes the return value and always try to rollback the fs even it fails to pass the check.

Fix it by correctly return the error before rollback the fs.

Fixes 1170ac307900 ("btrfs-progs: convert: Introduce function to check if convert image is able to be rolled back")
---
 convert/main.c | 2 ++
 1 file changed, 2 insertions(+)
convert/main.c +2 -0
index af2855316fee..89f9261172ca 100644
@@ -1443,6 +1443,8 @@ next:
 		}
 	}
 	btrfs_release_path(&path);
+	if (ret)
+		return ret;
 	/*
 	 * For HOLES mode (without NO_HOLES), we must ensure file extents
 	 * cover the whole range of the image
Signature · 1 line
2.15.0
author
#1 lore raw

PATCH 2/2 btrfs-progs: tests/convert: Introduce test case to ensure btrfs-convert won't rollback the filesystem after balance

---
 .../015-not-to-rollback-after-balance/test.sh      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100755 tests/convert-tests/015-not-to-rollback-after-balance/test.sh
tests/convert-tests/015-not-to-rollback-after-balance/test.sh new file +33 -0
new file mode 100755
index 000000000000..0d523d2250db
@@ -0,0 +1,33 @@
+#!/bin/bash
+# Check if btrfs-convert refuse to rollback the filesystem, and leave
+# the btrfs and the convert image untouched
+
+source "$TOP/tests/common"
+source "$TOP/tests/common.convert"
+
+setup_root_helper
+prepare_test_dev
+check_prereq btrfs-convert
+check_global_prereq mke2fs
+
+# convert_test_prep_fs() will create large enough file inside the
+# test device, that's good enough for us to test rollback failure.
+convert_test_prep_fs ext4 mke2fs -t ext4 -b 4096
+run_check_umount_test_dev
+convert_test_do_convert "" 4096
+
+run_check_mount_test_dev
+
+# Now the fs is converted, balance the fs so later rollback should fail
+run_check $SUDO_HELPER "$TOP/btrfs" balance start --full-balance $TEST_MNT
+run_check_umount_test_dev
+
+# rollback should fail
+run_mustfail "rollback fs after balance" "$TOP/btrfs-convert" -r $TEST_DEV
+
+# Ensure the fs and convert image can pass the check
+run_check "$TOP/btrfs" check $TEST_DEV
+
+run_check_mount_test_dev
+run_check $SUDO_HELPER e2fsck -fn $TEST_MNT/ext2_saved/image
+run_check_umount_test_dev
Signature · 1 line
2.15.0
#2 lore raw
In reply to Qu Wenruo
Quoted 8 lines · Qu Wenruo

Commit 1170ac307900 ("btrfs-progs: convert: Introduce function to check if convert image is able to be rolled back") reworked rollback check condition, by checking 1:1 mapping of each file extent.

The idea itself has nothing wrong, but error handler is not implemented correctly, which over writes the return value and always try to rollback the fs even it fails to pass the check.

Quoted 5 lines

Fix it by correctly return the error before rollback the fs.

Fixes 1170ac307900 ("btrfs-progs: convert: Introduce function to check if convert image is able to be rolled back")

1-2 applied, thanks.