* [PATCH 1/3] btrfs-progs: disk-io: Fix read_extent_data() error handler for missing device
2018-03-30 7:35 [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device Qu Wenruo
@ 2018-03-30 7:35 ` Qu Wenruo
2018-03-30 7:35 ` [PATCH 2/3] btrfs-progs: convert: Fix offset-by-one error in read_data_extent() Qu Wenruo
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-03-30 7:35 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] btrfs-progs: convert: Fix offset-by-one error in read_data_extent()
2018-03-30 7:35 [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device Qu Wenruo
2018-03-30 7:35 ` [PATCH 1/3] btrfs-progs: disk-io: Fix read_extent_data() error handler for " Qu Wenruo
@ 2018-03-30 7:35 ` Qu Wenruo
2018-03-30 7:35 ` [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device Qu Wenruo
2018-03-30 20:15 ` [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle " David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-03-30 7:35 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device
2018-03-30 7:35 [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device Qu Wenruo
2018-03-30 7:35 ` [PATCH 1/3] btrfs-progs: disk-io: Fix read_extent_data() error handler for " Qu Wenruo
2018-03-30 7:35 ` [PATCH 2/3] btrfs-progs: convert: Fix offset-by-one error in read_data_extent() Qu Wenruo
@ 2018-03-30 7:35 ` Qu Wenruo
2018-03-30 20:14 ` David Sterba
2018-03-30 20:15 ` [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle " David Sterba
3 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2018-03-30 7:35 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device
2018-03-30 7:35 ` [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device Qu Wenruo
@ 2018-03-30 20:14 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2018-03-30 20:14 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device
2018-03-30 7:35 [PATCH 0/3] btrfs-progs: Enhance btrfs-image to handle missing device Qu Wenruo
` (2 preceding siblings ...)
2018-03-30 7:35 ` [PATCH 3/3] btrfs-progs: tests/misc: Test if btrfs-image can handle RAID1 missing device Qu Wenruo
@ 2018-03-30 20:15 ` David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2018-03-30 20:15 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread