Re: [PATCH] btrfs-progs: convert: Mention which reserve_space call failed

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

[-- Attachment #1.1: Type: text/plain, Size: 3590 bytes --]



On 2020/9/24 下午7:54, Marcos Paulo de Souza wrote:
> On Thu, 2020-09-24 at 08:08 +0800, Qu Wenruo wrote:
>>
>> On 2020/9/24 上午1:14, Marcos Paulo de Souza wrote:
>>> From: Marcos Paulo de Souza <mpdesouza@suse.com>
>>>
>>> btrfs-convert currently can't handle more fragmented block groups
>> when
>>> converting ext4 because the minimum size of a data chunk is 32Mb.
>>>
>>> When converting an ext4 fs with more fragmented block group and the
>> disk
>>> almost full, we can end up hitting a ENOSPC problem [1] since
>> smaller
>>> block groups (10Mb for example) end up being extended to 32Mb,
>> leaving
>>> the free space tree smaller when converting it to btrfs.
>>>
>>> This patch adds error messages telling which needed bytes couldn't
>> be
>>> allocated from the free space tree:
>>>
>>> create btrfs filesystem:
>>>         blocksize: 4096
>>>         nodesize:  16384
>>>         features:  extref, skinny-metadata (default)
>>>         checksum:  crc32c
>>> free space report:
>>>         total:     1073741824
>>>         free:      39124992 (3.64%)
>>> ERROR: failed to reserve 33554432 bytes from free space for
>> metadata chunk
>>> ERROR: unable to create initial ctree: No space left on device
>>>
>>> Link: https://github.com/kdave/btrfs-progs/issues/251
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>
>> Looks pretty good, but can be enhanced a little, inlined below.
>>
>> Despite that, feel free to add my tag:
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>>> ---
>>>  convert/common.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/convert/common.c b/convert/common.c
>>> index 048629df..6392e7f4 100644
>>> --- a/convert/common.c
>>> +++ b/convert/common.c
>>> @@ -812,8 +812,10 @@ int make_convert_btrfs(int fd, struct
>> btrfs_mkfs_config *cfg,
>>>  	 */
>>>  	ret = reserve_free_space(free_space, BTRFS_STRIPE_LEN,
>>>  				 &cfg->super_bytenr);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		error("failed to reserve %d bytes from free space for
>> temporary superblock", BTRFS_STRIPE_LEN);
>>
>> It would be awesome if we can output the free space.
>>
>> Just the largest portion is enough to show that we're hitting a real
>> ENOSPC situation.
> 
> Indeed, I'll send a v2 printing the free space tree when ENOSPC
> happens.

And it would be even better to mention the fragmentation problem in the
man page for btrfs-convert.

The fragmentation problem is a little too complex to explain in the
error message nor usage.

Although I guess the man page update could be another patch.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>>  		goto out;
>>> +	}
>>>  
>>>  	/*
>>>  	 * Then reserve system chunk space
>>> @@ -823,12 +825,16 @@ int make_convert_btrfs(int fd, struct
>> btrfs_mkfs_config *cfg,
>>>  	 */
>>>  	ret = reserve_free_space(free_space,
>> BTRFS_MKFS_SYSTEM_GROUP_SIZE,
>>>  				 &sys_chunk_start);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		error("failed to reserve %d bytes from free space for
>> system chunk", BTRFS_MKFS_SYSTEM_GROUP_SIZE);
>>>  		goto out;
>>> +	}
>>>  	ret = reserve_free_space(free_space,
>> BTRFS_CONVERT_META_GROUP_SIZE,
>>>  				 &meta_chunk_start);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		error("failed to reserve %d bytes from free space for
>> metadata chunk", BTRFS_CONVERT_META_GROUP_SIZE);
>>>  		goto out;
>>> +	}
>>>  
>>>  	/*
>>>  	 * Allocated meta/sys chunks will be mapped 1:1 with device
>> offset.
>>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]