On 2022/6/7 17:40, Filipe Manana wrote:

On Tue, Jun 07, 2022 at 01:09:13PM +0800, Qu Wenruo wrote:

...

[BUG] After creating a dirty log tree, although btrfs_super_block::log_root and log_root_level is correctly populated, its generation is still left 0:

log_root 30474240 log_root_transid 0 <<< log_root_level 0

[CAUSE] We just forgot to update btrfs_super_block::log_root_transid completely.

Thus it's always the original value (0) from the initial super block.

Thankfully this old behavior won't break log replay, as in btrfs_read_tree(), parent generation 0 means we just skip the generation

btrfs_read_tree() does not exists, it's btrfs_read_tree_root().

This is actually irrelevant, because we don't read the root log tree with btrfs_read_tree_root(). We use read_tree_block() for that (at btrfs_replay_log()),

Oh, right, I forgot to check that code, and just assumed every root read from superblock would has its generation checked, but it's not the case for log tree root.

and we use a generation matching the committed transaction + 1 (as it can never be anything else).

For every other log tree, we use btrfs_read_tree_root(), but the generation is stored in the root's root item stored in the root log tree.

The log_root_transid field was added to the super block by:

commit c3027eb5523d6983f12628f3fe13d8a7576db701 Author: Chris Mason chris.mason@oracle.com Date: Mon Dec 8 16:40:21 2008 -0500

 Btrfs: Add inode sequence number for NFS and reserved space in a few structs

But it was never used.

So this change is not needed.

Then, considering we have never really set log_root_transid anywhere, and in theory we're always using btrfs_super_block::generation + 1, why not just deprecate that member?

In fact, every time (thankfully not that many times for me) I checked log_root_transid, I can not help but to wonder why it's always 0.

Thanks, Qu

Thanks.

...

check.

And per-root log tree is still done properly using the root generation, so here we really only missed the generation check for log tree root, and even we fixed it, it should not cause any compatible problem.

[FIX] Just update btrfs_super_block::log_root_transid properly.

We don't need this.

The log_root_transid field was added to the super block by:

commit c3027eb5523d6983f12628f3fe13d8a7576db701 Author: Chris Mason chris.mason@oracle.com Date: Mon Dec 8 16:40:21 2008 -0500

 Btrfs: Add inode sequence number for NFS and reserved space in a few structs

But it was never used. For btrfs_read_tree_root(), what we use is the

...

Cc: stable@vger.kernel.org #4.9+ Signed-off-by: Qu Wenruo wqu@suse.com

---

fs/btrfs/tree-log.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 370388fadf96..27a76d6fef8c 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -3083,7 +3083,8 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, struct btrfs_log_ctx root_log_ctx; struct blk_plug plug; u64 log_root_start;

• u64 log_root_level;

• u64 log_root_transid;

• u8 log_root_level;

  mutex_lock(&root->log_mutex); log_transid = ctx->log_transid;

@@ -3297,6 +3298,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,

log_root_start = log_root_tree->node->start; log_root_level = btrfs_header_level(log_root_tree->node);

• log_root_transid = btrfs_header_generation(log_root_tree->node); log_root_tree->log_transid++; mutex_unlock(&log_root_tree->log_mutex);

@@ -3334,6 +3336,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,

btrfs_set_super_log_root(fs_info->super_for_commit, log_root_start); btrfs_set_super_log_root_level(fs_info->super_for_commit, log_root_level);

