On 25.10.2018 14:18, David Sterba wrote:
> On Wed, Oct 24, 2018 at 03:48:07PM +0100, Nikolay Borisov wrote:
>>
>>
>> On 24.10.18 г. 15:30 ч., David Sterba wrote:
>>> On Fri, Oct 12, 2018 at 05:57:29PM +0800, Qu Wenruo wrote:
>>>>>>>> + printf("log_root_transid (deprecated)\t%llu\n",
>>>>>>>> + le64_to_cpu(sb->__unused_log_root_transid));
>>>>>>>
>>>>>>> This should be entirely removed.
>>>>>>
>>>>>> It looks OK to me.
>>>>>> Just like the old leafsize.
>>>>>>
>>>>>> And if we try to use this member again, even old progs could show it
>>>>>> without problem.
>>>>>
>>>>> But what is the point of having something which always shows 0 and is
>>>>> essentially unused?
>>>>
>>>> Personally speaking, even it's in fact never used, it is still part of
>>>> the specification of btrfs super blocks.
>>>>
>>>> Even it's a bad decision we made a long time ago, we still need to mark
>>>> it as unused, other than converting it back to "reserved".
>>>>
>>>> To make it short and clear, if some member is specified in early
>>>> specification, even it's never used, we still need to mark it unused and
>>>> keep part of naming.
>>>
>>> Agreed, follows the same patern as the leafsize. The superblock dump is
>>> a debugging and bug report tool, the full output is desirable.
>>
>> I really don't want to get into the argument but I think we should
>> really name unused fields unusedX where X is some number. Looking at the
>> code in struct btrfs_root_backup we have 2 unused fields. In the
>> superblock we have unused space which is called 'reserved'. In struct
>> btrfs_disk_balance_args there is a field called unused, same thing for
>> struct btrfs_balance_item.
>>
>> In btrfs_inode_item we have 'reserved' field, same thing for
>> btrfs_root_item.
>>
>> So the code base is yet again highly inconsistent and looking at it
>> there is only a single field following the __unused_original_name, so
>> it's really the exception rather than the rule.
>
> You're right about the inconsistent namig, for the "reserved for future
> use" members. This could be unified to "reserved", and even though it's
> in a public header I would not expect any application using the items.
>
> The __unused_original_name is a different kind of "unused", as it became
> obsolete at some point and the bytes cannot be reused, unlike the
> reserved ones.
>
> It's an exception because it was the first instance, there was no
> established rule/pattern to follow just a deliberate choice in the
> naming so it captures the original purpose and current status.
>
>> Again, I would argue that once a field is deprecated or in this case it
>> has _never_ actually been used then we might as well rename it to
>> something cryptic so as not to attach (false) meaning to it. Just my 50
>> cents
>
> The log_root_transid is a again different from leafsize. I don't know
> what exactly you mean by the cryptic naming here, I'd rather keep the
> name selfexplanatory. Using 'reserved' could be misleading and somebody
> would want to use it for futher extensions.
But this is exactly my point - log_root_transid can indeed be repurposed
because it has _never_ been used in the code.
