| Message ID | 20220623075547.1430106-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Properly flag filesystem with BTRFS_FEATURE_INCOMPAT_BIG_METADATA | expand |
On 2022/6/23 15:55, Nikolay Borisov wrote: > Commit 6f93e834fa7c seeimingly inadvertently moved the code responsible > for flagging the filesystem as having BIG_METADATA to a place where > setting the flag was essentially lost. Sorry, I didn't see the problem here. The existing check seems fine to me, mind to share why the existing call timing is bad? Thanks, Qu > This means that > filesystems created with kernels containing this bug (starting with 5.15) > can potentially be mounted by older (pre-3.10) kernels. In reality > chances for this happening are low because there are other incompat > flags introduced in the mean time. Still the correct behavior is to set > INCOMPAT_BIG_METADAT flag and persist this in the superblock. > > Fixes: 6f93e834fa7c ("btrfs: fix upper limit for max_inline for page size 64K") > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/disk-io.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 800ad3a9c68e..c3d92aadc820 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3464,16 +3464,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > */ > fs_info->compress_type = BTRFS_COMPRESS_ZLIB; > > - /* > - * Flag our filesystem as having big metadata blocks if they are bigger > - * than the page size. > - */ > - if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { > - if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) > - btrfs_info(fs_info, > - "flagging fs with big metadata feature"); > - features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; > - } > > /* Set up fs_info before parsing mount options */ > nodesize = btrfs_super_nodesize(disk_super); > @@ -3514,6 +3504,17 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) > btrfs_info(fs_info, "has skinny extents"); > > + /* > + * Flag our filesystem as having big metadata blocks if they are bigger > + * than the page size. > + */ > + if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { > + if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) > + btrfs_info(fs_info, > + "flagging fs with big metadata feature"); > + features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; > + } > + > /* > * mixed block groups end up with duplicate but slightly offset > * extent buffers for the same range. It leads to corruptions
On 23.06.22 г. 12:57 ч., Qu Wenruo wrote: > > > On 2022/6/23 15:55, Nikolay Borisov wrote: >> Commit 6f93e834fa7c seeimingly inadvertently moved the code responsible >> for flagging the filesystem as having BIG_METADATA to a place where >> setting the flag was essentially lost. > > Sorry, I didn't see the problem here. > > The existing check seems fine to me, mind to share why the existing call > timing is bad? > The problem is that when BTRFS_FEATURE_INCOMPAT_BIG_METADATA is set in features then we need to call btrfs_set_super_incompat_flags(disk_super, features); so that those flags are put into the superblock and subsequently persisted on-disk. However, with the code motion in the offending commit features would have BTRFS_FEATURE_INCOMPAT_BIG_METADATA set but then it would be rewritten by the call to: features = btrfs_super_incompat_flags(disk_super); meaning INCOMPAT_BIG_METADATA never went to disk. It's very subtle
On 2022/6/23 18:15, Nikolay Borisov wrote: > > > On 23.06.22 г. 12:57 ч., Qu Wenruo wrote: >> >> >> On 2022/6/23 15:55, Nikolay Borisov wrote: >>> Commit 6f93e834fa7c seeimingly inadvertently moved the code responsible >>> for flagging the filesystem as having BIG_METADATA to a place where >>> setting the flag was essentially lost. >> >> Sorry, I didn't see the problem here. >> >> The existing check seems fine to me, mind to share why the existing call >> timing is bad? >> > > > The problem is that when BTRFS_FEATURE_INCOMPAT_BIG_METADATA is set in > features then we need to call btrfs_set_super_incompat_flags(disk_super, > features); so that those flags are put into the superblock and > subsequently persisted on-disk. However, with the code motion in the > offending commit features would have BTRFS_FEATURE_INCOMPAT_BIG_METADATA > set but then it would be rewritten by the call to: > > features = btrfs_super_incompat_flags(disk_super); > > meaning INCOMPAT_BIG_METADATA never went to disk. It's very subtle Ah, got it. Indeed we later rewrite features. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu
On Thu, Jun 23, 2022 at 10:55:47AM +0300, Nikolay Borisov wrote: > Commit 6f93e834fa7c seeimingly inadvertently moved the code responsible > for flagging the filesystem as having BIG_METADATA to a place where > setting the flag was essentially lost. This means that > filesystems created with kernels containing this bug (starting with 5.15) > can potentially be mounted by older (pre-3.10) kernels. In reality > chances for this happening are low because there are other incompat > flags introduced in the mean time. Still the correct behavior is to set > INCOMPAT_BIG_METADAT flag and persist this in the superblock. > > Fixes: 6f93e834fa7c ("btrfs: fix upper limit for max_inline for page size 64K") > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 800ad3a9c68e..c3d92aadc820 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3464,16 +3464,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device */ fs_info->compress_type = BTRFS_COMPRESS_ZLIB; - /* - * Flag our filesystem as having big metadata blocks if they are bigger - * than the page size. - */ - if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { - if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) - btrfs_info(fs_info, - "flagging fs with big metadata feature"); - features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; - } /* Set up fs_info before parsing mount options */ nodesize = btrfs_super_nodesize(disk_super); @@ -3514,6 +3504,17 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) btrfs_info(fs_info, "has skinny extents"); + /* + * Flag our filesystem as having big metadata blocks if they are bigger + * than the page size. + */ + if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { + if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) + btrfs_info(fs_info, + "flagging fs with big metadata feature"); + features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; + } + /* * mixed block groups end up with duplicate but slightly offset * extent buffers for the same range. It leads to corruptions
Commit 6f93e834fa7c seeimingly inadvertently moved the code responsible for flagging the filesystem as having BIG_METADATA to a place where setting the flag was essentially lost. This means that filesystems created with kernels containing this bug (starting with 5.15) can potentially be mounted by older (pre-3.10) kernels. In reality chances for this happening are low because there are other incompat flags introduced in the mean time. Still the correct behavior is to set INCOMPAT_BIG_METADAT flag and persist this in the superblock. Fixes: 6f93e834fa7c ("btrfs: fix upper limit for max_inline for page size 64K") Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)