| Message ID | cover.1716440169.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: extent-map: unify the members with btrfs_ordered_extent | expand |
Looks good to me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Thu, May 23, 2024 at 6:04 AM Qu Wenruo <wqu@suse.com> wrote: > > [CHANGELOG] > v3: > - Rebased to the latest for-next > There is a small conflicts with the extent map tree members changes, > no big deal. > > - Fix an error where original code is checking > btrfs_file_extent_disk_bytenr() > The newer code is checking disk_num_bytes, which is wrong. > > - Various commit messages/comments update > Mostly some grammar fixes and removal of rants on the btrfs_file_extent > member mismatches for btrfs_alloc_ordered_extent(). > However a comment is still left inside btrfs_alloc_ordered_extent() > for NOCOW/PREALLOC as a reminder for further cleanup. I went through each new patch version, and it looks good. I replied to some individual patches with minor things that can be fixed at commit time when adding to for-next in case you don't send a new version. For patch 7/11 there's one issue. Otherwise looks great, so after the 7/11 fix you can add: Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks! > > v2: > - Rebased to the latest for-next > There is a conflicts with extent locking, and maybe some other > hidden conflicts for NOCOW/PREALLOC? > As previously the patchset passes fstests auto group, but after > the merging with other patches, it always crashes as btrfs/060. > > - Fix an error in the final cleanup patch > It's the NOCOW/PREALLOC shenanigans again, in the buffered NOCOW path, > that we have to use the old inaccurate numbers for NOCOW/PREALLOC OEs. > > - Split the final cleanup into 4 patches > Most cleanups are very straightforward, but the cleanup for > btrfs_alloc_ordered_extent() needs extra special handling for > NOCOW/PREALLOC. > > v1: > - Rebased to the latest for-next > To resolve the conflicts with the recently introduced extent map > shrinker > > - A new cleanup patch to remove the recursive header inclusion > > - Use a new structure to pass the file extent item related members > around > > - Add a new comment on why we're intentionally passing incorrect > numbers for NOCOW/PREALLOC ordered extents inside > btrfs_create_dio_extent() > > [REPO] > https://github.com/adam900710/linux/tree/em_cleanup > > This series introduce two new members (disk_bytenr/offset) to > extent_map, and removes three old members > (block_start/block_len/offset), finally rename one member > (orig_block_len -> disk_num_bytes). > > This should save us one u64 for extent_map, although with the recent > extent map shrinker, the saving is not that useful. > > But to make things safe to migrate, I introduce extra sanity checks for > extent_map, and do cross check for both old and new members. > > The extra sanity checks already exposed one bug (thankfully harmless) > causing em::block_start to be incorrect. > > But so far, the patchset is fine for default fstests run. > > Furthermore, since we're already having too long parameter lists for > extent_map/ordered_extent/can_nocow_extent, here is a new structure, > btrfs_file_extent, a memory-access-friendly structure to represent a > btrfs_file_extent_item. > > With the help of that structure, we can use that to represent a file > extent item without a super long parameter list. > > The patchset would rename orig_block_len to disk_num_bytes first. > Then introduce the new member, the extra sanity checks, and introduce the > new btrfs_file_extent structure and use that to remove the older 3 members > from extent_map. > > After all above works done, use btrfs_file_extent to further cleanup > can_nocow_file_extent_args()/btrfs_alloc_ordered_extent()/create_io_em()/ > btrfs_create_dio_extent(). > > The cleanup is in fact pretty tricky, the current code base never > expects correct numbers for NOCOW/PREALLOC OEs, thus we have to keep the > old but incorrect numbers just for NOCOW/PREALLOC. > > I will address the NOCOW/PREALLOC shenanigans the future, but > after the huge cleanup across multiple core structures. > > Qu Wenruo (11): > btrfs: rename extent_map::orig_block_len to disk_num_bytes > btrfs: export the expected file extent through can_nocow_extent() > btrfs: introduce new members for extent_map > btrfs: introduce extra sanity checks for extent maps > btrfs: remove extent_map::orig_start member > btrfs: remove extent_map::block_len member > btrfs: remove extent_map::block_start member > btrfs: cleanup duplicated parameters related to > can_nocow_file_extent_args > btrfs: cleanup duplicated parameters related to > btrfs_alloc_ordered_extent > btrfs: cleanup duplicated parameters related to create_io_em() > btrfs: cleanup duplicated parameters related to > btrfs_create_dio_extent() > > fs/btrfs/btrfs_inode.h | 4 +- > fs/btrfs/compression.c | 7 +- > fs/btrfs/defrag.c | 14 +- > fs/btrfs/extent_io.c | 10 +- > fs/btrfs/extent_map.c | 192 +++++++++++++------ > fs/btrfs/extent_map.h | 51 +++-- > fs/btrfs/file-item.c | 23 +-- > fs/btrfs/file.c | 18 +- > fs/btrfs/inode.c | 308 +++++++++++++----------------- > fs/btrfs/ordered-data.c | 34 +++- > fs/btrfs/ordered-data.h | 19 +- > fs/btrfs/relocation.c | 5 +- > fs/btrfs/tests/extent-map-tests.c | 114 ++++++----- > fs/btrfs/tests/inode-tests.c | 177 ++++++++--------- > fs/btrfs/tree-log.c | 23 ++- > fs/btrfs/zoned.c | 4 +- > include/trace/events/btrfs.h | 18 +- > 17 files changed, 541 insertions(+), 480 deletions(-) > > -- > 2.45.1 > >
[CHANGELOG] v3: - Rebased to the latest for-next There is a small conflicts with the extent map tree members changes, no big deal. - Fix an error where original code is checking btrfs_file_extent_disk_bytenr() The newer code is checking disk_num_bytes, which is wrong. - Various commit messages/comments update Mostly some grammar fixes and removal of rants on the btrfs_file_extent member mismatches for btrfs_alloc_ordered_extent(). However a comment is still left inside btrfs_alloc_ordered_extent() for NOCOW/PREALLOC as a reminder for further cleanup. v2: - Rebased to the latest for-next There is a conflicts with extent locking, and maybe some other hidden conflicts for NOCOW/PREALLOC? As previously the patchset passes fstests auto group, but after the merging with other patches, it always crashes as btrfs/060. - Fix an error in the final cleanup patch It's the NOCOW/PREALLOC shenanigans again, in the buffered NOCOW path, that we have to use the old inaccurate numbers for NOCOW/PREALLOC OEs. - Split the final cleanup into 4 patches Most cleanups are very straightforward, but the cleanup for btrfs_alloc_ordered_extent() needs extra special handling for NOCOW/PREALLOC. v1: - Rebased to the latest for-next To resolve the conflicts with the recently introduced extent map shrinker - A new cleanup patch to remove the recursive header inclusion - Use a new structure to pass the file extent item related members around - Add a new comment on why we're intentionally passing incorrect numbers for NOCOW/PREALLOC ordered extents inside btrfs_create_dio_extent() [REPO] https://github.com/adam900710/linux/tree/em_cleanup This series introduce two new members (disk_bytenr/offset) to extent_map, and removes three old members (block_start/block_len/offset), finally rename one member (orig_block_len -> disk_num_bytes). This should save us one u64 for extent_map, although with the recent extent map shrinker, the saving is not that useful. But to make things safe to migrate, I introduce extra sanity checks for extent_map, and do cross check for both old and new members. The extra sanity checks already exposed one bug (thankfully harmless) causing em::block_start to be incorrect. But so far, the patchset is fine for default fstests run. Furthermore, since we're already having too long parameter lists for extent_map/ordered_extent/can_nocow_extent, here is a new structure, btrfs_file_extent, a memory-access-friendly structure to represent a btrfs_file_extent_item. With the help of that structure, we can use that to represent a file extent item without a super long parameter list. The patchset would rename orig_block_len to disk_num_bytes first. Then introduce the new member, the extra sanity checks, and introduce the new btrfs_file_extent structure and use that to remove the older 3 members from extent_map. After all above works done, use btrfs_file_extent to further cleanup can_nocow_file_extent_args()/btrfs_alloc_ordered_extent()/create_io_em()/ btrfs_create_dio_extent(). The cleanup is in fact pretty tricky, the current code base never expects correct numbers for NOCOW/PREALLOC OEs, thus we have to keep the old but incorrect numbers just for NOCOW/PREALLOC. I will address the NOCOW/PREALLOC shenanigans the future, but after the huge cleanup across multiple core structures. Qu Wenruo (11): btrfs: rename extent_map::orig_block_len to disk_num_bytes btrfs: export the expected file extent through can_nocow_extent() btrfs: introduce new members for extent_map btrfs: introduce extra sanity checks for extent maps btrfs: remove extent_map::orig_start member btrfs: remove extent_map::block_len member btrfs: remove extent_map::block_start member btrfs: cleanup duplicated parameters related to can_nocow_file_extent_args btrfs: cleanup duplicated parameters related to btrfs_alloc_ordered_extent btrfs: cleanup duplicated parameters related to create_io_em() btrfs: cleanup duplicated parameters related to btrfs_create_dio_extent() fs/btrfs/btrfs_inode.h | 4 +- fs/btrfs/compression.c | 7 +- fs/btrfs/defrag.c | 14 +- fs/btrfs/extent_io.c | 10 +- fs/btrfs/extent_map.c | 192 +++++++++++++------ fs/btrfs/extent_map.h | 51 +++-- fs/btrfs/file-item.c | 23 +-- fs/btrfs/file.c | 18 +- fs/btrfs/inode.c | 308 +++++++++++++----------------- fs/btrfs/ordered-data.c | 34 +++- fs/btrfs/ordered-data.h | 19 +- fs/btrfs/relocation.c | 5 +- fs/btrfs/tests/extent-map-tests.c | 114 ++++++----- fs/btrfs/tests/inode-tests.c | 177 ++++++++--------- fs/btrfs/tree-log.c | 23 ++- fs/btrfs/zoned.c | 4 +- include/trace/events/btrfs.h | 18 +- 17 files changed, 541 insertions(+), 480 deletions(-)