Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS
@ 2020-02-12  7:46 Qu Wenruo
  2020-02-12 13:04 ` Josef Bacik
  2020-02-14 16:53 ` David Sterba
  0 siblings, 2 replies; 4+ messages in thread
From: Qu Wenruo @ 2020-02-12  7:46 UTC (permalink / raw)
  To: linux-btrfs

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


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

* Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS
  2020-02-12  7:46 [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS Qu Wenruo
@ 2020-02-12 13:04 ` Josef Bacik
  2020-02-14 16:53 ` David Sterba
  1 sibling, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2020-02-12 13:04 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs

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

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

* Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS
  2020-02-12  7:46 [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS Qu Wenruo
  2020-02-12 13:04 ` Josef Bacik
@ 2020-02-14 16:53 ` David Sterba
  2020-02-17  7:06   ` Qu Wenruo
  1 sibling, 1 reply; 4+ messages in thread
From: David Sterba @ 2020-02-14 16:53 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

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.

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

* Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS
  2020-02-14 16:53 ` David Sterba
@ 2020-02-17  7:06   ` Qu Wenruo
  0 siblings, 0 replies; 4+ messages in thread
From: Qu Wenruo @ 2020-02-17  7:06 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs

[-- 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 --]

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12  7:46 [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS Qu Wenruo
2020-02-12 13:04 ` Josef Bacik
2020-02-14 16:53 ` David Sterba
2020-02-17  7:06   ` Qu Wenruo

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git