Re: [PATCH] btrfs: correctly populate btrfs_super_block::log_root_transid

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

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