linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] btrfs-progs: find-root: Fix wrong generation for log tree
@ 2018-10-12  6:42 Qu Wenruo
  2018-10-12  6:42 ` [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid Qu Wenruo
  0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2018-10-12  6:42 UTC (permalink / raw)
  To: linux-btrfs

When searching for log tree, we should use btrfs_super_generation + 1
other than log_root_transid.

Since log_root_transid is never touched and will be deprecated soon.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 btrfs-find-root.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index e2d2e70c408c..376bbc4c47fa 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -73,7 +73,7 @@ static void get_root_gen_and_level(u64 objectid, struct btrfs_fs_info *fs_info,
 		break;
 	case BTRFS_TREE_LOG_OBJECTID:
 		level = btrfs_super_log_root_level(super);
-		gen = btrfs_super_log_root_transid(super);
+		gen = btrfs_super_generation(super) + 1;
 		break;
 	case BTRFS_UUID_TREE_OBJECTID:
 		gen = btrfs_super_uuid_tree_generation(super);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  6:42 [PATCH 1/2] btrfs-progs: find-root: Fix wrong generation for log tree Qu Wenruo
@ 2018-10-12  6:42 ` Qu Wenruo
  2018-10-12  6:53   ` Nikolay Borisov
  0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2018-10-12  6:42 UTC (permalink / raw)
  To: linux-btrfs

The only user of it is "btrfs inspect dump-super".

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds-inspect-dump-super.c | 4 ++--
 ctree.h                   | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
index e965267c5d96..3f33931ed9bc 100644
--- a/cmds-inspect-dump-super.c
+++ b/cmds-inspect-dump-super.c
@@ -387,8 +387,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
 	       (unsigned long long)btrfs_super_chunk_root_level(sb));
 	printf("log_root\t\t%llu\n",
 	       (unsigned long long)btrfs_super_log_root(sb));
-	printf("log_root_transid\t%llu\n",
-	       (unsigned long long)btrfs_super_log_root_transid(sb));
+	printf("log_root_transid (deprecated)\t%llu\n",
+	       le64_to_cpu(sb->__unused_log_root_transid));
 	printf("log_root_level\t\t%llu\n",
 	       (unsigned long long)btrfs_super_log_root_level(sb));
 	printf("total_bytes\t\t%llu\n",
diff --git a/ctree.h b/ctree.h
index 4719962df67d..a314fdb102b0 100644
--- a/ctree.h
+++ b/ctree.h
@@ -427,8 +427,8 @@ struct btrfs_super_block {
 	__le64 chunk_root;
 	__le64 log_root;
 
-	/* this will help find the new super based on the log root */
-	__le64 log_root_transid;
+	/* This member is never touched, should always be 0 */
+	__le64 __unused_log_root_transid;
 	__le64 total_bytes;
 	__le64 bytes_used;
 	__le64 root_dir_objectid;
@@ -2203,8 +2203,6 @@ BTRFS_SETGET_STACK_FUNCS(super_chunk_root_level, struct btrfs_super_block,
 			 chunk_root_level, 8);
 BTRFS_SETGET_STACK_FUNCS(super_log_root, struct btrfs_super_block,
 			 log_root, 64);
-BTRFS_SETGET_STACK_FUNCS(super_log_root_transid, struct btrfs_super_block,
-			 log_root_transid, 64);
 BTRFS_SETGET_STACK_FUNCS(super_log_root_level, struct btrfs_super_block,
 			 log_root_level, 8);
 BTRFS_SETGET_STACK_FUNCS(super_total_bytes, struct btrfs_super_block,
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  6:42 ` [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid Qu Wenruo
@ 2018-10-12  6:53   ` Nikolay Borisov
  2018-10-12  8:46     ` Qu Wenruo
  0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2018-10-12  6:53 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 12.10.2018 09:42, Qu Wenruo wrote:
> The only user of it is "btrfs inspect dump-super".
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  cmds-inspect-dump-super.c | 4 ++--
>  ctree.h                   | 6 ++----
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
> index e965267c5d96..3f33931ed9bc 100644
> --- a/cmds-inspect-dump-super.c
> +++ b/cmds-inspect-dump-super.c
> @@ -387,8 +387,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>  	       (unsigned long long)btrfs_super_chunk_root_level(sb));
>  	printf("log_root\t\t%llu\n",
>  	       (unsigned long long)btrfs_super_log_root(sb));
> -	printf("log_root_transid\t%llu\n",
> -	       (unsigned long long)btrfs_super_log_root_transid(sb));
> +	printf("log_root_transid (deprecated)\t%llu\n",
> +	       le64_to_cpu(sb->__unused_log_root_transid));

