| Message ID | a963c3b347e54bc23b9c0b39e8e864ae309dd148.1719291793.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: detect and fix the ram_bytes mismatch | expand |
On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote: > > [PROBLEMS] I wouldn't call this "problems", there are no bugs here or anything harmful. > Before commit 85de2be7129c ("btrfs: remove extent_map::block_start > member"), we utilized @bytenr variable inside > btrfs_extent_item_to_extent_map() to calculate block_start. > > But that commit removed block_start completely, we have no need to > advance @bytenr at all. > > Furthermore with recent enhanced btrfs-progs check for ram_bytes > mimsatch, it turns out that for truncated ordered extents, their mimsatch -> mismatch > ram_bytes can be smaller than disk_num_bytes. > > [ENHANCEMENT] > Thankfully all above problems are not really going to affect end users, > fix them by: > > - Declare @bytenr only inside the if branch and make it const > So we can remove the unnecessary advance of @bytenr. > > - Manually override extent_map::ram_bytes using disk_num_bytes > This is for non-compressed regular/preallocated extents. I don't see anything in the patch changing ram_bytes. Perhaps this is from an early patch version never submitted, or from some other patch? The code itself looks good. Thanks. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/file-item.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c > index 55703c833f3d..2cc61c792ee6 100644 > --- a/fs/btrfs/file-item.c > +++ b/fs/btrfs/file-item.c > @@ -1281,7 +1281,6 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, > const int slot = path->slots[0]; > struct btrfs_key key; > u64 extent_start; > - u64 bytenr; > u8 type = btrfs_file_extent_type(leaf, fi); > int compress_type = btrfs_file_extent_compression(leaf, fi); > > @@ -1291,22 +1290,22 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, > em->generation = btrfs_file_extent_generation(leaf, fi); > if (type == BTRFS_FILE_EXTENT_REG || > type == BTRFS_FILE_EXTENT_PREALLOC) { > + const u64 disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); > + > em->start = extent_start; > em->len = btrfs_file_extent_end(path) - extent_start; > - bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); > - if (bytenr == 0) { > + if (disk_bytenr == 0) { > em->disk_bytenr = EXTENT_MAP_HOLE; > em->disk_num_bytes = 0; > em->offset = 0; > return; > } > - em->disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); > + em->disk_bytenr = disk_bytenr; > em->disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi); > em->offset = btrfs_file_extent_offset(leaf, fi); > if (compress_type != BTRFS_COMPRESS_NONE) { > extent_map_set_compression(em, compress_type); > } else { > - bytenr += btrfs_file_extent_offset(leaf, fi); > if (type == BTRFS_FILE_EXTENT_PREALLOC) > em->flags |= EXTENT_FLAG_PREALLOC; > } > -- > 2.45.2 > >
在 2024/6/25 19:49, Filipe Manana 写道: > On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote: >> >> [PROBLEMS] > > I wouldn't call this "problems", there are no bugs here or anything harmful. > >> Before commit 85de2be7129c ("btrfs: remove extent_map::block_start >> member"), we utilized @bytenr variable inside >> btrfs_extent_item_to_extent_map() to calculate block_start. >> >> But that commit removed block_start completely, we have no need to >> advance @bytenr at all. >> >> Furthermore with recent enhanced btrfs-progs check for ram_bytes >> mimsatch, it turns out that for truncated ordered extents, their > > mimsatch -> mismatch > >> ram_bytes can be smaller than disk_num_bytes. >> >> [ENHANCEMENT] >> Thankfully all above problems are not really going to affect end users, >> fix them by: >> >> - Declare @bytenr only inside the if branch and make it const >> So we can remove the unnecessary advance of @bytenr. >> >> - Manually override extent_map::ram_bytes using disk_num_bytes >> This is for non-compressed regular/preallocated extents. > > I don't see anything in the patch changing ram_bytes. > Perhaps this is from an early patch version never submitted, or from > some other patch? My bad, when re-editing the commit message, I got confused with later patches. I will remove the ram_bytes related part. Thanks, Qu > > The code itself looks good. > Thanks. > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/file-item.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c >> index 55703c833f3d..2cc61c792ee6 100644 >> --- a/fs/btrfs/file-item.c >> +++ b/fs/btrfs/file-item.c >> @@ -1281,7 +1281,6 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, >> const int slot = path->slots[0]; >> struct btrfs_key key; >> u64 extent_start; >> - u64 bytenr; >> u8 type = btrfs_file_extent_type(leaf, fi); >> int compress_type = btrfs_file_extent_compression(leaf, fi); >> >> @@ -1291,22 +1290,22 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, >> em->generation = btrfs_file_extent_generation(leaf, fi); >> if (type == BTRFS_FILE_EXTENT_REG || >> type == BTRFS_FILE_EXTENT_PREALLOC) { >> + const u64 disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); >> + >> em->start = extent_start; >> em->len = btrfs_file_extent_end(path) - extent_start; >> - bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); >> - if (bytenr == 0) { >> + if (disk_bytenr == 0) { >> em->disk_bytenr = EXTENT_MAP_HOLE; >> em->disk_num_bytes = 0; >> em->offset = 0; >> return; >> } >> - em->disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); >> + em->disk_bytenr = disk_bytenr; >> em->disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi); >> em->offset = btrfs_file_extent_offset(leaf, fi); >> if (compress_type != BTRFS_COMPRESS_NONE) { >> extent_map_set_compression(em, compress_type); >> } else { >> - bytenr += btrfs_file_extent_offset(leaf, fi); >> if (type == BTRFS_FILE_EXTENT_PREALLOC) >> em->flags |= EXTENT_FLAG_PREALLOC; >> } >> -- >> 2.45.2 >> >> >
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c index 55703c833f3d..2cc61c792ee6 100644 --- a/fs/btrfs/file-item.c +++ b/fs/btrfs/file-item.c @@ -1281,7 +1281,6 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, const int slot = path->slots[0]; struct btrfs_key key; u64 extent_start; - u64 bytenr; u8 type = btrfs_file_extent_type(leaf, fi); int compress_type = btrfs_file_extent_compression(leaf, fi); @@ -1291,22 +1290,22 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode, em->generation = btrfs_file_extent_generation(leaf, fi); if (type == BTRFS_FILE_EXTENT_REG || type == BTRFS_FILE_EXTENT_PREALLOC) { + const u64 disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); + em->start = extent_start; em->len = btrfs_file_extent_end(path) - extent_start; - bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); - if (bytenr == 0) { + if (disk_bytenr == 0) { em->disk_bytenr = EXTENT_MAP_HOLE; em->disk_num_bytes = 0; em->offset = 0; return; } - em->disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi); + em->disk_bytenr = disk_bytenr; em->disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi); em->offset = btrfs_file_extent_offset(leaf, fi); if (compress_type != BTRFS_COMPRESS_NONE) { extent_map_set_compression(em, compress_type); } else { - bytenr += btrfs_file_extent_offset(leaf, fi); if (type == BTRFS_FILE_EXTENT_PREALLOC) em->flags |= EXTENT_FLAG_PREALLOC; }
[PROBLEMS] Before commit 85de2be7129c ("btrfs: remove extent_map::block_start member"), we utilized @bytenr variable inside btrfs_extent_item_to_extent_map() to calculate block_start. But that commit removed block_start completely, we have no need to advance @bytenr at all. Furthermore with recent enhanced btrfs-progs check for ram_bytes mimsatch, it turns out that for truncated ordered extents, their ram_bytes can be smaller than disk_num_bytes. [ENHANCEMENT] Thankfully all above problems are not really going to affect end users, fix them by: - Declare @bytenr only inside the if branch and make it const So we can remove the unnecessary advance of @bytenr. - Manually override extent_map::ram_bytes using disk_num_bytes This is for non-compressed regular/preallocated extents. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/file-item.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)