| Message ID | 20200320184348.845248-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: fix removal of raid[56|1c34} incompat flags after removing block group | expand |
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.
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); > } > } >
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?
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.
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.
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
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); } }