[PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[Qu Wenruo]

This bit is being used in too many locations while there is still no
good enough explaination for how this bit is used.

Not to mention its name really doesn't make much sense.

So this patch will add my explanation on this bit, considering only
subvolume trees, along with its reloc trees have this bit, to me it
looks like this bit shows whether tree blocks of a root can be shared.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 36df977b64d9..61ab6e8c9a18 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -971,6 +971,15 @@ enum {
 	 * is used to tell us when more checks are required
 	 */
 	BTRFS_ROOT_IN_TRANS_SETUP,
+
+	/*
+	 * Whether tree blocks of a root can be shared.
+	 *
+	 * All subvolume trees, and their reloc trees, have this bit set.
+	 *
+	 * While all other trees, including essential trees like root, csum,
+	 * extent, chunk trees, and log trees don't have this bit set.
+	 */
 	BTRFS_ROOT_REF_COWS,
 	BTRFS_ROOT_TRACK_DIRTY,
 	BTRFS_ROOT_IN_RADIX,
-- 
2.25.0

Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[Josef Bacik]

On 2/12/20 2:46 AM, Qu Wenruo wrote:
> This bit is being used in too many locations while there is still no
> good enough explaination for how this bit is used.                  ^^
               explanation
> 
> Not to mention its name really doesn't make much sense.
> 
> So this patch will add my explanation on this bit, considering only
> subvolume trees, along with its reloc trees have this bit, to me it
> looks like this bit shows whether tree blocks of a root can be shared.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[David Sterba]

On Wed, Feb 12, 2020 at 03:46:51PM +0800, Qu Wenruo wrote:
> This bit is being used in too many locations while there is still no
> good enough explaination for how this bit is used.
> 
> Not to mention its name really doesn't make much sense.
> 
> So this patch will add my explanation on this bit, considering only
> subvolume trees, along with its reloc trees have this bit, to me it
> looks like this bit shows whether tree blocks of a root can be shared.

I think there's more tan just sharing, it should say something about
reference counted sharing. See eg. btrfs_block_can_be_shared:

 864         /*
 865          * Tree blocks not in reference counted trees and tree roots
 866          * are never shared. If a block was allocated after the last
 867          * snapshot and the block was not allocated by tree relocation,
 868          * we know the block is not shared.
 869          */

And there can be more specialities found when grepping for REF_COWS. The
comment explaination should be complete or at least mention what's not
documenting. The I find the suggested version insufficient but don't
have a concrete suggestions for improvement. By reading the comment and
going through code I don't feel any wiser.

Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[Qu Wenruo]

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



On 2020/2/15 上午12:53, David Sterba wrote:
> On Wed, Feb 12, 2020 at 03:46:51PM +0800, Qu Wenruo wrote:
>> This bit is being used in too many locations while there is still no
>> good enough explaination for how this bit is used.
>>
>> Not to mention its name really doesn't make much sense.
>>
>> So this patch will add my explanation on this bit, considering only
>> subvolume trees, along with its reloc trees have this bit, to me it
>> looks like this bit shows whether tree blocks of a root can be shared.
> 
> I think there's more tan just sharing, it should say something about
> reference counted sharing. See eg. btrfs_block_can_be_shared:
> 
>  864         /*
>  865          * Tree blocks not in reference counted trees and tree roots
>  866          * are never shared. If a block was allocated after the last
>  867          * snapshot and the block was not allocated by tree relocation,
>  868          * we know the block is not shared.
>  869          */
> 
> And there can be more specialities found when grepping for REF_COWS. The
> comment explaination should be complete or at least mention what's not
> documenting. The I find the suggested version insufficient but don't
> have a concrete suggestions for improvement. By reading the comment and
> going through code I don't feel any wiser.
> 

I see nothing extra conflicting the "shared tree blocks" part from
btrfs_block_can_be_shared().

In fact, reloc tree can only be created for trees with REF_COW bit.

For tree without that bit, we go a completely different way to relocate
them, by just cowing the path (aka the cowonly bit in build_backref_tree()).

	if (root) {
		if (test_bit(BTRFS_ROOT_REF_COWS, &root->state)) {
			BUG_ON(node->new_bytenr);
			BUG_ON(!list_empty(&node->list));
			btrfs_record_root_in_trans(trans, root);
			root = root->reloc_root;
			node->new_bytenr = root->node->start;
			node->root = root;
			list_add_tail(&node->list, &rc->backref_cache.changed);
		} else {
			path->lowest_level = node->level;
			ret = btrfs_search_slot(trans, root, key, path, 0, 1);  <<<
			btrfs_release_path(path);
			if (ret > 0)
				ret = 0;
		}

So the "REF_COW means tree blocks can be shared" still looks pretty
valid to me.

Thanks,
Qu


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

Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[Qu Wenruo]

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

Hi David,

Any extra comment on this?

From my recent dig into relocation code, the REF_COW really only means
if the tree blocks of one tree can be shared by multiple roots.
(Just like subvolume trees).

It mostly affects:
- How the root is updated at commit transaction
  This bit is conflicting with TRACK_DIRTY, thus we need to manually
  call record_root_in_trans() to mark one REF_COW root dirty.

- How relocation handles its tree blocks
  For non-REF_COW trees, relocation just uses COW to do the relocation,
  while for tree with such bit, it goes the path replace way.


Finally maybe a little off-topic, tree without REF_COW bit can still
contain inodes/extent data, like root tree and data reloc tree.
Root tree is the only exception where we have no REF_COW bit but still
allow file extents to be created in it.
(And I'm working on clear the REF_COW bit for data reloc tree, as we
can't create snapshot for that root).


Any more comment?

Thanks,
Qu

On 2020/2/17 下午3:06, Qu Wenruo wrote:
> 
> 
> On 2020/2/15 上午12:53, David Sterba wrote:
>> On Wed, Feb 12, 2020 at 03:46:51PM +0800, Qu Wenruo wrote:
>>> This bit is being used in too many locations while there is still no
>>> good enough explaination for how this bit is used.
>>>
>>> Not to mention its name really doesn't make much sense.
>>>
>>> So this patch will add my explanation on this bit, considering only
>>> subvolume trees, along with its reloc trees have this bit, to me it
>>> looks like this bit shows whether tree blocks of a root can be shared.
>>
>> I think there's more tan just sharing, it should say something about
>> reference counted sharing. See eg. btrfs_block_can_be_shared:
>>
>>  864         /*
>>  865          * Tree blocks not in reference counted trees and tree roots
>>  866          * are never shared. If a block was allocated after the last
>>  867          * snapshot and the block was not allocated by tree relocation,
>>  868          * we know the block is not shared.
>>  869          */
>>
>> And there can be more specialities found when grepping for REF_COWS. The
>> comment explaination should be complete or at least mention what's not
>> documenting. The I find the suggested version insufficient but don't
>> have a concrete suggestions for improvement. By reading the comment and
>> going through code I don't feel any wiser.
>>
> 
> I see nothing extra conflicting the "shared tree blocks" part from
> btrfs_block_can_be_shared().
> 
> In fact, reloc tree can only be created for trees with REF_COW bit.
> 
> For tree without that bit, we go a completely different way to relocate
> them, by just cowing the path (aka the cowonly bit in build_backref_tree()).
> 
> 	if (root) {
> 		if (test_bit(BTRFS_ROOT_REF_COWS, &root->state)) {
> 			BUG_ON(node->new_bytenr);
> 			BUG_ON(!list_empty(&node->list));
> 			btrfs_record_root_in_trans(trans, root);
> 			root = root->reloc_root;
> 			node->new_bytenr = root->node->start;
> 			node->root = root;
> 			list_add_tail(&node->list, &rc->backref_cache.changed);
> 		} else {
> 			path->lowest_level = node->level;
> 			ret = btrfs_search_slot(trans, root, key, path, 0, 1);  <<<
> 			btrfs_release_path(path);
> 			if (ret > 0)
> 				ret = 0;
> 		}
> 
> So the "REF_COW means tree blocks can be shared" still looks pretty
> valid to me.
> 
> Thanks,
> Qu
> 


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