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 '>'
<snip>
