Re: [PATCH 2/3] btrfs-progs: image: enlarge the output file if no tree modification is needed for restore

[Su Yue <l@damenly.su>]

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().