Re: [PATCH] btrfs: Add comment for BTRFS_ROOT_REF_COWS

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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