| Message ID | cover.1714707707.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: extent-map: unify the members with btrfs_ordered_extent | expand |
On Fri, May 03, 2024 at 03:31:35PM +0930, Qu Wenruo wrote: > [CHANGELOG] > 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. Regarding the merge target, it's too close to 6.10 merge window freeze so this series is scheduled for 6.11.
On Fri, May 3, 2024 at 7:02 AM Qu Wenruo <wqu@suse.com> wrote: > > [CHANGELOG] > 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. The shrinker doesn't invalidate or make this patchset less useful. It's always good to reduce the size of a structure like this for which we can easily have millions of instances, it reduces the number of pages we consume. Things are a bit hard to review here, because a lot of code is added and then removed later and fields at a time, so a lot of cross reference checks are needed. Changing the approach here would be a lot of work, and probably would be more bike shedding than anything else. But it looks fine, and all the comments on the individual patches are minor, except for a bug in patch 8/11. Thanks! > > 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 | 187 ++++++++++++------ > 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 | 36 +++- > fs/btrfs/ordered-data.h | 22 ++- > 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 | 25 +-- > fs/btrfs/zoned.c | 4 +- > include/trace/events/btrfs.h | 26 +-- > 17 files changed, 548 insertions(+), 483 deletions(-) > > -- > 2.45.0 > >
[CHANGELOG] 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 | 187 ++++++++++++------ 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 | 36 +++- fs/btrfs/ordered-data.h | 22 ++- 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 | 25 +-- fs/btrfs/zoned.c | 4 +- include/trace/events/btrfs.h | 26 +-- 17 files changed, 548 insertions(+), 483 deletions(-)