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

[Josef Bacik <josef@toxicpanda.com>]

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.

<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,

Josef