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

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

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



On 2020/7/2 上午1:39, David Sterba wrote:
> 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.
> 

You mean to extract btrfs_init_fs_root() out of btrfs_get_fs_root()?

That looks a little risky and I can't find any good solution to make it
more elegant than the current one.

Although I would definitely remove the "__" prefix as we shouldn't add
such prefix anymore.

Thanks,
Qu


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