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 <wqu@xxxxxxxx> > --- <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 '>' <snip>
