| Message ID | 52ebe8f2afb1460ef9b5abc814c432b4f4bd0dd4.1713223082.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: fix btrfs_file_extent_item::ram_bytes of btrfs_split_ordered_extent() | expand |
On Tue, Apr 16, 2024 at 12:24 AM Qu Wenruo <wqu@suse.com> wrote: > > During my development on extent map cleanups, I hit a case where we can > create a file extent item that has ram_bytes double the size of > num_bytes but it's not compressed. > > Later it turns out to be a bug in btrfs_split_ordered_extent(), and > thankfully it doesn't cause any real corruption, just a drift from > on-disk format. > > Here we add an extra check on ram_bytes for btrfs_file_extent_item to > catch such problem. > > However considering the incorrect ram_bytes are already in the wild, and > no real data corruption, we do not want end users to be bothered as their > data is still consistent. > > So this patch would only hide the check behind DEBUG builds for us > developers to catch future problem. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 35 +++++++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index c8fbcae4e88e..8dfbec3e6ba2 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -212,6 +212,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, > u32 sectorsize = fs_info->sectorsize; > u32 item_size = btrfs_item_size(leaf, slot); > u64 extent_end; > + u8 compression; > > if (unlikely(!IS_ALIGNED(key->offset, sectorsize))) { > file_extent_err(leaf, slot, > @@ -251,16 +252,15 @@ static int check_extent_data_item(struct extent_buffer *leaf, > return -EUCLEAN; > } > > + compression = btrfs_file_extent_compression(leaf, fi); > /* > * Support for new compression/encryption must introduce incompat flag, > * and must be caught in open_ctree(). > */ > - if (unlikely(btrfs_file_extent_compression(leaf, fi) >= > - BTRFS_NR_COMPRESS_TYPES)) { > + if (unlikely(compression >= BTRFS_NR_COMPRESS_TYPES)) { > file_extent_err(leaf, slot, > "invalid compression for file extent, have %u expect range [0, %u]", > - btrfs_file_extent_compression(leaf, fi), > - BTRFS_NR_COMPRESS_TYPES - 1); > + compression, BTRFS_NR_COMPRESS_TYPES - 1); > return -EUCLEAN; > } > if (unlikely(btrfs_file_extent_encryption(leaf, fi))) { > @@ -279,8 +279,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, > } > > /* Compressed inline extent has no on-disk size, skip it */ > - if (btrfs_file_extent_compression(leaf, fi) != > - BTRFS_COMPRESS_NONE) > + if (compression != BTRFS_COMPRESS_NONE) > return 0; > > /* Uncompressed inline extent size must match item size */ > @@ -319,6 +318,30 @@ static int check_extent_data_item(struct extent_buffer *leaf, > return -EUCLEAN; > } > > + /* > + * If it's a uncompressed regular extents, its ram size should match > + * disk_num_bytes. But for now we have several call sites that doesn't > + * properly update @ram_bytes, so at least make sure > + * @ram_bytes <= @disk_num_bytes. > + * > + * However we have a recent bug related to @ram_bytes update, causing Reading this in one year or more will be confusing. Why not just say: "However we had a bug related to ..." > + * all zoned btrfs and regular btrfs DIO to be affected. > + * Thankfully the ram_bytes is not critical for non-compressed file extents. > + * So here we hide the check behind DEBUG builds for developers only. > + */ > +#ifdef CONFIG_BTRFS_DEBUG > + if (unlikely(compression == BTRFS_COMPRESS_NONE && > + btrfs_file_extent_disk_bytenr(leaf, fi) && > + btrfs_file_extent_ram_bytes(leaf, fi) > > + btrfs_file_extent_disk_num_bytes(leaf, fi))) { > + file_extent_err(leaf, slot, > + "invalid ram_bytes, have %llu expect <=%llu", Please leave a space between <= and %llu, that makes it more readable, consistent with code and consistent with an error message at check_block_group_item(). With that change: Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > + btrfs_file_extent_ram_bytes(leaf, fi), > + btrfs_file_extent_disk_num_bytes(leaf, fi)); > + return -EUCLEAN; > + } > +#endif > + > /* > * Check that no two consecutive file extent items, in the same leaf, > * present ranges that overlap each other. > -- > 2.44.0 > >
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index c8fbcae4e88e..8dfbec3e6ba2 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -212,6 +212,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, u32 sectorsize = fs_info->sectorsize; u32 item_size = btrfs_item_size(leaf, slot); u64 extent_end; + u8 compression; if (unlikely(!IS_ALIGNED(key->offset, sectorsize))) { file_extent_err(leaf, slot, @@ -251,16 +252,15 @@ static int check_extent_data_item(struct extent_buffer *leaf, return -EUCLEAN; } + compression = btrfs_file_extent_compression(leaf, fi); /* * Support for new compression/encryption must introduce incompat flag, * and must be caught in open_ctree(). */ - if (unlikely(btrfs_file_extent_compression(leaf, fi) >= - BTRFS_NR_COMPRESS_TYPES)) { + if (unlikely(compression >= BTRFS_NR_COMPRESS_TYPES)) { file_extent_err(leaf, slot, "invalid compression for file extent, have %u expect range [0, %u]", - btrfs_file_extent_compression(leaf, fi), - BTRFS_NR_COMPRESS_TYPES - 1); + compression, BTRFS_NR_COMPRESS_TYPES - 1); return -EUCLEAN; } if (unlikely(btrfs_file_extent_encryption(leaf, fi))) { @@ -279,8 +279,7 @@ static int check_extent_data_item(struct extent_buffer *leaf, } /* Compressed inline extent has no on-disk size, skip it */ - if (btrfs_file_extent_compression(leaf, fi) != - BTRFS_COMPRESS_NONE) + if (compression != BTRFS_COMPRESS_NONE) return 0; /* Uncompressed inline extent size must match item size */ @@ -319,6 +318,30 @@ static int check_extent_data_item(struct extent_buffer *leaf, return -EUCLEAN; } + /* + * If it's a uncompressed regular extents, its ram size should match + * disk_num_bytes. But for now we have several call sites that doesn't + * properly update @ram_bytes, so at least make sure + * @ram_bytes <= @disk_num_bytes. + * + * However we have a recent bug related to @ram_bytes update, causing + * all zoned btrfs and regular btrfs DIO to be affected. + * Thankfully the ram_bytes is not critical for non-compressed file extents. + * So here we hide the check behind DEBUG builds for developers only. + */ +#ifdef CONFIG_BTRFS_DEBUG + if (unlikely(compression == BTRFS_COMPRESS_NONE && + btrfs_file_extent_disk_bytenr(leaf, fi) && + btrfs_file_extent_ram_bytes(leaf, fi) > + btrfs_file_extent_disk_num_bytes(leaf, fi))) { + file_extent_err(leaf, slot, + "invalid ram_bytes, have %llu expect <=%llu", + btrfs_file_extent_ram_bytes(leaf, fi), + btrfs_file_extent_disk_num_bytes(leaf, fi)); + return -EUCLEAN; + } +#endif + /* * Check that no two consecutive file extent items, in the same leaf, * present ranges that overlap each other.
During my development on extent map cleanups, I hit a case where we can create a file extent item that has ram_bytes double the size of num_bytes but it's not compressed. Later it turns out to be a bug in btrfs_split_ordered_extent(), and thankfully it doesn't cause any real corruption, just a drift from on-disk format. Here we add an extra check on ram_bytes for btrfs_file_extent_item to catch such problem. However considering the incorrect ram_bytes are already in the wild, and no real data corruption, we do not want end users to be bothered as their data is still consistent. So this patch would only hide the check behind DEBUG builds for us developers to catch future problem. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)