[PATCH 1/2] btrfs: Make btrfs_(set|clear)_header_flag return void

[PATCH 1/2] btrfs: Make btrfs_(set|clear)_header_flag return void

[Qu Wenruo]

From the introduce of btrfs_(set|clear)_header_flag, there is no usage
of its return value.

So just make it return void.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index b3642367a595..c03852d1aa34 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2163,18 +2163,16 @@ static inline int btrfs_header_flag(const struct extent_buffer *eb, u64 flag)
 	return (btrfs_header_flags(eb) & flag) == flag;
 }
 
-static inline int btrfs_set_header_flag(struct extent_buffer *eb, u64 flag)
+static inline void btrfs_set_header_flag(struct extent_buffer *eb, u64 flag)
 {
 	u64 flags = btrfs_header_flags(eb);
 	btrfs_set_header_flags(eb, flags | flag);
-	return (flags & flag) == flag;
 }
 
-static inline int btrfs_clear_header_flag(struct extent_buffer *eb, u64 flag)
+static inline void btrfs_clear_header_flag(struct extent_buffer *eb, u64 flag)
 {
 	u64 flags = btrfs_header_flags(eb);
 	btrfs_set_header_flags(eb, flags & ~flag);
-	return (flags & flag) == flag;
 }
 
 static inline int btrfs_header_backref_rev(const struct extent_buffer *eb)
-- 
2.21.0

[PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[Qu Wenruo]

This new tree block flag is to indicate the tree block belongs to a log
tree.

For btrfs on-disk format, there are several different trees could use
the same owner number:
- ordinary subvolume tree
- log tree for ordinary subvolume
- reloc tree for ordinary subvolume

It's possible to do the backref walk or use the content to determine the
real owner of the tree block.
But backref walk is too expensive, content detection is not reliable.

So adding a new flag to explicitly show the owner of log tree.

Thankfully, tree-checker hasn't checked header flags yet, and there is
no real user for this flag yet.
So we don't need to introduce any compatibility flag for this
modification.

The introduction of this new flag makes tree block owner easier to
distinguish, making life easier for:
- tree checker on log tree
  Log tree could contain some items impossible for regular trees
- performance profiler
  Now it's much easier to distinguish log tree.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c              | 1 +
 include/uapi/linux/btrfs_tree.h | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6fe9197f6ee4..8cffdb7789d8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1362,6 +1362,7 @@ static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,
 		return ERR_CAST(leaf);
 	}
 
+	btrfs_set_header_flag(leaf, BTRFS_HEADER_FLAG_LOG);
 	root->node = leaf;
 
 	btrfs_mark_buffer_dirty(root->node);
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index e974f4bb5378..d4ef80b702a0 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -447,9 +447,15 @@ struct btrfs_free_space_header {
 	__le64 num_bitmaps;
 } __attribute__ ((__packed__));
 
+/* Tree/super has written to disk */
 #define BTRFS_HEADER_FLAG_WRITTEN	(1ULL << 0)
+
+/* Tree block which gets relocated */
 #define BTRFS_HEADER_FLAG_RELOC		(1ULL << 1)
 
+/* Tree block which belows to log tree */
+#define BTRFS_HEADER_FLAG_LOG		(1ULL << 2)
+
 /* Super block flags */
 /* Errors detected */
 #define BTRFS_SUPER_FLAG_ERROR		(1ULL << 2)
-- 
2.21.0

Re: [PATCH 1/2] btrfs: Make btrfs_(set|clear)_header_flag return void

[Nikolay Borisov]

On 19.03.19 г. 8:04 ч., Qu Wenruo wrote:
> From the introduce of btrfs_(set|clear)_header_flag, there is no usage
> of its return value.
> 
> So just make it return void.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/ctree.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index b3642367a595..c03852d1aa34 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2163,18 +2163,16 @@ static inline int btrfs_header_flag(const struct extent_buffer *eb, u64 flag)
>  	return (btrfs_header_flags(eb) & flag) == flag;
>  }
>  
> -static inline int btrfs_set_header_flag(struct extent_buffer *eb, u64 flag)
> +static inline void btrfs_set_header_flag(struct extent_buffer *eb, u64 flag)
>  {
>  	u64 flags = btrfs_header_flags(eb);
>  	btrfs_set_header_flags(eb, flags | flag);
> -	return (flags & flag) == flag;
>  }
>  
> -static inline int btrfs_clear_header_flag(struct extent_buffer *eb, u64 flag)
> +static inline void btrfs_clear_header_flag(struct extent_buffer *eb, u64 flag)
>  {
>  	u64 flags = btrfs_header_flags(eb);
>  	btrfs_set_header_flags(eb, flags & ~flag);
> -	return (flags & flag) == flag;
>  }
>  
>  static inline int btrfs_header_backref_rev(const struct extent_buffer *eb)
> 