This should be entirely removed.

>  	printf("log_root_level\t\t%llu\n",
>  	       (unsigned long long)btrfs_super_log_root_level(sb));
>  	printf("total_bytes\t\t%llu\n",
> diff --git a/ctree.h b/ctree.h
> index 4719962df67d..a314fdb102b0 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -427,8 +427,8 @@ struct btrfs_super_block {
>  	__le64 chunk_root;
>  	__le64 log_root;
>  
> -	/* this will help find the new super based on the log root */
> -	__le64 log_root_transid;
> +	/* This member is never touched, should always be 0 */
> +	__le64 __unused_log_root_transid;
>  	__le64 total_bytes;
>  	__le64 bytes_used;
>  	__le64 root_dir_objectid;
> @@ -2203,8 +2203,6 @@ BTRFS_SETGET_STACK_FUNCS(super_chunk_root_level, struct btrfs_super_block,
>  			 chunk_root_level, 8);
>  BTRFS_SETGET_STACK_FUNCS(super_log_root, struct btrfs_super_block,
>  			 log_root, 64);
> -BTRFS_SETGET_STACK_FUNCS(super_log_root_transid, struct btrfs_super_block,
> -			 log_root_transid, 64);
>  BTRFS_SETGET_STACK_FUNCS(super_log_root_level, struct btrfs_super_block,
>  			 log_root_level, 8);
>  BTRFS_SETGET_STACK_FUNCS(super_total_bytes, struct btrfs_super_block,
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  6:53   ` Nikolay Borisov
@ 2018-10-12  8:46     ` Qu Wenruo
  2018-10-12  9:13       ` Nikolay Borisov
  0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2018-10-12  8:46 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/10/12 下午2:53, Nikolay Borisov wrote:
> 
> 
> On 12.10.2018 09:42, Qu Wenruo wrote:
>> The only user of it is "btrfs inspect dump-super".
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  cmds-inspect-dump-super.c | 4 ++--
>>  ctree.h                   | 6 ++----
>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>> index e965267c5d96..3f33931ed9bc 100644
>> --- a/cmds-inspect-dump-super.c
>> +++ b/cmds-inspect-dump-super.c
>> @@ -387,8 +387,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>  	       (unsigned long long)btrfs_super_chunk_root_level(sb));
>>  	printf("log_root\t\t%llu\n",
>>  	       (unsigned long long)btrfs_super_log_root(sb));
>> -	printf("log_root_transid\t%llu\n",
>> -	       (unsigned long long)btrfs_super_log_root_transid(sb));
>> +	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.

Thanks,
Qu

> 
>>  	printf("log_root_level\t\t%llu\n",
>>  	       (unsigned long long)btrfs_super_log_root_level(sb));
>>  	printf("total_bytes\t\t%llu\n",
>> diff --git a/ctree.h b/ctree.h
>> index 4719962df67d..a314fdb102b0 100644
>> --- a/ctree.h
>> +++ b/ctree.h
>> @@ -427,8 +427,8 @@ struct btrfs_super_block {
>>  	__le64 chunk_root;
>>  	__le64 log_root;
>>  
>> -	/* this will help find the new super based on the log root */
>> -	__le64 log_root_transid;
>> +	/* This member is never touched, should always be 0 */
>> +	__le64 __unused_log_root_transid;
>>  	__le64 total_bytes;
>>  	__le64 bytes_used;
>>  	__le64 root_dir_objectid;
>> @@ -2203,8 +2203,6 @@ BTRFS_SETGET_STACK_FUNCS(super_chunk_root_level, struct btrfs_super_block,
>>  			 chunk_root_level, 8);
>>  BTRFS_SETGET_STACK_FUNCS(super_log_root, struct btrfs_super_block,
>>  			 log_root, 64);
>> -BTRFS_SETGET_STACK_FUNCS(super_log_root_transid, struct btrfs_super_block,
>> -			 log_root_transid, 64);
>>  BTRFS_SETGET_STACK_FUNCS(super_log_root_level, struct btrfs_super_block,
>>  			 log_root_level, 8);
>>  BTRFS_SETGET_STACK_FUNCS(super_total_bytes, struct btrfs_super_block,
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  8:46     ` Qu Wenruo
@ 2018-10-12  9:13       ` Nikolay Borisov
  2018-10-12  9:57         ` Qu Wenruo
  0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2018-10-12  9:13 UTC (permalink / raw)
  To: Qu Wenruo, Qu Wenruo, linux-btrfs



On 12.10.2018 11:46, Qu Wenruo wrote:
> 
> 
> On 2018/10/12 下午2:53, Nikolay Borisov wrote:
>>
>>
>> On 12.10.2018 09:42, Qu Wenruo wrote:
>>> The only user of it is "btrfs inspect dump-super".
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  cmds-inspect-dump-super.c | 4 ++--
>>>  ctree.h                   | 6 ++----
>>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>>> index e965267c5d96..3f33931ed9bc 100644
>>> --- a/cmds-inspect-dump-super.c
>>> +++ b/cmds-inspect-dump-super.c
>>> @@ -387,8 +387,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>  	       (unsigned long long)btrfs_super_chunk_root_level(sb));
>>>  	printf("log_root\t\t%llu\n",
>>>  	       (unsigned long long)btrfs_super_log_root(sb));
>>> -	printf("log_root_transid\t%llu\n",
>>> -	       (unsigned long long)btrfs_super_log_root_transid(sb));
>>> +	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?

> 
> Thanks,
> Qu
> 
>>
>>>  	printf("log_root_level\t\t%llu\n",
>>>  	       (unsigned long long)btrfs_super_log_root_level(sb));
>>>  	printf("total_bytes\t\t%llu\n",
>>> diff --git a/ctree.h b/ctree.h
>>> index 4719962df67d..a314fdb102b0 100644
>>> --- a/ctree.h
>>> +++ b/ctree.h
>>> @@ -427,8 +427,8 @@ struct btrfs_super_block {
>>>  	__le64 chunk_root;
>>>  	__le64 log_root;
>>>  
>>> -	/* this will help find the new super based on the log root */
>>> -	__le64 log_root_transid;
>>> +	/* This member is never touched, should always be 0 */
>>> +	__le64 __unused_log_root_transid;
>>>  	__le64 total_bytes;
>>>  	__le64 bytes_used;
>>>  	__le64 root_dir_objectid;
>>> @@ -2203,8 +2203,6 @@ BTRFS_SETGET_STACK_FUNCS(super_chunk_root_level, struct btrfs_super_block,
>>>  			 chunk_root_level, 8);
>>>  BTRFS_SETGET_STACK_FUNCS(super_log_root, struct btrfs_super_block,
>>>  			 log_root, 64);
>>> -BTRFS_SETGET_STACK_FUNCS(super_log_root_transid, struct btrfs_super_block,
>>> -			 log_root_transid, 64);
>>>  BTRFS_SETGET_STACK_FUNCS(super_log_root_level, struct btrfs_super_block,
>>>  			 log_root_level, 8);
>>>  BTRFS_SETGET_STACK_FUNCS(super_total_bytes, struct btrfs_super_block,
>>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  9:13       ` Nikolay Borisov
@ 2018-10-12  9:57         ` Qu Wenruo
  2018-10-24 14:30           ` David Sterba
  0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2018-10-12  9:57 UTC (permalink / raw)
  To: Nikolay Borisov, Qu Wenruo, linux-btrfs



On 2018/10/12 下午5:13, Nikolay Borisov wrote:
> 
> 
> On 12.10.2018 11:46, Qu Wenruo wrote:
>>
>>
>> On 2018/10/12 下午2:53, Nikolay Borisov wrote:
>>>
>>>
>>> On 12.10.2018 09:42, Qu Wenruo wrote:
>>>> The only user of it is "btrfs inspect dump-super".
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  cmds-inspect-dump-super.c | 4 ++--
>>>>  ctree.h                   | 6 ++----
>>>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
>>>> index e965267c5d96..3f33931ed9bc 100644
>>>> --- a/cmds-inspect-dump-super.c
>>>> +++ b/cmds-inspect-dump-super.c
>>>> @@ -387,8 +387,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>>>  	       (unsigned long long)btrfs_super_chunk_root_level(sb));
>>>>  	printf("log_root\t\t%llu\n",
>>>>  	       (unsigned long long)btrfs_super_log_root(sb));
>>>> -	printf("log_root_transid\t%llu\n",
>>>> -	       (unsigned long long)btrfs_super_log_root_transid(sb));
>>>> +	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.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>>  	printf("log_root_level\t\t%llu\n",
>>>>  	       (unsigned long long)btrfs_super_log_root_level(sb));
>>>>  	printf("total_bytes\t\t%llu\n",
>>>> diff --git a/ctree.h b/ctree.h
>>>> index 4719962df67d..a314fdb102b0 100644
>>>> --- a/ctree.h
>>>> +++ b/ctree.h
>>>> @@ -427,8 +427,8 @@ struct btrfs_super_block {
>>>>  	__le64 chunk_root;
>>>>  	__le64 log_root;
>>>>  
>>>> -	/* this will help find the new super based on the log root */
>>>> -	__le64 log_root_transid;
>>>> +	/* This member is never touched, should always be 0 */
>>>> +	__le64 __unused_log_root_transid;
>>>>  	__le64 total_bytes;
>>>>  	__le64 bytes_used;
>>>>  	__le64 root_dir_objectid;
>>>> @@ -2203,8 +2203,6 @@ BTRFS_SETGET_STACK_FUNCS(super_chunk_root_level, struct btrfs_super_block,
>>>>  			 chunk_root_level, 8);
>>>>  BTRFS_SETGET_STACK_FUNCS(super_log_root, struct btrfs_super_block,
>>>>  			 log_root, 64);
>>>> -BTRFS_SETGET_STACK_FUNCS(super_log_root_transid, struct btrfs_super_block,
>>>> -			 log_root_transid, 64);
>>>>  BTRFS_SETGET_STACK_FUNCS(super_log_root_level, struct btrfs_super_block,
>>>>  			 log_root_level, 8);
>>>>  BTRFS_SETGET_STACK_FUNCS(super_total_bytes, struct btrfs_super_block,
>>>>
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-12  9:57         ` Qu Wenruo
@ 2018-10-24 14:30           ` David Sterba
  2018-10-24 14:48             ` Nikolay Borisov
  0 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-10-24 14:30 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Nikolay Borisov, Qu Wenruo, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-24 14:30           ` David Sterba
@ 2018-10-24 14:48             ` Nikolay Borisov
  2018-10-25 11:18               ` David Sterba
  0 siblings, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2018-10-24 14:48 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs



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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-24 14:48             ` Nikolay Borisov
@ 2018-10-25 11:18               ` David Sterba
  2018-10-25 20:40                 ` Nikolay Borisov
  0 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2018-10-25 11:18 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: dsterba, Qu Wenruo, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid
  2018-10-25 11:18               ` David Sterba
@ 2018-10-25 20:40                 ` Nikolay Borisov
  0 siblings, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2018-10-25 20:40 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs



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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-10-25 20:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-12  6:42 [PATCH 1/2] btrfs-progs: find-root: Fix wrong generation for log tree Qu Wenruo
2018-10-12  6:42 ` [PATCH 2/2] btrfs-progs: Deprecate unused super block member log_root_transid Qu Wenruo
2018-10-12  6:53   ` Nikolay Borisov
2018-10-12  8:46     ` Qu Wenruo
2018-10-12  9:13       ` Nikolay Borisov
2018-10-12  9:57         ` Qu Wenruo
2018-10-24 14:30           ` David Sterba
2018-10-24 14:48             ` Nikolay Borisov
2018-10-25 11:18               ` David Sterba
2018-10-25 20:40                 ` Nikolay Borisov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).