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.