Re: [PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[Nikolay Borisov]

On 19.03.19 г. 8:04 ч., Qu Wenruo wrote:
> This new tree block flag is to indicate the tree block belongs to a log
> tree.
> 
> For btrfs on-disk format, there are several different trees could use
> the same owner number:
> - ordinary subvolume tree
> - log tree for ordinary subvolume
> - reloc tree for ordinary subvolume
> 
> It's possible to do the backref walk or use the content to determine the
> real owner of the tree block.
> But backref walk is too expensive, content detection is not reliable.
> 
> So adding a new flag to explicitly show the owner of log tree.
> 
> Thankfully, tree-checker hasn't checked header flags yet, and there is
> no real user for this flag yet.
> So we don't need to introduce any compatibility flag for this
> modification.
> 
> The introduction of this new flag makes tree block owner easier to
> distinguish, making life easier for:
> - tree checker on log tree
>   Log tree could contain some items impossible for regular trees
> - performance profiler
>   Now it's much easier to distinguish log tree.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c              | 1 +
>  include/uapi/linux/btrfs_tree.h | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 6fe9197f6ee4..8cffdb7789d8 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1362,6 +1362,7 @@ static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,
>  		return ERR_CAST(leaf);
>  	}
>  
> +	btrfs_set_header_flag(leaf, BTRFS_HEADER_FLAG_LOG);
>  	root->node = leaf;

Unless you are going to tag ALL extent buffers which are allocated as
part of the log tree this patch is redundant, because even now you can
detect the root of the log tree by simply checking for the
root->root_key.objectid. Alternatively if you are going to be tagging
all such blocks then those 2 patches need to be part of a larger series
so the intended usage is more clear.

>  
>  	btrfs_mark_buffer_dirty(root->node);
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index e974f4bb5378..d4ef80b702a0 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -447,9 +447,15 @@ struct btrfs_free_space_header {
>  	__le64 num_bitmaps;
>  } __attribute__ ((__packed__));
>  
> +/* Tree/super has written to disk */
tree block has been correctly written to disk

>  #define BTRFS_HEADER_FLAG_WRITTEN	(1ULL << 0)
> +
> +/* Tree block which gets relocated */

tree block belongs to a relocation tree

>  #define BTRFS_HEADER_FLAG_RELOC		(1ULL << 1)
>  
> +/* Tree block which belows to log tree */
> +#define BTRFS_HEADER_FLAG_LOG		(1ULL << 2)
> +
>  /* Super block flags */
>  /* Errors detected */
>  #define BTRFS_SUPER_FLAG_ERROR		(1ULL << 2)
> 

Re: [PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[Qu Wenruo]

On 2019/3/19 下午3:04, Nikolay Borisov wrote:
> 
> 
> On 19.03.19 г. 8:04 ч., Qu Wenruo wrote:
>> This new tree block flag is to indicate the tree block belongs to a log
>> tree.
>>
>> For btrfs on-disk format, there are several different trees could use
>> the same owner number:
>> - ordinary subvolume tree
>> - log tree for ordinary subvolume
>> - reloc tree for ordinary subvolume
>>
>> It's possible to do the backref walk or use the content to determine the
>> real owner of the tree block.
>> But backref walk is too expensive, content detection is not reliable.
>>
>> So adding a new flag to explicitly show the owner of log tree.
>>
>> Thankfully, tree-checker hasn't checked header flags yet, and there is
>> no real user for this flag yet.
>> So we don't need to introduce any compatibility flag for this
>> modification.
>>
>> The introduction of this new flag makes tree block owner easier to
>> distinguish, making life easier for:
>> - tree checker on log tree
>>   Log tree could contain some items impossible for regular trees
>> - performance profiler
>>   Now it's much easier to distinguish log tree.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/disk-io.c              | 1 +
>>  include/uapi/linux/btrfs_tree.h | 6 ++++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 6fe9197f6ee4..8cffdb7789d8 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1362,6 +1362,7 @@ static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,
>>  		return ERR_CAST(leaf);
>>  	}
>>  
>> +	btrfs_set_header_flag(leaf, BTRFS_HEADER_FLAG_LOG);
>>  	root->node = leaf;
> 
> Unless you are going to tag ALL extent buffers which are allocated as
> part of the log tree this patch is redundant, because even now you can
> detect the root of the log tree by simply checking for the
> root->root_key.objectid.

