btrfs: Add comment for BTRFS_ROOT_REF_COWS
diff mbox series

Message ID 20200212074651.33008-1-wqu@suse.com
State New
Headers show
Series
  • btrfs: Add comment for BTRFS_ROOT_REF_COWS
Related show

Commit Message

Qu Wenruo Feb. 12, 2020, 7:46 a.m. UTC
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(+)

Comments

Josef Bacik Feb. 12, 2020, 1:04 p.m. UTC | #1
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
David Sterba Feb. 14, 2020, 4:53 p.m. UTC | #2
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.
Qu Wenruo Feb. 17, 2020, 7:06 a.m. UTC | #3
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

Patch
diff mbox series

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,