| Message ID | fee2c7df3d0a2e91e9b5e07a04242fcf28ade6bf.1690178924.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: warn on tree blocks which are not nodesize aligned | expand |
On 24/7/23 14:09, Qu Wenruo wrote: > A long time ago, we have some metadata chunks which starts at sector > boundary but not aligned at nodesize boundary. > > This led to some older fs which can have tree blocks only aligned to > sectorsize, but not nodesize. > > Later btrfs check gained the ability to detect and warn about such tree > blocks, and kernel fixed the chunk allocation behavior, nowadays those > tree blocks should be pretty rare. > > But in the future, if we want to migrate metadata to folio, we can not > have such tree blocks, as filemap_add_folio() requires the page index to > be aligned with the folio number of pages. > (AKA, such unaligned tree blocks can lead to VM_BUG_ON().) > > So this patch adds extra warning for those unaligned tree blocks, as a > preparation for the future folio migration. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 8 ++++++++ > fs/btrfs/fs.h | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 65b01ec5bab1..0aa27a212d1e 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct btrfs_fs_info *fs_info, u64 start) > start, fs_info->nodesize); > return -EINVAL; > } > + if (!IS_ALIGNED(start, fs_info->nodesize) && > + !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK, > + &fs_info->flags)) { > + btrfs_warn(fs_info, > + "tree block not nodesize aligned, start %llu nodesize %u", > + start, fs_info->nodesize); > + btrfs_warn(fs_info, "this can be solved by a full metadata balance"); > + } A btrfs_warn_rl() will be a safer option IMO. Thanks. > return 0; > } > > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > index 203d2a267828..2de3961aee44 100644 > --- a/fs/btrfs/fs.h > +++ b/fs/btrfs/fs.h > @@ -141,6 +141,13 @@ enum { > */ > BTRFS_FS_FEATURE_CHANGED, > > + /* > + * Indicate if we have tree block which is only aligned to sectorsize, > + * but not to nodesize. > + * This should be rare nowadays. > + */ > + BTRFS_FS_UNALIGNED_TREE_BLOCK, > + > #if BITS_PER_LONG == 32 > /* Indicate if we have error/warn message printed on 32bit systems */ > BTRFS_FS_32BIT_ERROR,
On 2023/7/24 15:43, Anand Jain wrote: > On 24/7/23 14:09, Qu Wenruo wrote: >> A long time ago, we have some metadata chunks which starts at sector >> boundary but not aligned at nodesize boundary. >> >> This led to some older fs which can have tree blocks only aligned to >> sectorsize, but not nodesize. >> >> Later btrfs check gained the ability to detect and warn about such tree >> blocks, and kernel fixed the chunk allocation behavior, nowadays those >> tree blocks should be pretty rare. >> >> But in the future, if we want to migrate metadata to folio, we can not >> have such tree blocks, as filemap_add_folio() requires the page index to >> be aligned with the folio number of pages. >> (AKA, such unaligned tree blocks can lead to VM_BUG_ON().) >> >> So this patch adds extra warning for those unaligned tree blocks, as a >> preparation for the future folio migration. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 8 ++++++++ >> fs/btrfs/fs.h | 7 +++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 65b01ec5bab1..0aa27a212d1e 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct >> btrfs_fs_info *fs_info, u64 start) >> start, fs_info->nodesize); >> return -EINVAL; >> } >> + if (!IS_ALIGNED(start, fs_info->nodesize) && >> + !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK, >> + &fs_info->flags)) { >> + btrfs_warn(fs_info, >> + "tree block not nodesize aligned, start %llu nodesize %u", >> + start, fs_info->nodesize); >> + btrfs_warn(fs_info, "this can be solved by a full metadata >> balance"); >> + } > > A btrfs_warn_rl() will be a safer option IMO. Not really, as this warning will only be output once, as we are doing test_and_set_bit(). Thus I really want to all messages to be shown, including the solution to fix it. Thanks, Qu > > Thanks. > >> return 0; >> } >> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h >> index 203d2a267828..2de3961aee44 100644 >> --- a/fs/btrfs/fs.h >> +++ b/fs/btrfs/fs.h >> @@ -141,6 +141,13 @@ enum { >> */ >> BTRFS_FS_FEATURE_CHANGED, >> + /* >> + * Indicate if we have tree block which is only aligned to >> sectorsize, >> + * but not to nodesize. >> + * This should be rare nowadays. >> + */ >> + BTRFS_FS_UNALIGNED_TREE_BLOCK, >> + >> #if BITS_PER_LONG == 32 >> /* Indicate if we have error/warn message printed on 32bit >> systems */ >> BTRFS_FS_32BIT_ERROR, >
A _warn level printk seems pretty excessive for something that was perfectly legal and created by earlier implementations.
On 24/7/23 16:55, Qu Wenruo wrote: > > > On 2023/7/24 15:43, Anand Jain wrote: >> On 24/7/23 14:09, Qu Wenruo wrote: >>> A long time ago, we have some metadata chunks which starts at sector >>> boundary but not aligned at nodesize boundary. >>> >>> This led to some older fs which can have tree blocks only aligned to >>> sectorsize, but not nodesize. >>> >>> Later btrfs check gained the ability to detect and warn about such tree >>> blocks, and kernel fixed the chunk allocation behavior, nowadays those >>> tree blocks should be pretty rare. >>> >>> But in the future, if we want to migrate metadata to folio, we can not >>> have such tree blocks, as filemap_add_folio() requires the page index to >>> be aligned with the folio number of pages. >>> (AKA, such unaligned tree blocks can lead to VM_BUG_ON().) >>> >>> So this patch adds extra warning for those unaligned tree blocks, as a >>> preparation for the future folio migration. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/extent_io.c | 8 ++++++++ >>> fs/btrfs/fs.h | 7 +++++++ >>> 2 files changed, 15 insertions(+) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index 65b01ec5bab1..0aa27a212d1e 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct >>> btrfs_fs_info *fs_info, u64 start) >>> start, fs_info->nodesize); >>> return -EINVAL; >>> } >>> + if (!IS_ALIGNED(start, fs_info->nodesize) && >>> + !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK, >>> + &fs_info->flags)) { >>> + btrfs_warn(fs_info, >>> + "tree block not nodesize aligned, start %llu nodesize %u", >>> + start, fs_info->nodesize); >>> + btrfs_warn(fs_info, "this can be solved by a full metadata >>> balance"); >>> + } >> >> A btrfs_warn_rl() will be a safer option IMO. > > Not really, as this warning will only be output once, as we are doing > test_and_set_bit(). Oh. Right. Reviewed-by: Anand Jain <anand.jain@oracle.com>
On 2023/7/24 22:12, Christoph Hellwig wrote: > A _warn level printk seems pretty excessive for something that was > perfectly legal and created by earlier implementations. > At least we have a valid workaround, and this partial aligned tree block is already checked and warned by btrfs-check for a long long time. Thanks, Qu
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 65b01ec5bab1..0aa27a212d1e 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct btrfs_fs_info *fs_info, u64 start) start, fs_info->nodesize); return -EINVAL; } + if (!IS_ALIGNED(start, fs_info->nodesize) && + !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK, + &fs_info->flags)) { + btrfs_warn(fs_info, + "tree block not nodesize aligned, start %llu nodesize %u", + start, fs_info->nodesize); + btrfs_warn(fs_info, "this can be solved by a full metadata balance"); + } return 0; } diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h index 203d2a267828..2de3961aee44 100644 --- a/fs/btrfs/fs.h +++ b/fs/btrfs/fs.h @@ -141,6 +141,13 @@ enum { */ BTRFS_FS_FEATURE_CHANGED, + /* + * Indicate if we have tree block which is only aligned to sectorsize, + * but not to nodesize. + * This should be rare nowadays. + */ + BTRFS_FS_UNALIGNED_TREE_BLOCK, + #if BITS_PER_LONG == 32 /* Indicate if we have error/warn message printed on 32bit systems */ BTRFS_FS_32BIT_ERROR,
A long time ago, we have some metadata chunks which starts at sector boundary but not aligned at nodesize boundary. This led to some older fs which can have tree blocks only aligned to sectorsize, but not nodesize. Later btrfs check gained the ability to detect and warn about such tree blocks, and kernel fixed the chunk allocation behavior, nowadays those tree blocks should be pretty rare. But in the future, if we want to migrate metadata to folio, we can not have such tree blocks, as filemap_add_folio() requires the page index to be aligned with the folio number of pages. (AKA, such unaligned tree blocks can lead to VM_BUG_ON().) So this patch adds extra warning for those unaligned tree blocks, as a preparation for the future folio migration. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 8 ++++++++ fs/btrfs/fs.h | 7 +++++++ 2 files changed, 15 insertions(+)