That is obvious.

As mentioned, the purpose is to detect owner for root->log_root.
So it's to tag all log tree blocks.

> Alternatively if you are going to be tagging
> all such blocks then those 2 patches need to be part of a larger series
> so the intended usage is more clear.

The problem is, the larger part is not going to be a kernel patch.

Mostly btrfs-progs patch and 3rd-part kernel profiling tool.

> 
>>  
>>  	btrfs_mark_buffer_dirty(root->node);
>> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
>> index e974f4bb5378..d4ef80b702a0 100644
>> --- a/include/uapi/linux/btrfs_tree.h
>> +++ b/include/uapi/linux/btrfs_tree.h
>> @@ -447,9 +447,15 @@ struct btrfs_free_space_header {
>>  	__le64 num_bitmaps;
>>  } __attribute__ ((__packed__));
>>  
>> +/* Tree/super has written to disk */
> tree block has been correctly written to disk

The trick here is, super block reuses this flag.
So we still need to mention the super block part.

Thanks,
Qu

> 
>>  #define BTRFS_HEADER_FLAG_WRITTEN	(1ULL << 0)
>> +
>> +/* Tree block which gets relocated */
> 
> tree block belongs to a relocation tree
> 
>>  #define BTRFS_HEADER_FLAG_RELOC		(1ULL << 1)
>>  
>> +/* Tree block which belows to log tree */
>> +#define BTRFS_HEADER_FLAG_LOG		(1ULL << 2)
>> +
>>  /* Super block flags */
>>  /* Errors detected */
>>  #define BTRFS_SUPER_FLAG_ERROR		(1ULL << 2)
>>

Re: [PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2651 bytes --]



On 2019/3/19 下午2:04, Qu Wenruo wrote:
> This new tree block flag is to indicate the tree block belongs to a log
> tree.
> 
> For btrfs on-disk format, there are several different trees could use
> the same owner number:
> - ordinary subvolume tree
> - log tree for ordinary subvolume
> - reloc tree for ordinary subvolume
> 
> It's possible to do the backref walk or use the content to determine the
> real owner of the tree block.
> But backref walk is too expensive, content detection is not reliable.
> 
> So adding a new flag to explicitly show the owner of log tree.
> 
> Thankfully, tree-checker hasn't checked header flags yet, and there is
> no real user for this flag yet.
> So we don't need to introduce any compatibility flag for this
> modification.
> 
> The introduction of this new flag makes tree block owner easier to
> distinguish, making life easier for:
> - tree checker on log tree
>   Log tree could contain some items impossible for regular trees
> - performance profiler
>   Now it's much easier to distinguish log tree.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Please discard this patch, it lacks header flags inherit part.

E.g. when splitting tree block, the higher level and brother tree block
doesn't inherit the header bit.

I'll address the problem in next version.

Thanks,
Qu

> ---
>  fs/btrfs/disk-io.c              | 1 +
>  include/uapi/linux/btrfs_tree.h | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 6fe9197f6ee4..8cffdb7789d8 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1362,6 +1362,7 @@ static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,
>  		return ERR_CAST(leaf);
>  	}
>  
> +	btrfs_set_header_flag(leaf, BTRFS_HEADER_FLAG_LOG);
>  	root->node = leaf;
>  
>  	btrfs_mark_buffer_dirty(root->node);
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index e974f4bb5378..d4ef80b702a0 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -447,9 +447,15 @@ struct btrfs_free_space_header {
>  	__le64 num_bitmaps;
>  } __attribute__ ((__packed__));
>  
> +/* Tree/super has written to disk */
>  #define BTRFS_HEADER_FLAG_WRITTEN	(1ULL << 0)
> +
> +/* Tree block which gets relocated */
>  #define BTRFS_HEADER_FLAG_RELOC		(1ULL << 1)
>  
> +/* Tree block which belows to log tree */
> +#define BTRFS_HEADER_FLAG_LOG		(1ULL << 2)
> +
>  /* Super block flags */
>  /* Errors detected */
>  #define BTRFS_SUPER_FLAG_ERROR		(1ULL << 2)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] btrfs: Make btrfs_(set|clear)_header_flag return void

