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

Qu Wenruo
2020-07-01T23:57:07Z

[-- 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 --]