Re: [PATCH v4 2/4] btrfs: extent-tree: Kill BUG_ON() in __btrfs_free_extent() and do better comment

[David Sterba <dsterba@suse.cz>]

On Fri, Aug 14, 2020 at 08:52:19AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/8/13 下午10:10, David Sterba wrote:
> > On Wed, Aug 12, 2020 at 02:05:07PM +0800, Qu Wenruo wrote:
> >> @@ -2987,13 +3049,20 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> >>  				found_extent = 1;
> >>  				break;
> >>  			}
> >> +
> >> +			/* Quick path didn't find the EXTEMT/METADATA_ITEM */
> >>  			if (path->slots[0] - extent_slot > 5)
> >>  				break;
> >>  			extent_slot--;
> >>  		}
> >>
> >>  		if (!found_extent) {
> >> -			BUG_ON(iref);
> >> +			if (unlikely(iref)) {
> >> +				btrfs_crit(info,
> >> +"invalid iref, no EXTENT/METADATA_ITEM found but has inline extent ref");
> >> +				goto err_dump_abort;
> >
> > This is not calling transaction abort at the place where it happens,
> > here and several other places too.
> 
> Did you mean, we want the btrfs_abort_transaction() call not merged
> under one tag, so that we can get the kernel warning with the line number?
> 
> If so, that's indeed the case, we lose the exact line number.
> 
> But we still have the unique error message to locate the problem without
> much hassle (it's less obvious than the line number thought).
> 
> Thus to me, we don't lose much debug info anyway.

https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort

"Please keep all transaction abort exactly at the place where they happen
and do not merge them to one. This pattern should be used everwhere and
is important when debugging because we can pinpoint the line in the code
from the syslog message and do not have to guess which way it got to the
merged call."

It's very convenient to paste the file:line number from a stacktrace
report and land exactly in the spot where it failed.