Re: error in backport of 'btrfs: fix possible free space tree corruption with online conversion'

["Holger Hoffstätte" <holger@applied-asynchrony.com>]

On 2021-02-19 16:20, Wang Yugui wrote:
> Hi,
> 
>> On 2021-02-19 04:17, Wang Yugui wrote:
>>> Hi, Josef Bacik
>>>
>>> We noticed an error in 5.10.x backport of 'btrfs: fix possible free
>>> space tree corruption with online conversion'
>>>
>>> It is wrong in 5.10.13, but right in 5.11.
>>>
>>> 5.10.13
>>> @@ -146,6 +146,9 @@ enum {
>>>    	BTRFS_FS_STATE_DEV_REPLACING,
>>>    	/* The btrfs_fs_info created for self-tests */
>>>    	BTRFS_FS_STATE_DUMMY_FS_INFO,
>>> +
>>> +	/* Indicate that we can't trust the free space tree for caching yet */
>>> +	BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED,
>>>    };
>>>
>>> the usage sample of this enum:
>>> set_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
>>>
>>>
>>> 5.11
>>> enum{
>>> ..
>>>       /* Indicate that the discard workqueue can service discards. */
>>>       BTRFS_FS_DISCARD_RUNNING,
>>>
>>>       /* Indicate that we need to cleanup space cache v1 */
>>>       BTRFS_FS_CLEANUP_SPACE_CACHE_V1,
>>>
>>>       /* Indicate that we can't trust the free space tree for caching yet */
>>>       BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED,
>>> };
>>>
>>> the usage sample of this enum:
>>> set_bit(BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED, &fs_info->flags);
>>>
>> Out of curiosity I decided to check how this happened, but don't see it.
>> Here is the commit that went into 5.10.13 and it looks correct to me:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.10.y&id=2175bf57dc9522c58d93dcd474758434a3f05c57
> 
>> The patch that went into 5.10 looks identical to the original commit in 5.11.
>> What tree are you looking at?
> 
> the 5.10.y is the URL that you point out.
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.10.y&id=2175bf57dc9522c58d93dcd474758434a3f05c57
> 
> but the right one for 5.11 is
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/btrfs?id=2f96e40212d435b328459ba6b3956395eed8fa9f
> 
> 5.11:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/btrfs?id=2f96e40212d435b328459ba6b3956395eed8fa9f
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 0225c5208f44c..47ca8edafb5e6 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -564,6 +564,9 @@ enum {
>   
>   	/* Indicate that we need to cleanup space cache v1 */
>   	BTRFS_FS_CLEANUP_SPACE_CACHE_V1,
> +
> +	/* Indicate that we can't trust the free space tree for caching yet */
> +	BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED,
>   };
>   
>   /*
> 
> but 5.10.y:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.10.y&id=2175bf57dc9522c58d93dcd474758434a3f05c57
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index e01545538e07f..30ea9780725ff 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -146,6 +146,9 @@ enum {
>   	BTRFS_FS_STATE_DEV_REPLACING,
>   	/* The btrfs_fs_info created for self-tests */
>   	BTRFS_FS_STATE_DUMMY_FS_INFO,
> +
> +	/* Indicate that we can't trust the free space tree for caching yet */
> +	BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED,
>   };
>   
>   #define BTRFS_BACKREF_REV_MAX		256
> 
> Both the line(Line:146 vs Line:564) and the content are wrong.
> 

Ahh..now I understand, indeed the merge of BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED went into
the wrong enum. I misunderstood your original posting to mean that it had somehow missed
a chunk or used the wrong enum value in set_bit.

Anyway, good catch! I guess Dave needs to decide how to fix this, maybe
let Greg revert & re-apply properly.

Can anybody explain why git decided to do this?

-h