Re: [PATCH 04/17] btrfs: move btrfs on disk definitions out of ctree.h

[Qu Wenruo <wqu@suse.com>]

On 2022/10/8 01:07, David Sterba wrote:
> On Thu, Sep 15, 2022 at 05:07:42PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2022/9/14 23:06, Josef Bacik wrote:
>>> The bulk of our on-disk definitions exist in btrfs_tree.h, which user
>>> space can use.
>>
>> Previously I tried to move some members to btrfs_tree.h, but didn't get
>> approved, mostly due to the fact that, we have those members exposed
>> through uapi is for TREE_SEARCH ioctl.
>>
>> But I'm not buying that reason at all.
>>
>> To me, all on-disk format, no matter if it's exposed through tree-search
>> should be in btrfs_tree.h.
> 
> All the structures are internal and not a guaranteed public API, we may
> want to change them any time. So if we move the definitions to UAPI then
> with a disclaimer that it's not a stable api and any compilation
> failures outside of kernel are up to the users to fix.

The point is, if we're changing the super block format, it's no 
difference than changing a on-disk structure.

It's still the same incompat/compat_ro/compat change depending on the 
member we change.

> 
> Which does not work in practice as easy as said and we have reverted
> some changes. See 34c51814b2b8 ("btrfs: re-instantiate the removed
> BTRFS_SUBVOL_CREATE_ASYNC definition").

That commit indeed teaches us something, even if we deprecated some 
features, it still has to be kept in the UAPI.

But that argument doesn't really affect the super block move AFAIK.

Since it's not even part of an ioctl, thus I don't think user-space tool 
would really both that.

> 
>> Although I'd prefer to rename btrfs_tree.h to btrfs_ondisk_format.h.
>>
>> Thus to David:
>>
>> Can we make it clear that, btrfs_tree.h is not only for tree search
>> ioctl, but also all the on-disk format thing?
> 
> It is for on-disk format defitions, but that's a different problem than
> the internal/external API.

Then, what's the proper way to export btrfs on-disk format?

 From an ioctl point of view, all those on-disk format things don't even 
need to be exported.

TREE_SEARCH ioctl is just returning a certain TLV formatted memory.
Yes, the basic things like btrfs_ioctl_search_header, but the content is 
still internal, not directly user-facing.

All the returned TLV members are really implementation related details, 
and even the TREE_SEARCH ioctl itself is more like a debug tool than 
proper interface.
A lot of things are done using TREE_SEARCH ioctl because we don't have 
better more defined interface.

Thanks,
Qu

> 
>> Reject once that's fine, but reject twice from two different guys, I
>> think it's not correct.
>>
>>>   Keep things consistent and move the rest of the on disk
>>> definitions out of ctree.h into btrfs_tree.h.  Note I did have to update
>>> all u8's to __u8, but otherwise this is a strict copy and paste.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>