[PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device

[PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device

[Qu Wenruo]

Can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/btrfs_image_fix

Bug report:
https://github.com/kdave/btrfs-progs/issues/118

In short, the problem is caused by some old code (read_extent_data()
from ancient btrfs check code) and offset-by-one from btrfs-image.
Which makes btrfs-image can only read from the first stripe of RAID1.

And if device of the first stripe is missing, btrfs-image will fail.

Fix the problem and add test case for it.

Qu Wenruo (3):
  btrfs-progs: disk-io: Fix read_extent_data() error handler for missing
    device
  btrfs-progs: convert: Fix offset-by-one error in read_data_extent()
  btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing
    device

 disk-io.c                                         |  6 ++-
 image/main.c                                      |  2 +-
 tests/misc-tests/030-missing-device-image/test.sh | 57 +++++++++++++++++++++++
 3 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100755 tests/misc-tests/030-missing-device-image/test.sh

-- 
2.16.3

[PATCH 1/3] btrfs-progs: disk-io: Fix read_extent_data() error handler for missing device

[Qu Wenruo]

When device is missing, read_extent_data() (function exported from old
btrfs check code) has the following problems:

1) Modify @len parameter if device is missing
   If device returned in @multi is missing, @len can be larger than
   @max_len (originl length).

   This could confusing caller and underflow the read loop.

2) Still return 0 for missing device
   It only handles read error, missing device is not handled and 0 is
   returned.

3) Wrong check for device->fd
   In fact, 0 is also a valid fd.
   Although not possible under most case, it's still need fix.

Fix them all.

Fixes: 1bad2f2f2dfe ("Btrfs-progs: fsck: add an option to check data csums")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 disk-io.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 610963357675..310ab19cf099 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -395,10 +395,12 @@ int read_extent_data(struct btrfs_fs_info *fs_info, char *data, u64 logical,
 	}
 	device = multi->stripes[0].dev;
 
-	if (device->fd <= 0)
-		goto err;
 	if (*len > max_len)
 		*len = max_len;
+	if (device->fd < 0) {
+		ret = -EIO;
+		goto err;
+	}
 
 	ret = pread64(device->fd, data, *len, multi->stripes[0].physical);
 	if (ret != *len)
-- 
2.16.3

[PATCH 2/3] btrfs-progs: convert: Fix offset-by-one error in read_data_extent()

[Qu Wenruo]

