[PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We are incorrectly dropping the raid56 and raid1c34 incompat flags when
there are still raid56 and raid1c34 block groups, not when we do not any
of those anymore. The logic just got unintentionally broken after adding
the support for the raid1c34 modes.

Fix this by clear the flags only if we do not have block groups with the
respective profiles.

Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 7b003a2df79e..b8f39a679064 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -856,9 +856,9 @@ static void clear_incompat_bg_bits(struct btrfs_fs_info *fs_info, u64 flags)
 				found_raid1c34 = true;
 			up_read(&sinfo->groups_sem);
 		}
-		if (found_raid56)
+		if (!found_raid56)
 			btrfs_clear_fs_incompat(fs_info, RAID56);
-		if (found_raid1c34)
+		if (!found_raid1c34)
 			btrfs_clear_fs_incompat(fs_info, RAID1C34);
 	}
 }
-- 
2.25.0

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[David Sterba]

On Fri, Mar 20, 2020 at 06:43:48PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We are incorrectly dropping the raid56 and raid1c34 incompat flags when
> there are still raid56 and raid1c34 block groups, not when we do not any
> of those anymore. The logic just got unintentionally broken after adding
> the support for the raid1c34 modes.
> 
> Fix this by clear the flags only if we do not have block groups with the
> respective profiles.
> 
> Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Thanks, that was a stupid mistake. As it's a regression fix, I'll send
as an rc fix.

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1468 bytes --]



On 2020/3/21 上午2:43, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We are incorrectly dropping the raid56 and raid1c34 incompat flags when
> there are still raid56 and raid1c34 block groups, not when we do not any
> of those anymore. The logic just got unintentionally broken after adding
> the support for the raid1c34 modes.
> 
> Fix this by clear the flags only if we do not have block groups with the
> respective profiles.
> 
> Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

The fix is OK.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just interesting do we really need to remove such flags?
To me, keep the flag is completely sane.

Thanks,
Qu
> ---
>  fs/btrfs/block-group.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 7b003a2df79e..b8f39a679064 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -856,9 +856,9 @@ static void clear_incompat_bg_bits(struct btrfs_fs_info *fs_info, u64 flags)
>  				found_raid1c34 = true;
>  			up_read(&sinfo->groups_sem);
>  		}
> -		if (found_raid56)
> +		if (!found_raid56)
>  			btrfs_clear_fs_incompat(fs_info, RAID56);
> -		if (found_raid1c34)
> +		if (!found_raid1c34)
>  			btrfs_clear_fs_incompat(fs_info, RAID1C34);
>  	}
>  }
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[David Sterba]

On Sat, Mar 21, 2020 at 09:43:21AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/3/21 上午2:43, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > We are incorrectly dropping the raid56 and raid1c34 incompat flags when
> > there are still raid56 and raid1c34 block groups, not when we do not any
> > of those anymore. The logic just got unintentionally broken after adding
> > the support for the raid1c34 modes.
> > 
> > Fix this by clear the flags only if we do not have block groups with the
> > respective profiles.
> > 
> > Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> 
> The fix is OK.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Just interesting do we really need to remove such flags?
> To me, keep the flag is completely sane.

So you'd suggest to keep a flag for a feature that's not used on the
filesystem so it's not possible to mount the filesystem on an older
kernel?

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]



On 2020/3/22 上午1:45, David Sterba wrote:
> On Sat, Mar 21, 2020 at 09:43:21AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/3/21 上午2:43, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> We are incorrectly dropping the raid56 and raid1c34 incompat flags when
>>> there are still raid56 and raid1c34 block groups, not when we do not any
>>> of those anymore. The logic just got unintentionally broken after adding
>>> the support for the raid1c34 modes.
>>>
>>> Fix this by clear the flags only if we do not have block groups with the
>>> respective profiles.
>>>
>>> Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>
>> The fix is OK.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Just interesting do we really need to remove such flags?
>> To me, keep the flag is completely sane.
> 
> So you'd suggest to keep a flag for a feature that's not used on the
> filesystem so it's not possible to mount the filesystem on an older
> kernel?
> 
If user is using this feature, they aren't expecting mounting it on
older kernel either.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[David Sterba]

