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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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