| Message ID | cover.1712614770.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: extent-map: use disk_bytenr/offset to replace block_start/block_len/orig_start | expand |
On Tue, Apr 09, 2024 at 08:03:39AM +0930, Qu Wenruo wrote: > [REASON FOR RFC] > Not all sanity checks are implemented, there is a missing check for > ram_bytes on non-compressed extent. > Because even without this series, generic/311 can generate a file extent > with ram_bytes larger than disk_num_bytes. > > This seems harmless, but I still want to fix it and implement a full > version of the em sanity check. > > [REPO] > https://github.com/adam900710/linux/tree/em_cleanup > > Which relies on previous changes on extent maps. > > 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. > > 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. > > There is another bug related to bad btrfs_file_extent_item::ram_bytes, > which can be larger than disk_num_bytes for non-compressed file extents. > (Generated by generic/311 test case, but it seems to be created on-disk > first) > > But so far, the patchset is fine for default fstests run. I've only paged through the patches, besides the renames there are so many changes where it's hard to spot a subtle bug, but overall it looks ok.
在 2024/4/10 00:27, David Sterba 写道: > On Tue, Apr 09, 2024 at 08:03:39AM +0930, Qu Wenruo wrote: >> [REASON FOR RFC] >> Not all sanity checks are implemented, there is a missing check for >> ram_bytes on non-compressed extent. >> Because even without this series, generic/311 can generate a file extent >> with ram_bytes larger than disk_num_bytes. >> >> This seems harmless, but I still want to fix it and implement a full >> version of the em sanity check. >> >> [REPO] >> https://github.com/adam900710/linux/tree/em_cleanup >> >> Which relies on previous changes on extent maps. >> >> 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. >> >> 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. >> >> There is another bug related to bad btrfs_file_extent_item::ram_bytes, >> which can be larger than disk_num_bytes for non-compressed file extents. >> (Generated by generic/311 test case, but it seems to be created on-disk >> first) >> >> But so far, the patchset is fine for default fstests run. > > I've only paged through the patches, besides the renames there are so > many changes where it's hard to spot a subtle bug, but overall it looks > ok. > That's why this time I go sanity/cross checks immediately after adding new members, to make sure the behavior is not changed. So that even it's pretty hard to review, if something obviously wrong happened, I'll hit a crash. You will be surprised by how many crashes it triggered during the development. Thanks, Qu
On Wed, Apr 10, 2024 at 07:10:08AM +0930, Qu Wenruo wrote: > 在 2024/4/10 00:27, David Sterba 写道: > >> > >> The extra sanity checks already exposed one bug (thankfully harmless) > >> causing em::block_start to be incorrect. > >> > >> There is another bug related to bad btrfs_file_extent_item::ram_bytes, > >> which can be larger than disk_num_bytes for non-compressed file extents. > >> (Generated by generic/311 test case, but it seems to be created on-disk > >> first) > >> > >> But so far, the patchset is fine for default fstests run. > > > > I've only paged through the patches, besides the renames there are so > > many changes where it's hard to spot a subtle bug, but overall it looks > > ok. > > That's why this time I go sanity/cross checks immediately after adding > new members, to make sure the behavior is not changed. > > So that even it's pretty hard to review, if something obviously wrong > happened, I'll hit a crash. > > You will be surprised by how many crashes it triggered during the > development. Yeah, I can imagine. If you make it work in the end then it's good. The self tests can verify the boundary conditions without having to create a complete filesystem and just focused on some API so here it's easier. The review I'd like to see is by somebody familiar with the extent maps if it all makes sense or there are no missing test cases. This series depends on another one that looks almost finished except some comments in patch that adds just code comments (so no functional changes). This kind of changes would be good to get enough testing in for-next so please go ahead and add it in case there's not something really serious outstanding. We can do some minor tweaks later.