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

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

[Wang Yugui]

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

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/02/19

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

[Holger Hoffstätte]

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?

-h

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

[Wang Yugui]

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.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/02/19

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

[Holger Hoffstätte]

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

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

[David Sterba]

On Fri, Feb 19, 2021 at 05:12:12PM +0100, Holger Hoffstätte wrote:
> On 2021-02-19 16:20, Wang Yugui wrote:
> > --- 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?

Git finds that the patch does not apply and lets the user to fix it.

I did git cherry-pick of 2f96e40212d435b3284 on v5.10.12 and got 2
conflicts:

first was in caching_thread around

	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))

that got resolved correctly, and the second one in the enum, but the
conflict was suggested in the right enum (lines 559+), so all I had to
do was to remove unmatched context and the <<< >>> markers. It's
possible that git version could affect that, mine is 2.29.2. Or stable
team does not use git for the intermediate patches and quilt did not get
it right.

I doubt that the conflict resolution was done incorrectly by hand, the
enums are quite far away so it would not be just a trivial change (like
context fixups) that are in the scope of semi-automatic stable
backports.

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

[David Sterba]

On Fri, Feb 19, 2021 at 11:20:51PM +0800, Wang Yugui wrote:
> > 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.

You're right, good catch.

The wrong value corresponds to BTRFS_FS_QUOTA_ENABLE in the right enum
set, so this could collide. With quotas enabled the on-line conversion
won't be possible as the free space tree would be considered untrusted.
The other way around, no quotas enabled by user, but with tree
conversion going on, then there are a lot of check for the bit set, now
it won't have the quota tree and other structures initialized. This
could be problmenatic.

I'll send a fixup.