Re: [PATCH] btrfs: allow defrag to convert inline extents to regular extents

[Qu Wenruo <wqu@suse.com>]

On 2022/5/9 10:15, Anand Jain wrote:
> On 5/7/22 12:09, Qu Wenruo wrote:
>> Btrfs defaults to max_inline=2K to make small writes inlined into
>> metadata.
>>
>> The default value is always a win, as even DUP/RAID1/RAID10 doubles the
>> metadata usage, it should still cause less physical space used compared
>> to a 4K regular extents.
>>
>> But since the introduce of RAID1C3 and RAID1C4 it's no longer the case,
>> users may find inlined extents causing too much space wasted, and want
>> to convert those inlined extents back to regular extents.
>>
>> Unfortunately defrag will unconditionally skip all inline extents, no
>> matter if the user is trying to converting them back to regular extents.
>>
>> So this patch will add a small exception for defrag_collect_targets() to
>> allow defragging inline extents, if and only if the inlined extents are
>> larger than max_inline, allowing users to convert them to regular ones.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ioctl.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 9d8e46815ee4..852c49565ab2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1420,8 +1420,19 @@ static int defrag_collect_targets(struct 
>> btrfs_inode *inode,
>>           if (!em)
>>               break;
>> -        /* Skip hole/inline/preallocated extents */
>> -        if (em->block_start >= EXTENT_MAP_LAST_BYTE ||
>> +        /*
>> +         * If the file extent is an inlined one, we may still want to
>> +         * defrag it (fallthrough) if it will cause a regular extent.
>> +         * This is for users who want to convert inline extents to
>> +         * regular ones through max_inline= mount option.
>> +         */
>> +        if (em->block_start == EXTENT_MAP_INLINE &&
>> +            em->len <= inode->root->fs_info->max_inline)
>> +            goto next;
>> +
>> +        /* Skip hole/delalloc/preallocated extents */
>> +        if (em->block_start == EXTENT_MAP_HOLE ||
>> +            em->block_start == EXTENT_MAP_DELALLOC ||
>>               test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
>>               goto next;
>> @@ -1480,6 +1491,15 @@ static int defrag_collect_targets(struct 
>> btrfs_inode *inode,
>>           if (em->len >= get_extent_max_capacity(em))
>>               goto next;
> 
> 
>> +        /*
>> +         * For inline extent it should be the first extent and it
>> +         * should not have a next extent.
>> +         * If the inlined extent passed all above checks, just add it
>> +         * for defrag, and be converted to regular extents.
>> +         */
>> +        if (em->block_start == EXTENT_MAP_INLINE)
>> +            goto add;
>> +
>>           next_mergeable = defrag_check_next_extent(&inode->vfs_inode, 
>> em,
>>                           extent_thresh, newer_than, locked);
>>           if (!next_mergeable) {
> Why not also let the inline extent have the next_mergeable checked?
> So the new regular extent will defrag. No?

You definitely forget the fact that inline extent should NOT have 
regular extents following it.

> 
> -Anand
>