| Message ID | 20200212074651.33008-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Add comment for BTRFS_ROOT_REF_COWS | expand |
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
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.
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
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 >
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,
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(+)