Re: [PATCH] btrfs-progs: Add minimum device size check.

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

-------- Original Message --------
Subject: Re: [PATCH] btrfs-progs: Add minimum device size check.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年06月23日 22:36
> On Thu, Jun 19, 2014 at 11:25:38AM +0800, Qu Wenruo wrote:
>> Btrfs has global block reservation, so even mkfs.btrfs can execute
>> without problem, there is still a possibility that the filesystem can't
>> be mounted.
>> For example when mkfs.btrfs on a 8M file on x86_64 platform, kernel will
>> refuse to mount due to ENOSPC, since system block group takes 4M and
>> mixed block group takes 4M, and global block reservation will takes all
>> the 4M from mixed block group, which makes btrfs unable to create uuid
>> tree.
>>
>> This patch will add minimum device size check before actually mkfs.
>> The minimum size calculation uses a simplified one:
>> minimum_size_for_each_dev = 2 * (system block group + global block rsv)
> So the global block reserve is set to 1024 * sectorsize. I know the
> minimum size guesses are easy to get wrong with various option mixes,
> some sane minimum is good. A filesystem in the scale of megabytes is not
> recommended anyway.
>
>> --- a/mkfs.c
>> +++ b/mkfs.c
>> @@ -1337,6 +1337,15 @@ int main(int ac, char **av)
>>   					       "metadata/data groups\n");
>>   					mixed = 1;
>>   				}
>> +				if (block_count < BTRFS_MIN_SIZE_EACH) {
>> +					fprintf(stderr,
>> +						"Size '%llu' is too small to make a usable btrfs\n",
>> +						block_count);
>> +					fprintf(stderr,
>> +						"Recommended size for each device is %llu\n",
>> +						BTRFS_MIN_SIZE_EACH);
> I'd suggest to merge the messages into one and do not use "Recommended".
> As described in the changelog it's the 'minimum' accepted size.
>
>> @@ -1370,6 +1379,21 @@ int main(int ac, char **av)
>>   	}
>>   	while (dev_cnt-- > 0) {
>>   		file = av[optind++];
>> +		ret = test_minimum_size(file);
>> +		if (ret < 0) {
>> +			fprintf(stderr, "Failed to check size for '%s': %s\n",
>> +				file, strerror(-ret));
>> +			exit(1);
>> +		}
>> +		if (ret > 0) {
>> +			fprintf(stderr,
>> +				"'%s' is too small to make a usable btrfs\n",
>> +				file);
>> +			fprintf(stderr,
>> +				"Recommended size for each device is %llu\n",
>> +				BTRFS_MIN_SIZE_EACH);
>> +			exit(1);
> Same here.
>
>> +		}
>>   		if (is_block_device(file))
>>   			if (test_dev_for_mkfs(file, force_overwrite, estr)) {
>>   				fprintf(stderr, "Error: %s", estr);
>> --- a/utils.h
>> +++ b/utils.h
>> @@ -101,5 +101,18 @@ int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
>>   int find_mount_root(const char *path, char **mount_root);
>>   int get_device_info(int fd, u64 devid,
>>   		struct btrfs_ioctl_dev_info_args *di_args);
>> +int test_minimum_size(const char *file);
>>   
>> +/*
>> + * Btrfs minimum size calculation is complicated, it should include at least:
>> + * 1. system group size
>> + * 2. minimum global block reserve
>> + * 3. metadata used at mkfs
>> + * 4. space reservation to create uuid for first mount.
>> + * Also, raid factor should also be taken into consideration.
>> + * To avoid the overkill calculation, (system group + global block rsv) * 2
>> + * for *EACH* device should be good enough.
>> + */
>> +#define BTRFS_MIN_SIZE_EACH	(2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE + \
>> +				      ((u64)sysconf(_SC_PAGESIZE) << 10)))
> The use of PAGESIZE here is well hidden in the macro, although it should
> be sectorsize of the currently created filesystem. Also,
> BTRFS_MIN_SIZE_EACH should be rather denote it's related to a device, so
> BTRFS_MIN_DEVICE_SIZE.
>
> Separating the minimum global block reserve size into a macro would be
> also cleaner.
>
> Thanks.
Thanks for the comment, I'll send the v3 patch fixing all the problem.

Thanks,
Qu