For read_data_extent() in convert/main.c it's using mirror number in a
incorrect way, which will not get correct copy for RAID1:
------
	for (cur_mirror = 0; cur_mirror < num_copies; cur_mirror++) {
------

In such case, for RAID1 @cur_mirror will only be 0 and 1.

However for 0 and 1 case, btrfs_map_block() will only return the first
copy.
To reach the 2nd copy, it correct @cur_mirror range should be 1 and 2.

So with this offset-by-one error, btrfs-image will never be able to read
out data extent if the first stripe of the chunk is the missing one.

Fix it by starting @cur_mirror from 1 and to @num_copies (including).

Fixes: 2d46558b30f5 ("btrfs-progs: Use existing facility to replace read_data_extent function")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/image/main.c b/image/main.c
index 5f155c23dce6..351c5a256938 100644
--- a/image/main.c
+++ b/image/main.c
@@ -571,7 +571,7 @@ static int read_data_extent(struct metadump_struct *md,
 	num_copies = btrfs_num_copies(root->fs_info, logical, bytes_left);
 
 	/* Try our best to read data, just like read_tree_block() */
-	for (cur_mirror = 0; cur_mirror < num_copies; cur_mirror++) {
+	for (cur_mirror = 1; cur_mirror <= num_copies; cur_mirror++) {
 		while (bytes_left) {
 			read_len = bytes_left;
 			ret = read_extent_data(fs_info,
-- 
2.16.3

[PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/misc-tests/030-missing-device-image/test.sh | 57 +++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100755 tests/misc-tests/030-missing-device-image/test.sh

diff --git a/tests/misc-tests/030-missing-device-image/test.sh b/tests/misc-tests/030-missing-device-image/test.sh
new file mode 100755
index 000000000000..b8ae3a950cc9
--- /dev/null
+++ b/tests/misc-tests/030-missing-device-image/test.sh
@@ -0,0 +1,57 @@
+#!/bin/bash
+# Test that btrfs-image can dump image correctly for missing device (RAID1)
+#
+# At least for RAID1, btrfs-image should be able to handle one missing device
+# without any problem
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs-image
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+setup_loopdevs 2
+prepare_loopdevs
+dev1=${loopdevs[1]}
+dev2=${loopdevs[2]}
+
+# $1:	device number to remove (either 1 or 2)
+tmp=$(mktemp --tmpdir -d btrfs-progs-misc-test-XXXXXXX)
+test_missing()
+{
+	bad_num=$1
+	bad_dev=${loopdevs[$bad_num]}
+	good_num=$((3 - $bad_num))
+	good_dev=${loopdevs[$good_num]}
+
+	run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f -d raid1 -m raid1 \
+		"$dev1" "$dev2"
+	
+	# fill the fs with some data, we could create space cache
+	run_check $SUDO_HELPER mount "$dev1" "$TEST_MNT"
+	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/a" bs=1M count=10
+	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/b" bs=4k count=1000 conv=sync
+	run_check $SUDO_HELPER umount "$TEST_MNT"
+	
+	# make sure we have space cache
+	run_check_stdout "$TOP/btrfs" inspect dump-tree -t root "$dev1" \
+		> "$tmp/output"
+	if ! grep -q "EXTENT_DATA" "$tmp/output" ; then
+		# normally above operation should create space cache.
+		# if not, it may means we have migrated to v2 cache by default
+		_not_run "unable to create v1 space cache"
+	fi
+
+	# now wipe the device
+	run_check wipefs -fa "$bad_dev"
+
+	# we don't care about the image but btrfs-image must not fail
+	run_check "$TOP/btrfs-image" "$good_dev" /dev/null
+}
+
+# Test with either device missing, so we're ensured to hit missing device
+test_missing 1
+test_missing 2
+cleanup_loopdevs
+rm $tmp -rf
-- 
2.16.3

Re: [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device

[David Sterba]

On Fri, Mar 30, 2018 at 03:35:28PM +0800, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Applied with the fixes below, thanks.

> ---
>  tests/misc-tests/030-missing-device-image/test.sh | 57 +++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100755 tests/misc-tests/030-missing-device-image/test.sh
> 
> diff --git a/tests/misc-tests/030-missing-device-image/test.sh b/tests/misc-tests/030-missing-device-image/test.sh
> new file mode 100755
> index 000000000000..b8ae3a950cc9
> --- /dev/null
> +++ b/tests/misc-tests/030-missing-device-image/test.sh
> @@ -0,0 +1,57 @@
> +#!/bin/bash
> +# Test that btrfs-image can dump image correctly for missing device (RAID1)
> +#
> +# At least for RAID1, btrfs-image should be able to handle one missing device
> +# without any problem
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq btrfs-image
> +check_prereq mkfs.btrfs
> +check_prereq btrfs
> +
> +setup_root_helper
> +setup_loopdevs 2
> +prepare_loopdevs
> +dev1=${loopdevs[1]}
> +dev2=${loopdevs[2]}
> +
> +# $1:	device number to remove (either 1 or 2)
> +tmp=$(mktemp --tmpdir -d btrfs-progs-misc-test-XXXXXXX)

Not necessary.

> +test_missing()
> +{
> +	bad_num=$1
> +	bad_dev=${loopdevs[$bad_num]}
> +	good_num=$((3 - $bad_num))
> +	good_dev=${loopdevs[$good_num]}

All of them should be declared as local

> +
> +	run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f -d raid1 -m raid1 \
> +		"$dev1" "$dev2"
> +	
> +	# fill the fs with some data, we could create space cache
> +	run_check $SUDO_HELPER mount "$dev1" "$TEST_MNT"
> +	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/a" bs=1M count=10
> +	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/b" bs=4k count=1000 conv=sync
> +	run_check $SUDO_HELPER umount "$TEST_MNT"
> +	
> +	# make sure we have space cache
> +	run_check_stdout "$TOP/btrfs" inspect dump-tree -t root "$dev1" \
> +		> "$tmp/output"
> +	if ! grep -q "EXTENT_DATA" "$tmp/output" ; then

this can be glued to one line, making the tmp files unnecessry

> +		# normally above operation should create space cache.
> +		# if not, it may means we have migrated to v2 cache by default
> +		_not_run "unable to create v1 space cache"
> +	fi
> +
> +	# now wipe the device
> +	run_check wipefs -fa "$bad_dev"
> +
> +	# we don't care about the image but btrfs-image must not fail
> +	run_check "$TOP/btrfs-image" "$good_dev" /dev/null
> +}
> +
> +# Test with either device missing, so we're ensured to hit missing device
> +test_missing 1
> +test_missing 2
> +cleanup_loopdevs
> +rm $tmp -rf

Re: [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device

[David Sterba]

On Fri, Mar 30, 2018 at 03:35:25PM +0800, Qu Wenruo wrote:
> Can be fetched from github:
> https://github.com/adam900710/btrfs-progs/tree/btrfs_image_fix
> 
> Bug report:
> https://github.com/kdave/btrfs-progs/issues/118
> 
> In short, the problem is caused by some old code (read_extent_data()
> from ancient btrfs check code) and offset-by-one from btrfs-image.
> Which makes btrfs-image can only read from the first stripe of RAID1.
> 
> And if device of the first stripe is missing, btrfs-image will fail.
> 
> Fix the problem and add test case for it.
> 
> Qu Wenruo (3):
>   btrfs-progs: disk-io: Fix read_extent_data() error handler for missing
>     device
>   btrfs-progs: convert: Fix offset-by-one error in read_data_extent()
>   btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing
>     device

1-3 applied, thanks. I'll reorder the uuid tree patch after those
patches so the test does not fail.