On Sun, Mar 22, 2020 at 08:42:20AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/3/22 上午1:45, David Sterba wrote:
> > On Sat, Mar 21, 2020 at 09:43:21AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2020/3/21 上午2:43, fdmanana@kernel.org wrote:
> >>> From: Filipe Manana <fdmanana@suse.com>
> >>>
> >>> We are incorrectly dropping the raid56 and raid1c34 incompat flags when
> >>> there are still raid56 and raid1c34 block groups, not when we do not any
> >>> of those anymore. The logic just got unintentionally broken after adding
> >>> the support for the raid1c34 modes.
> >>>
> >>> Fix this by clear the flags only if we do not have block groups with the
> >>> respective profiles.
> >>>
> >>> Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >>
> >> The fix is OK.
> >>
> >> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >>
> >> Just interesting do we really need to remove such flags?
> >> To me, keep the flag is completely sane.
> > 
> > So you'd suggest to keep a flag for a feature that's not used on the
> > filesystem so it's not possible to mount the filesystem on an older
> > kernel?
> > 
> If user is using this feature, they aren't expecting mounting it on
> older kernel either.

Before we go in a loop throwing single statements, let me take a broader
look.

First thing, the removal of incompat bit was asked for by users, Hugo is
as reporter in the commit 6d58a55a894e863.

https://lore.kernel.org/linux-btrfs/20190610144806.GB21016@carfax.org.uk/

  "   We've had a couple of cases in the past where people have tried out
  a new feature on a new kernel, then turned it off again and not been
  able to go back to an earlier kernel. Particularly in this case, I can
  see people being surprised at the trapdoor. "I don't have any RAID13C
  on this filesystem: why can't I go back to 5.2?"

That itself is a strong sign to me that there's a need or usecase or a
good idea. Though we have a lot of them, this one was simple to
implement and made sense to me. For the raid56 it's a simple check,
unlike for other features that would need to go through significant
portion of metadata.

Booting older kernel might sound like, why would anybody want to do
that, but if the bit is there preventing boot/mount, then it's an
unnecessary complication. In rescue environmnents it's gold.

Usability improvements tend to be boring from code POV but it is
something that users can observe and make use of.

Re: [PATCH] Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2749 bytes --]



On 2020/3/24 上午3:28, David Sterba wrote:
> On Sun, Mar 22, 2020 at 08:42:20AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/3/22 上午1:45, David Sterba wrote:
>>> On Sat, Mar 21, 2020 at 09:43:21AM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2020/3/21 上午2:43, fdmanana@kernel.org wrote:
>>>>> From: Filipe Manana <fdmanana@suse.com>
>>>>>
>>>>> We are incorrectly dropping the raid56 and raid1c34 incompat flags when
>>>>> there are still raid56 and raid1c34 block groups, not when we do not any
>>>>> of those anymore. The logic just got unintentionally broken after adding
>>>>> the support for the raid1c34 modes.
>>>>>
>>>>> Fix this by clear the flags only if we do not have block groups with the
>>>>> respective profiles.
>>>>>
>>>>> Fixes: 9c907446dce3 ("btrfs: drop incompat bit for raid1c34 after last block group is gone")
>>>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>>>
>>>> The fix is OK.
>>>>
>>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>>>
>>>> Just interesting do we really need to remove such flags?
>>>> To me, keep the flag is completely sane.
>>>
>>> So you'd suggest to keep a flag for a feature that's not used on the
>>> filesystem so it's not possible to mount the filesystem on an older
>>> kernel?
>>>
>> If user is using this feature, they aren't expecting mounting it on
>> older kernel either.
> 
> Before we go in a loop throwing single statements, let me take a broader
> look.
> 
> First thing, the removal of incompat bit was asked for by users, Hugo is
> as reporter in the commit 6d58a55a894e863.
> 
> https://lore.kernel.org/linux-btrfs/20190610144806.GB21016@carfax.org.uk/
> 
>   "   We've had a couple of cases in the past where people have tried out
>   a new feature on a new kernel, then turned it off again and not been
>   able to go back to an earlier kernel. Particularly in this case, I can
>   see people being surprised at the trapdoor. "I don't have any RAID13C
>   on this filesystem: why can't I go back to 5.2?"
> 
> That itself is a strong sign to me that there's a need or usecase or a
> good idea. Though we have a lot of them, this one was simple to
> implement and made sense to me. For the raid56 it's a simple check,
> unlike for other features that would need to go through significant
> portion of metadata.
> 
> Booting older kernel might sound like, why would anybody want to do
> that, but if the bit is there preventing boot/mount, then it's an
> unnecessary complication. In rescue environmnents it's gold.
> 
> Usability improvements tend to be boring from code POV but it is
> something that users can observe and make use of.
> 
Thanks for the full picture.

That makes sense.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]