All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] btrfs-progs: image: make restored image file to be properly enlarged
@ 2021-03-26 12:50 Qu Wenruo
  2021-03-26 12:50 ` [PATCH 1/3] btrfs: image: remove the dead stat() call Qu Wenruo
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-03-26 12:50 UTC (permalink / raw)
  To: linux-btrfs

Recent kernel will refuse to mount restored image, even the source fs is
empty:
 # mkfs.btrfs -f /dev/test/test
 # btrfs-image /dev/test/test /tmp/dump
 # btrfs-image -r /tmp/dump ~/test.img
 # mount ~/test.img /mnt/btrfs
 mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
 # dmesg -t | tail -n 7
 loop0: detected capacity change from 10592 to 0
 BTRFS info (device loop0): disk space caching is enabled
 BTRFS info (device loop0): has skinny extents
 BTRFS info (device loop0): flagging fs with big metadata feature
 BTRFS error (device loop0): device total_bytes should be at most 5423104 but found 10737418240
 BTRFS error (device loop0): failed to read chunk tree: -22
 BTRFS error (device loop0): open_ctree failed

This is triggered by a recent kernel commit 3a160a933111 ("btrfs: drop never met disk total
bytes check in verify_one_dev_extent").

But the root cause is, we didn't enlarge the output file if the source
image only contains single device.

This bug won't affect restore to block device, or the destination file
is already large enough.

This patchset will fix the problem, and with new test case to detect
such problem.

Also remove one dead code exposed during the development.

Qu Wenruo (3):
  btrfs: image: remove the dead stat() call
  btrfs-progs: image: enlarge the output file if no tree modification is
    needed for restore
  btrfs-progs: misc-tests: add test to ensure the restored image can be
    mounted

 image/main.c                                  | 51 ++++++++++++++++---
 .../047-image-restore-mount/test.sh           | 19 +++++++
 2 files changed, 62 insertions(+), 8 deletions(-)
 create mode 100755 tests/misc-tests/047-image-restore-mount/test.sh

-- 
2.30.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] btrfs: image: remove the dead stat() call
  2021-03-26 12:50 [PATCH 0/3] btrfs-progs: image: make restored image file to be properly enlarged Qu Wenruo
@ 2021-03-26 12:50 ` Qu Wenruo
  2021-03-26 14:08   ` Su Yue
  2021-03-26 12:50 ` [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore Qu Wenruo
  2021-03-26 12:50 ` [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted Qu Wenruo
  2 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2021-03-26 12:50 UTC (permalink / raw)
  To: linux-btrfs

In restore_metadump(), we call stat() but never uses the result get.

This call site is left by some code refactor, as the stat() call is now
moved into fixup_device_size().

So we can safely remove the call.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/image/main.c b/image/main.c
index 48070e52c21f..24393188e5e3 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2690,7 +2690,6 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 	if (!ret && !multi_devices && !old_restore &&
 	    btrfs_super_num_devices(mdrestore.original_super) != 1) {
 		struct btrfs_root *root;
-		struct stat st;
 
 		root = open_ctree_fd(fileno(out), target, 0,
 					  OPEN_CTREE_PARTIAL |
@@ -2703,13 +2702,6 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		}
 		info = root->fs_info;
 
-		if (stat(target, &st)) {
-			error("stat %s failed: %m", target);
-			close_ctree(info->chunk_root);
-			free(cluster);
-			return 1;
-		}
-
 		ret = fixup_chunks_and_devices(info, &mdrestore, fileno(out));
 		close_ctree(info->chunk_root);
 		if (ret)
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore
  2021-03-26 12:50 [PATCH 0/3] btrfs-progs: image: make restored image file to be properly enlarged Qu Wenruo
  2021-03-26 12:50 ` [PATCH 1/3] btrfs: image: remove the dead stat() call Qu Wenruo
@ 2021-03-26 12:50 ` Qu Wenruo
  2021-03-26 13:52   ` Su Yue
  2021-04-16 17:40   ` David Sterba
  2021-03-26 12:50 ` [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted Qu Wenruo
  2 siblings, 2 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-03-26 12:50 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Nikolay Borisov

[BUG]
If restoring dumpped image into a new file, under most cases kernel will
reject it:

 # mkfs.btrfs -f /dev/test/test
 # btrfs-image /dev/test/test /tmp/dump
 # btrfs-image -r /tmp/dump ~/test.img
 # mount ~/test.img /mnt/btrfs
 mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
 # dmesg -t | tail -n 7
 loop0: detected capacity change from 10592 to 0
 BTRFS info (device loop0): disk space caching is enabled
 BTRFS info (device loop0): has skinny extents
 BTRFS info (device loop0): flagging fs with big metadata feature
 BTRFS error (device loop0): device total_bytes should be at most 5423104 but found 10737418240
 BTRFS error (device loop0): failed to read chunk tree: -22
 BTRFS error (device loop0): open_ctree failed

[CAUSE]
When btrfs-image restores an image into a file, and the source image
contains only single device, then we don't need to modify the
chunk/device tree, as we can reuse the existing chunk/dev tree without
any problem.

This also means, for such restore, we also won't do any target file
enlarge. This behavior itself is fine, as at that time, kernel won't
check if the device is smaller than the device size recorded in device
tree.

But later kernel commit 3a160a933111 ("btrfs: drop never met disk total
bytes check in verify_one_dev_extent") introduces new check on device
size at mount time, rejecting any loop file which is smaller than the
original device size.

[FIX]
Do extra file enlarge for single device restore.

Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 image/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/image/main.c b/image/main.c
index 24393188e5e3..9933f69d0fdb 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2706,6 +2706,49 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		close_ctree(info->chunk_root);
 		if (ret)
 			goto out;
+	} else {
+		struct btrfs_root *root;
+		struct stat st;
+		u64 dev_size;
+
+		if (!info) {
+			root = open_ctree_fd(fileno(out), target, 0, 0);
+			if (!root) {
+				error("open ctree failed in %s", target);
+				ret = -EIO;
+				goto out;
+			}
+
+			info = root->fs_info;
+
+			dev_size = btrfs_stack_device_total_bytes(
+					&info->super_copy->dev_item);
+			close_ctree(root);
+			info = NULL;
+		} else {
+			dev_size = btrfs_stack_device_total_bytes(
+					&info->super_copy->dev_item);
+		}
+
+		/*
+		 * We don't need extra tree modification, but if the output is
+		 * a file, we need to enlarge the output file so that
+		 * newer kernel won't report error.
+		 */
+		ret = fstat(fileno(out), &st);
+		if (ret < 0) {
+			error("failed to stat result image: %m");
+			ret = -errno;
+			goto out;
+		}
+		if (S_ISREG(st.st_mode)) {
+			ret = ftruncate64(fileno(out), dev_size);
+			if (ret < 0) {
+				error("failed to enlarge result image: %m");
+				ret = -errno;
+				goto out;
+			}
+		}
 	}
 out:
 	mdrestore_destroy(&mdrestore, num_threads);
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted
  2021-03-26 12:50 [PATCH 0/3] btrfs-progs: image: make restored image file to be properly enlarged Qu Wenruo
  2021-03-26 12:50 ` [PATCH 1/3] btrfs: image: remove the dead stat() call Qu Wenruo
  2021-03-26 12:50 ` [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore Qu Wenruo
@ 2021-03-26 12:50 ` Qu Wenruo
  2021-04-16 17:46   ` David Sterba
  2 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2021-03-26 12:50 UTC (permalink / raw)
  To: linux-btrfs

This new test case is to make sure the restored image file has been
properly enlarged so that newer kernel won't complain.

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

diff --git a/tests/misc-tests/047-image-restore-mount/test.sh b/tests/misc-tests/047-image-restore-mount/test.sh
new file mode 100755
index 000000000000..7f12afa2bab6
--- /dev/null
+++ b/tests/misc-tests/047-image-restore-mount/test.sh
@@ -0,0 +1,19 @@
+#!/bin/bash
+# Verify that the restored image of an empty btrfs can still be mounted
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs-image
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+tmp=$(mktemp -d --tmpdir btrfs-progs-image.XXXXXXXX)
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check "$TOP/btrfs-image" "$TEST_DEV" "$tmp/dump"
+run_check "$TOP/btrfs-image" -r "$tmp/dump" "$tmp/restored"
+
+run_check $SUDO_HELPER mount -t btrfs -o loop "$tmp/restored" "$TEST_MNT"
+umount "$TEST_MNT" &> /dev/null
+rm -rf -- "$tmp"
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore
  2021-03-26 12:50 ` [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore Qu Wenruo
@ 2021-03-26 13:52   ` Su Yue
  2021-03-26 14:29     ` Su Yue
  2021-04-16 17:40   ` David Sterba
  1 sibling, 1 reply; 11+ messages in thread
From: Su Yue @ 2021-03-26 13:52 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs, Nikolay Borisov


On Fri 26 Mar 2021 at 20:50, Qu Wenruo <wqu@suse.com> wrote:

> [BUG]
> If restoring dumpped image into a new file, under most cases 
> kernel will
> reject it:
>
>  # mkfs.btrfs -f /dev/test/test
>  # btrfs-image /dev/test/test /tmp/dump
>  # btrfs-image -r /tmp/dump ~/test.img
>  # mount ~/test.img /mnt/btrfs
>  mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on 
>  /dev/loop0, missing codepage or helper program, or other error.
>  # dmesg -t | tail -n 7
>  loop0: detected capacity change from 10592 to 0
>  BTRFS info (device loop0): disk space caching is enabled
>  BTRFS info (device loop0): has skinny extents
>  BTRFS info (device loop0): flagging fs with big metadata 
>  feature
>  BTRFS error (device loop0): device total_bytes should be at 
>  most 5423104 but found 10737418240
>  BTRFS error (device loop0): failed to read chunk tree: -22
>  BTRFS error (device loop0): open_ctree failed
>
> [CAUSE]
> When btrfs-image restores an image into a file, and the source 
> image
> contains only single device, then we don't need to modify the
> chunk/device tree, as we can reuse the existing chunk/dev tree 
> without
> any problem.
>
> This also means, for such restore, we also won't do any target 
> file
> enlarge. This behavior itself is fine, as at that time, kernel 
> won't
> check if the device is smaller than the device size recorded in 
> device
> tree.
>
> But later kernel commit 3a160a933111 ("btrfs: drop never met 
> disk total
> bytes check in verify_one_dev_extent") introduces new check on 
> device
> size at mount time, rejecting any loop file which is smaller 
> than the
> original device size.
>
> [FIX]
> Do extra file enlarge for single device restore.
>
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/image/main.c b/image/main.c
> index 24393188e5e3..9933f69d0fdb 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2706,6 +2706,49 @@ static int restore_metadump(const char 
> *input, FILE *out, int old_restore,
>  		close_ctree(info->chunk_root);
>  		if (ret)
>  			goto out;
> +	} else {
> +		struct btrfs_root *root;
> +		struct stat st;
> +		u64 dev_size;
> +
> +		if (!info) {
>
Here info is not NULL else above fixup_chunks_and_devices() 
already
crashed at deferencing fs_info->super_copy. So I guess the branch
can be deleted?

> +			root = open_ctree_fd(fileno(out), target, 0, 0);
> +			if (!root) {
> +				error("open ctree failed in %s", target);
> +				ret = -EIO;
> +				goto out;
> +			}
> +
> +			info = root->fs_info;
> +
> +			dev_size = btrfs_stack_device_total_bytes(
> +					&info->super_copy->dev_item);
> +			close_ctree(root);
> +			info = NULL;
> +		} else {
> +			dev_size = btrfs_stack_device_total_bytes(
> +					&info->super_copy->dev_item);
> +		}
> +
> +		/*
> +		 * We don't need extra tree modification, but if the 
> output is
> +		 * a file, we need to enlarge the output file so that
> +		 * newer kernel won't report error.
> +		 */
> +		ret = fstat(fileno(out), &st);
> +		if (ret < 0) {
> +			error("failed to stat result image: %m");
> +			ret = -errno;
> +			goto out;
> +		}
> +		if (S_ISREG(st.st_mode)) {
> +			ret = ftruncate64(fileno(out), dev_size);
> +			if (ret < 0) {
> +				error("failed to enlarge result image: %m");
> +				ret = -errno;
> +				goto out;
>
I see there is a same pattern inside fixup_device_size().

--
Su
> +			}
> +		}
>  	}
>  out:
>  	mdrestore_destroy(&mdrestore, num_threads);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] btrfs: image: remove the dead stat() call
  2021-03-26 12:50 ` [PATCH 1/3] btrfs: image: remove the dead stat() call Qu Wenruo
@ 2021-03-26 14:08   ` Su Yue
  0 siblings, 0 replies; 11+ messages in thread
From: Su Yue @ 2021-03-26 14:08 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs


On Fri 26 Mar 2021 at 20:50, Qu Wenruo <wqu@suse.com> wrote:

> In restore_metadump(), we call stat() but never uses the result 
> get.
>
> This call site is left by some code refactor, as the stat() call 
> is now
> moved into fixup_device_size().
>
fixup_device_size() already calls fstat() on out_fd.

Reviewed-by: Su Yue <l@damenly.su>


> So we can safely remove the call.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/image/main.c b/image/main.c
> index 48070e52c21f..24393188e5e3 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2690,7 +2690,6 @@ static int restore_metadump(const char 
> *input, FILE *out, int old_restore,
>  	if (!ret && !multi_devices && !old_restore &&
>  	    btrfs_super_num_devices(mdrestore.original_super) != 1) {
>  		struct btrfs_root *root;
> -		struct stat st;
>
>  		root = open_ctree_fd(fileno(out), target, 0,
>  					  OPEN_CTREE_PARTIAL |
> @@ -2703,13 +2702,6 @@ static int restore_metadump(const char 
> *input, FILE *out, int old_restore,
>  		}
>  		info = root->fs_info;
>
> -		if (stat(target, &st)) {
> -			error("stat %s failed: %m", target);
> -			close_ctree(info->chunk_root);
> -			free(cluster);
> -			return 1;
> -		}
> -
>  		ret = fixup_chunks_and_devices(info, &mdrestore, 
>  fileno(out));
>  		close_ctree(info->chunk_root);
>  		if (ret)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore
  2021-03-26 13:52   ` Su Yue
@ 2021-03-26 14:29     ` Su Yue
  0 siblings, 0 replies; 11+ messages in thread
From: Su Yue @ 2021-03-26 14:29 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs, Nikolay Borisov


On Fri 26 Mar 2021 at 21:52, Su Yue <l@damenly.su> wrote:

> On Fri 26 Mar 2021 at 20:50, Qu Wenruo <wqu@suse.com> wrote:
>
>> [BUG]
>> If restoring dumpped image into a new file, under most cases
>> kernel will
>> reject it:
>>
>>  # mkfs.btrfs -f /dev/test/test
>>  # btrfs-image /dev/test/test /tmp/dump
>>  # btrfs-image -r /tmp/dump ~/test.img
>>  # mount ~/test.img /mnt/btrfs
>>  mount: /mnt/btrfs: wrong fs type, bad option, bad superblock 
>>  on
>>  /dev/loop0, missing codepage or helper program, or other 
>>  error.
>>  # dmesg -t | tail -n 7
>>  loop0: detected capacity change from 10592 to 0
>>  BTRFS info (device loop0): disk space caching is enabled
>>  BTRFS info (device loop0): has skinny extents
>>  BTRFS info (device loop0): flagging fs with big metadata
>>  feature
>>  BTRFS error (device loop0): device total_bytes should be at
>>  most 5423104 but found 10737418240
>>  BTRFS error (device loop0): failed to read chunk tree: -22
>>  BTRFS error (device loop0): open_ctree failed
>>
>> [CAUSE]
>> When btrfs-image restores an image into a file, and the source
>> image
>> contains only single device, then we don't need to modify the
>> chunk/device tree, as we can reuse the existing chunk/dev tree
>> without
>> any problem.
>>
>> This also means, for such restore, we also won't do any target
>> file
>> enlarge. This behavior itself is fine, as at that time, kernel
>> won't
>> check if the device is smaller than the device size recorded in
>> device
>> tree.
>>
>> But later kernel commit 3a160a933111 ("btrfs: drop never met
>> disk total
>> bytes check in verify_one_dev_extent") introduces new check on
>> device
>> size at mount time, rejecting any loop file which is smaller
>> than the
>> original device size.
>>
>> [FIX]
>> Do extra file enlarge for single device restore.
>>
>> Reported-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  image/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/image/main.c b/image/main.c
>> index 24393188e5e3..9933f69d0fdb 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2706,6 +2706,49 @@ static int restore_metadump(const char
>> *input, FILE *out, int old_restore,
>>  		close_ctree(info->chunk_root);
>>  		if (ret)
>>  			goto out;
>> +	} else {

The indent... Never mind. I think I should go to sleep.

Reviewed-by: Su Yue <l@damenly.su>

>> +		struct btrfs_root *root;
>> +		struct stat st;
>> +		u64 dev_size;
>> +
>> +		if (!info) {
>>
> Here info is not NULL else above fixup_chunks_and_devices()
> already
> crashed at deferencing fs_info->super_copy. So I guess the 
> branch
> can be deleted?

>> +			root = open_ctree_fd(fileno(out), target, 0, 0);
>> +			if (!root) {
>> +				error("open ctree failed in %s", target);
>> +				ret = -EIO;
>> +				goto out;
>> +			}
>> +
>> +			info = root->fs_info;
>> +
>> +			dev_size = btrfs_stack_device_total_bytes(
>> +					&info->super_copy->dev_item);
>> +			close_ctree(root);
>> +			info = NULL;
>> +		} else {
>> +			dev_size = btrfs_stack_device_total_bytes(
>> +					&info->super_copy->dev_item);
>> +		}
>> +
>> +		/*
>> +		 * We don't need extra tree modification, but if the
>> output is
>> +		 * a file, we need to enlarge the output file so that
>> +		 * newer kernel won't report error.
>> +		 */
>> +		ret = fstat(fileno(out), &st);
>> +		if (ret < 0) {
>> +			error("failed to stat result image: %m");
>> +			ret = -errno;
>> +			goto out;
>> +		}
>> +		if (S_ISREG(st.st_mode)) {
>> +			ret = ftruncate64(fileno(out), dev_size);
>> +			if (ret < 0) {
>> +				error("failed to enlarge result image: %m");
>> +				ret = -errno;
>> +				goto out;
>>
> I see there is a same pattern inside fixup_device_size().


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore
  2021-03-26 12:50 ` [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore Qu Wenruo
  2021-03-26 13:52   ` Su Yue
@ 2021-04-16 17:40   ` David Sterba
  2021-04-17  0:17     ` Qu Wenruo
  1 sibling, 1 reply; 11+ messages in thread
From: David Sterba @ 2021-04-16 17:40 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs, Nikolay Borisov

On Fri, Mar 26, 2021 at 08:50:46PM +0800, Qu Wenruo wrote:
> [BUG]
> If restoring dumpped image into a new file, under most cases kernel will
> reject it:
> 
>  # mkfs.btrfs -f /dev/test/test
>  # btrfs-image /dev/test/test /tmp/dump
>  # btrfs-image -r /tmp/dump ~/test.img
>  # mount ~/test.img /mnt/btrfs
>  mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>  # dmesg -t | tail -n 7
>  loop0: detected capacity change from 10592 to 0
>  BTRFS info (device loop0): disk space caching is enabled
>  BTRFS info (device loop0): has skinny extents
>  BTRFS info (device loop0): flagging fs with big metadata feature
>  BTRFS error (device loop0): device total_bytes should be at most 5423104 but found 10737418240
>  BTRFS error (device loop0): failed to read chunk tree: -22
>  BTRFS error (device loop0): open_ctree failed
> 
> [CAUSE]
> When btrfs-image restores an image into a file, and the source image
> contains only single device, then we don't need to modify the
> chunk/device tree, as we can reuse the existing chunk/dev tree without
> any problem.
> 
> This also means, for such restore, we also won't do any target file
> enlarge. This behavior itself is fine, as at that time, kernel won't
> check if the device is smaller than the device size recorded in device
> tree.
> 
> But later kernel commit 3a160a933111 ("btrfs: drop never met disk total
> bytes check in verify_one_dev_extent") introduces new check on device
> size at mount time, rejecting any loop file which is smaller than the
> original device size.
> 
> [FIX]
> Do extra file enlarge for single device restore.
> 
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  image/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/image/main.c b/image/main.c
> index 24393188e5e3..9933f69d0fdb 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2706,6 +2706,49 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>  		close_ctree(info->chunk_root);
>  		if (ret)
>  			goto out;
> +	} else {
> +		struct btrfs_root *root;
> +		struct stat st;
> +		u64 dev_size;
> +
> +		if (!info) {
> +			root = open_ctree_fd(fileno(out), target, 0, 0);
> +			if (!root) {
> +				error("open ctree failed in %s", target);
> +				ret = -EIO;
> +				goto out;
> +			}
> +
> +			info = root->fs_info;
> +
> +			dev_size = btrfs_stack_device_total_bytes(
> +					&info->super_copy->dev_item);
> +			close_ctree(root);
> +			info = NULL;
> +		} else {
> +			dev_size = btrfs_stack_device_total_bytes(
> +					&info->super_copy->dev_item);
> +		}
> +
> +		/*
> +		 * We don't need extra tree modification, but if the output is
> +		 * a file, we need to enlarge the output file so that
> +		 * newer kernel won't report error.
> +		 */
> +		ret = fstat(fileno(out), &st);
> +		if (ret < 0) {
> +			error("failed to stat result image: %m");
> +			ret = -errno;
> +			goto out;
> +		}
> +		if (S_ISREG(st.st_mode)) {
> +			ret = ftruncate64(fileno(out), dev_size);

This truncates the file unconditionally, so if the file is larger than
required, I don't think it's necessary to do it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted
  2021-03-26 12:50 ` [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted Qu Wenruo
@ 2021-04-16 17:46   ` David Sterba
  2021-04-17  0:18     ` Qu Wenruo
  0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2021-04-16 17:46 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Fri, Mar 26, 2021 at 08:50:47PM +0800, Qu Wenruo wrote:
> This new test case is to make sure the restored image file has been
> properly enlarged so that newer kernel won't complain.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  .../047-image-restore-mount/test.sh           | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100755 tests/misc-tests/047-image-restore-mount/test.sh
> 
> diff --git a/tests/misc-tests/047-image-restore-mount/test.sh b/tests/misc-tests/047-image-restore-mount/test.sh
> new file mode 100755
> index 000000000000..7f12afa2bab6
> --- /dev/null
> +++ b/tests/misc-tests/047-image-restore-mount/test.sh
> @@ -0,0 +1,19 @@
> +#!/bin/bash
> +# Verify that the restored image of an empty btrfs can still be mounted
                                                ^^^^^

I've seen that in patches and comments, the use of word 'btrfs' instead
of 'filesystem' sounds a bit inappropriate to me, so I change it
whenever I see it. It's perhaps matter of taste and style, one can write
it also as 'btrfs filesystem' but that may belong to some more polished
documentation, so you can go with just 'filesystem'.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore
  2021-04-16 17:40   ` David Sterba
@ 2021-04-17  0:17     ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-04-17  0:17 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs, Nikolay Borisov



On 2021/4/17 上午1:40, David Sterba wrote:
> On Fri, Mar 26, 2021 at 08:50:46PM +0800, Qu Wenruo wrote:
>> [BUG]
>> If restoring dumpped image into a new file, under most cases kernel will
>> reject it:
>>
>>   # mkfs.btrfs -f /dev/test/test
>>   # btrfs-image /dev/test/test /tmp/dump
>>   # btrfs-image -r /tmp/dump ~/test.img
>>   # mount ~/test.img /mnt/btrfs
>>   mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>>   # dmesg -t | tail -n 7
>>   loop0: detected capacity change from 10592 to 0
>>   BTRFS info (device loop0): disk space caching is enabled
>>   BTRFS info (device loop0): has skinny extents
>>   BTRFS info (device loop0): flagging fs with big metadata feature
>>   BTRFS error (device loop0): device total_bytes should be at most 5423104 but found 10737418240
>>   BTRFS error (device loop0): failed to read chunk tree: -22
>>   BTRFS error (device loop0): open_ctree failed
>>
>> [CAUSE]
>> When btrfs-image restores an image into a file, and the source image
>> contains only single device, then we don't need to modify the
>> chunk/device tree, as we can reuse the existing chunk/dev tree without
>> any problem.
>>
>> This also means, for such restore, we also won't do any target file
>> enlarge. This behavior itself is fine, as at that time, kernel won't
>> check if the device is smaller than the device size recorded in device
>> tree.
>>
>> But later kernel commit 3a160a933111 ("btrfs: drop never met disk total
>> bytes check in verify_one_dev_extent") introduces new check on device
>> size at mount time, rejecting any loop file which is smaller than the
>> original device size.
>>
>> [FIX]
>> Do extra file enlarge for single device restore.
>>
>> Reported-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   image/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/image/main.c b/image/main.c
>> index 24393188e5e3..9933f69d0fdb 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2706,6 +2706,49 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>>   		close_ctree(info->chunk_root);
>>   		if (ret)
>>   			goto out;
>> +	} else {
>> +		struct btrfs_root *root;
>> +		struct stat st;
>> +		u64 dev_size;
>> +
>> +		if (!info) {
>> +			root = open_ctree_fd(fileno(out), target, 0, 0);
>> +			if (!root) {
>> +				error("open ctree failed in %s", target);
>> +				ret = -EIO;
>> +				goto out;
>> +			}
>> +
>> +			info = root->fs_info;
>> +
>> +			dev_size = btrfs_stack_device_total_bytes(
>> +					&info->super_copy->dev_item);
>> +			close_ctree(root);
>> +			info = NULL;
>> +		} else {
>> +			dev_size = btrfs_stack_device_total_bytes(
>> +					&info->super_copy->dev_item);
>> +		}
>> +
>> +		/*
>> +		 * We don't need extra tree modification, but if the output is
>> +		 * a file, we need to enlarge the output file so that
>> +		 * newer kernel won't report error.
>> +		 */
>> +		ret = fstat(fileno(out), &st);
>> +		if (ret < 0) {
>> +			error("failed to stat result image: %m");
>> +			ret = -errno;
>> +			goto out;
>> +		}
>> +		if (S_ISREG(st.st_mode)) {
>> +			ret = ftruncate64(fileno(out), dev_size);
>
> This truncates the file unconditionally, so if the file is larger than
> required, I don't think it's necessary to do it.
>
Indeed, I'll update the patchset to do conditional truncation.

Thanks,
Qu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted
  2021-04-16 17:46   ` David Sterba
@ 2021-04-17  0:18     ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-04-17  0:18 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs



On 2021/4/17 上午1:46, David Sterba wrote:
> On Fri, Mar 26, 2021 at 08:50:47PM +0800, Qu Wenruo wrote:
>> This new test case is to make sure the restored image file has been
>> properly enlarged so that newer kernel won't complain.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   .../047-image-restore-mount/test.sh           | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>   create mode 100755 tests/misc-tests/047-image-restore-mount/test.sh
>>
>> diff --git a/tests/misc-tests/047-image-restore-mount/test.sh b/tests/misc-tests/047-image-restore-mount/test.sh
>> new file mode 100755
>> index 000000000000..7f12afa2bab6
>> --- /dev/null
>> +++ b/tests/misc-tests/047-image-restore-mount/test.sh
>> @@ -0,0 +1,19 @@
>> +#!/bin/bash
>> +# Verify that the restored image of an empty btrfs can still be mounted
>                                                  ^^^^^
>
> I've seen that in patches and comments, the use of word 'btrfs' instead
> of 'filesystem' sounds a bit inappropriate to me, so I change it
> whenever I see it. It's perhaps matter of taste and style, one can write
> it also as 'btrfs filesystem' but that may belong to some more polished
> documentation, so you can go with just 'filesystem'.
>
Thanks for pointing this out.

I'll use 'filesystem' from now on.

Thanks,
Qu

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-17  0:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 12:50 [PATCH 0/3] btrfs-progs: image: make restored image file to be properly enlarged Qu Wenruo
2021-03-26 12:50 ` [PATCH 1/3] btrfs: image: remove the dead stat() call Qu Wenruo
2021-03-26 14:08   ` Su Yue
2021-03-26 12:50 ` [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore Qu Wenruo
2021-03-26 13:52   ` Su Yue
2021-03-26 14:29     ` Su Yue
2021-04-16 17:40   ` David Sterba
2021-04-17  0:17     ` Qu Wenruo
2021-03-26 12:50 ` [PATCH 3/3] btrfs-progs: misc-tests: add test to ensure the restored image can be mounted Qu Wenruo
2021-04-16 17:46   ` David Sterba
2021-04-17  0:18     ` Qu Wenruo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.