Re: [PATCH 3/4] btrfs: preallocate anon_dev for subvolume and snapshot creation

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 01, 2020 at 11:25:27AM +0800, Qu Wenruo wrote:
> >> +struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info,
> >> +					 u64 objectid, dev_t anon_dev)
> >> +{
> >> +	return __get_fs_root(fs_info, objectid, anon_dev, true);
> >> +}
> > 
> > This does not look like a good API, we should keep btrfs_get_fs_root and
> > add the anon_bdev initialization to the callers, there are only a few.
> > 
> 
> A few = over 25?
> 
> I have switched to keep btrfs_get_fs_root(), but you won't like the summary:
> 
> Old:
>  fs/btrfs/disk-io.h     |  2 ++
>  fs/btrfs/ioctl.c       | 21 ++++++++++++++++-
>  fs/btrfs/transaction.c |  3 ++-
>  fs/btrfs/transaction.h |  2 ++
>  5 files changed, 70 insertions(+), 9 deletions(-)
> 
> New:
>  fs/btrfs/backref.c     |  4 ++--
>  fs/btrfs/disk-io.c     | 42 ++++++++++++++++++++++++++++++++++--------
>  fs/btrfs/disk-io.h     |  3 ++-
>  fs/btrfs/export.c      |  2 +-
>  fs/btrfs/file.c        |  2 +-
>  fs/btrfs/inode.c       |  2 +-
>  fs/btrfs/ioctl.c       | 31 +++++++++++++++++++++++++------
>  fs/btrfs/relocation.c  | 11 ++++++-----
>  fs/btrfs/root-tree.c   |  2 +-
>  fs/btrfs/scrub.c       |  2 +-
>  fs/btrfs/send.c        |  4 ++--
>  fs/btrfs/super.c       |  2 +-
>  fs/btrfs/transaction.c |  3 ++-
>  fs/btrfs/transaction.h |  2 ++
>  fs/btrfs/tree-log.c    |  2 +-
>  fs/btrfs/uuid-tree.c   |  2 +-
>  16 files changed, 83 insertions(+), 33 deletions(-)
> 
> Do we really go that direction?

You're right, I don't like the summary and I don't like the code either.

Adding the anon_dev argument to btrfs_get_fs_root is wrong and I have
never suggested that. What I meant is to put the actual id allocation
to the callers where the subvolume is created, ie only 2 places.