On 2019/7/10 下午6:48, Nikolay Borisov wrote: > > > On 10.07.19 г. 11:02 ч., Qu Wenruo wrote: >> __btrfs_free_extent() is one of the best cases to show how optimization >> could make a function hard to read. >> >> In fact __btrfs_free_extent() is only doing two major works: >> 1. Reduce the refs number of an extent backref >> Either it's an inlined extent backref (inside EXTENT/METADATA item) or >> a keyed extent backref (SHARED_* item). >> We only need to locate that backref line, either reduce the number or >> remove the backref line completely. >> >> 2. Update the refs count in EXTENT/METADATA_ITEM >> >> But in real world, we do it in a complex but somewhat efficient way. >> During step 1), we will try to locate the EXTENT/METADATA_ITEM without >> triggering another btrfs_search_slot() as fast path. >> >> Only when we failed to locate that item, we will trigger another >> btrfs_search_slot() to get that EXTENT/METADATA_ITEM after we >> updated/deleted the backref line. >> >> And we have a lot of restrict check on things like refs_to_drop against >> extent refs and special case check for single ref extent. >> >> All of these results: >> - 7 BUG_ON()s in a single function >> Although all these BUG_ON() are doing correct check, they're super >> easy to get triggered for fuzzed images. >> It's never a good idea to piss the end user. >> >> - Near 300 lines without much useful comments but a lot of hidden >> conditions >> I believe even the author needs several minutes to recall what the >> code is doing >> Not to mention a lot of BUG_ON() conditions needs to go back tens of >> lines to find out why. >> >> This patch address all these problems by: >> - Introduce two examples to show what __btrfs_free_extent() is doing >> One inlined backref case and one keyed case. >> Should cover most cases. >> >> - Kill all BUG_ON()s with proper error message and optional leaf dump >> >> - Add comment to show the overall workflow >> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202819 >> [ The report triggers one BUG_ON() in __btrfs_free_extent() ] >> Signed-off-by: Qu Wenruo <w...@suse.com> >> --- > > <snip> > >> @@ -6997,19 +7068,24 @@ static int __btrfs_free_extent(struct >> btrfs_trans_handle *trans, >> if (owner_objectid < BTRFS_FIRST_FREE_OBJECTID && >> key.type == BTRFS_EXTENT_ITEM_KEY) { >> struct btrfs_tree_block_info *bi; >> - BUG_ON(item_size < sizeof(*ei) + sizeof(*bi)); >> + if (unlikely(item_size < sizeof(*ei) + sizeof(*bi))) { >> + btrfs_crit(info, >> +"invalid extent item size for key (%llu, %u, %llu) owner %llu, has %u >> expect >%lu", > nit: stray '>'
With values filled, it would be: "invalid extent item size for key (1048575, 168, 4096) owner 7, has 10 expect >33" Now you'd see it's not a stray '>'. Thanks, Qu > > <snip> >
