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:
signature.asc
Description: OpenPGP digital signature