• btrfs_set_super_log_root_transid(fs_info->super_for_commit, log_root_transid); ret = write_all_supers(fs_info, 1); mutex_unlock(&fs_info->tree_log_mutex); if (ret) {

-- 2.36.1

On 2022/6/7 18:45, Filipe Manana wrote:

On Tue, Jun 07, 2022 at 06:30:03PM +0800, Qu Wenruo wrote:

...

On 2022/6/7 17:40, Filipe Manana wrote:

...

On Tue, Jun 07, 2022 at 01:09:13PM +0800, Qu Wenruo wrote:

...

[BUG] After creating a dirty log tree, although btrfs_super_block::log_root and log_root_level is correctly populated, its generation is still left 0:

log_root 30474240 log_root_transid 0 <<< log_root_level 0

[CAUSE] We just forgot to update btrfs_super_block::log_root_transid completely.

Thus it's always the original value (0) from the initial super block.

Thankfully this old behavior won't break log replay, as in btrfs_read_tree(), parent generation 0 means we just skip the generation

btrfs_read_tree() does not exists, it's btrfs_read_tree_root().

This is actually irrelevant, because we don't read the root log tree with btrfs_read_tree_root(). We use read_tree_block() for that (at btrfs_replay_log()),

Oh, right, I forgot to check that code, and just assumed every root read from superblock would has its generation checked, but it's not the case for log tree root.

...

and we use a generation matching the committed transaction + 1 (as it can never be anything else).

For every other log tree, we use btrfs_read_tree_root(), but the generation is stored in the root's root item stored in the root log tree.

The log_root_transid field was added to the super block by:

commit c3027eb5523d6983f12628f3fe13d8a7576db701 Author: Chris Mason chris.mason@oracle.com Date: Mon Dec 8 16:40:21 2008 -0500

  Btrfs: Add inode sequence number for NFS and reserved space in a few structs

But it was never used.

So this change is not needed.

Then, considering we have never really set log_root_transid anywhere, and in theory we're always using btrfs_super_block::generation + 1, why

It's not in theory only, it's in practice too. We use committed generation + 1 since the first day the log tree was added:

commit e02119d5a7b4396c5a872582fddc8bd6d305a70a Author: Chris Mason chris.mason@oracle.com Date: Fri Sep 5 16:13:11 2008 -0400

 Btrfs: Add a write ahead tree log to optimize synchronous operations

(back then we read the root log tree directly at open_ctree()).

...

not just deprecate that member?

In fact, every time (thankfully not that many times for me) I checked log_root_transid, I can not help but to wonder why it's always 0.

You'd have to ask Chris why he added the field if it was never used and, as far as I can see, was always useless since the life of a log tree has never spanned more than 1 transaction, its generation must necessarily be "committed transaction generation + 1".

Maybe just add a comment on top of the field saying it's unused and should always be 0.

It's technically part of the disk format, so it's probably better not being renamed to '__le64 unused;'.

OK, I'd follow the old leafsize way to rename it to __le64 __unused_log_root_transid.

I'm asking because some newer feature will going to add something very similar to log tree (write-intent tree, just like the write-intent bitmap from DM code, for RAID56 mount time scrub).

Initially I was not that confident about the extra tree root, tree level, tree transid, extra write-intent generation.

But it looks like we're fine to contain at least 25 u64, so it should be mostly fine, and no need to reuse old members like leafsize nor this log_tree_transid.

Thanks, Qu

Thanks.

...

Thanks, Qu

...

Thanks.

...

check.

And per-root log tree is still done properly using the root generation, so here we really only missed the generation check for log tree root, and even we fixed it, it should not cause any compatible problem.

[FIX] Just update btrfs_super_block::log_root_transid properly.

We don't need this.

The log_root_transid field was added to the super block by:

commit c3027eb5523d6983f12628f3fe13d8a7576db701 Author: Chris Mason chris.mason@oracle.com Date: Mon Dec 8 16:40:21 2008 -0500

  Btrfs: Add inode sequence number for NFS and reserved space in a few structs

But it was never used. For btrfs_read_tree_root(), what we use is the

...

Cc: stable@vger.kernel.org #4.9+ Signed-off-by: Qu Wenruo wqu@suse.com

---

fs/btrfs/tree-log.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 370388fadf96..27a76d6fef8c 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -3083,7 +3083,8 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans, struct btrfs_log_ctx root_log_ctx; struct blk_plug plug; u64 log_root_start;

• u64 log_root_level;

• u64 log_root_transid;

• u8 log_root_level;

  mutex_lock(&root->log_mutex); log_transid = ctx->log_transid;

@@ -3297,6 +3298,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,

log_root_start = log_root_tree->node->start;
log_root_level = btrfs_header_level(log_root_tree->node);

• log_root_transid = btrfs_header_generation(log_root_tree->node); log_root_tree->log_transid++; mutex_unlock(&log_root_tree->log_mutex);

@@ -3334,6 +3336,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,

btrfs_set_super_log_root(fs_info->super_for_commit, log_root_start);
btrfs_set_super_log_root_level(fs_info->super_for_commit, log_root_level);

• btrfs_set_super_log_root_transid(fs_info->super_for_commit, log_root_transid); ret = write_all_supers(fs_info, 1); mutex_unlock(&fs_info->tree_log_mutex); if (ret) {

-- 2.36.1