Re: [PATCH v2] btrfs-progs: add --subvol option to mkfs.btrfs

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

在 2024/7/31 23:15, Josef Bacik 写道:
> On Tue, Jul 30, 2024 at 07:25:29PM +0930, Qu Wenruo wrote:
>>
>>
>> 在 2024/7/30 19:08, Mark Harmstone 写道:
>>> This patch adds a --subvol option, which tells mkfs.btrfs to create the
>>> specified directories as subvolumes.
>>>
>>> Given a populated directory img, the command
>>>
>>> $ mkfs.btrfs --rootdir img --subvol img/usr --subvol img/home --subvol img/home/username /dev/loop0
>>>
>>> will create subvolumes usr and home within the FS root, and subvolume
>>> username within the home subvolume. It will fail if any of the
>>> directories do not yet exist.
>>>
>>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>>
>> Unfortunately I'm still not a fan of splitting the handling of subvolume
>> and directory creation.
>>
>> Why not go a hashmap to save all the subvolume paths, and check if a
>> directory should be a subvolume, then go btrfs_make_subvolume() and
>> btrfs_link_subvolume() inside that context?
>>
>
> We don't have a hashmap readily available, and btrfs_make_subvolume() has to be
> done before we rebuild the UUID tree, otherwise we won't get the UUID entries.
> You're asking for a good deal of work ontop of an already relatively large
> patch.
>

For initial version we do not need hashmap, but just simple string array.
Yes, that will be a little slower but should not be a big deal.

> <snip>
>
>>> diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
>>> index 2b39f6bb..e3e576b2 100644
>>> --- a/mkfs/rootdir.c
>>> +++ b/mkfs/rootdir.c
>>> @@ -184,17 +184,58 @@ static void free_namelist(struct dirent **files, int count)
>>>    	free(files);
>>>    }
>>>
>>> -static u64 calculate_dir_inode_size(const char *dirname)
>>> +static u64 calculate_dir_inode_size(const char *src_dir,
>>> +				    struct list_head *subvol_children,
>>> +				    const char *dest_dir)
>>
>> And before the --subvolume option, I'm more intesrested in getting rid
>> of the function completely.
>>
>> Instead, if we go something like the following, there will be no need to
>> calculate the dir inode size:
>>
>> - Create a new inode/subvolume
>> - Link the new inode to the parent inode
>>
>> As btrfs_link_inode()/btrfs_link_subvolume() would handle the increase
>> of inode size.
>>
>> As long as we're only using btrfs_link_inode() and
>> btrfs_link_subvolume(), then there is no need to go this function.
>>
>> Furthermore it would make the whole opeartion more like a regular copy
>> (although implemented in progs), other than some specific btrfs hacks
>> just to pass the pretty strict "btrfs check".
>
> This is legitimate follow up work, but this thing exists today.  We have a long
> standing policy that gating peoples work on cleanups and refactors is generally
> unhelpful and demoralizing behavior.  I agree this needs to be reworked, but
> that's out of scope for this particular patchset.
>
> That being said this will be the next thing that Mark tackles.  Is that
> acceptable?  Thanks,

Well, I have already done the cleanup, furthermore the cleanup would
make new subvolume creation much easier:

https://lore.kernel.org/linux-btrfs/667ee4f02fdc2cb6f186eb8b06dd089f3ce53141.1722418505.git.wqu@suse.com/T/#u

Furthermore, with all the new rework, I believe the patch itself can be
much smaller than the current one, since we get rid of all the complex
hacks.

Thanks,
Qu
>
> Josef
>