[David Sterba]

On Tue, Mar 19, 2019 at 02:04:17PM +0800, Qu Wenruo wrote:
> >From the introduce of btrfs_(set|clear)_header_flag, there is no usage
> of its return value.
> 
> So just make it return void.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This is an independent patch so I'll add it to misc-next, thanks.

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[David Sterba]

On Tue, Mar 19, 2019 at 02:04:18PM +0800, Qu Wenruo wrote:
> This new tree block flag is to indicate the tree block belongs to a log
> tree.
> 
> For btrfs on-disk format, there are several different trees could use
> the same owner number:
> - ordinary subvolume tree
> - log tree for ordinary subvolume
> - reloc tree for ordinary subvolume
> 
> It's possible to do the backref walk or use the content to determine the
> real owner of the tree block.
> But backref walk is too expensive, content detection is not reliable.
> 
> So adding a new flag to explicitly show the owner of log tree.
> 
> Thankfully, tree-checker hasn't checked header flags yet, and there is
> no real user for this flag yet.
> So we don't need to introduce any compatibility flag for this
> modification.

I think we can't simply skip the backward compatibility here, this is a
change of on-disk format and at least we have to understand what can go
wrong when the combinations of new/old kernel/fs are used.

> The introduction of this new flag makes tree block owner easier to
> distinguish, making life easier for:
> - tree checker on log tree
>   Log tree could contain some items impossible for regular trees
> - performance profiler
>   Now it's much easier to distinguish log tree.

So is this for enhanced correctness or performance helper? I'd tend to
skip the performance part for now, it's not convincing argument for the
on-disk change.

And for the correctness I'd like more details, but the idea sounds good
in general.

Re: [PATCH 2/2] btrfs: Introduce new tree block flag, BTRFS_HEADER_FLAG_LOG

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2565 bytes --]



On 2019/3/20 上午2:23, David Sterba wrote:
> On Tue, Mar 19, 2019 at 02:04:18PM +0800, Qu Wenruo wrote:
>> This new tree block flag is to indicate the tree block belongs to a log
>> tree.
>>
>> For btrfs on-disk format, there are several different trees could use
>> the same owner number:
>> - ordinary subvolume tree
>> - log tree for ordinary subvolume
>> - reloc tree for ordinary subvolume
>>
>> It's possible to do the backref walk or use the content to determine the
>> real owner of the tree block.
>> But backref walk is too expensive, content detection is not reliable.
>>
>> So adding a new flag to explicitly show the owner of log tree.
>>
>> Thankfully, tree-checker hasn't checked header flags yet, and there is
>> no real user for this flag yet.
>> So we don't need to introduce any compatibility flag for this
>> modification.
> 
> I think we can't simply skip the backward compatibility here, this is a
> change of on-disk format and at least we have to understand what can go
> wrong when the combinations of new/old kernel/fs are used.

Currently since neither new fs nor old fs cares the unused bit in extent
buffer headers, so it won't cause anything different.

But if newer fs is going to rely on this bit to do tree-checker, then we
should introduce a COMPATIBLE bit.
Without that bit, kernel do no tagging, to be backward compatible.

> 
>> The introduction of this new flag makes tree block owner easier to
>> distinguish, making life easier for:
>> - tree checker on log tree
>>   Log tree could contain some items impossible for regular trees
>> - performance profiler
>>   Now it's much easier to distinguish log tree.
> 
> So is this for enhanced correctness or performance helper? I'd tend to
> skip the performance part for now, it's not convincing argument for the
> on-disk change.
> 
> And for the correctness I'd like more details, but the idea sounds good
> in general.

The chaos of eb->owner is, we have 3 root referring to one subvolid:
ordinary root
root->reloc_root
root->log_root

For ordinary root and reloc root, they're the completely the same
content, so tree-checker needs no modification at all.

But for log_root, its content is completely different, it can contain
things like EXTENT_CSUM.

With the ability to know if a tree block is a log tree one, we can
finally implement owner based key type checker.
E.g. if we have an EXTENT_CSUM item in subvol 256 and it's not a log
tree eb, then we know something is definitely wrong.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]