Re: [PATCH v2] btrfs: replace BUG_ON() with error return in get_new_location()

[David Sterba]

On Mon, Apr 27, 2026 at 10:49:26AM +0930, Qu Wenruo wrote:
> > @@ -835,10 +836,16 @@ static int get_new_location(struct inode *reloc_inode, u64 *new_bytenr,
> >   	fi = btrfs_item_ptr(leaf, path->slots[0],
> >   			    struct btrfs_file_extent_item);
> >   
> > -	BUG_ON(btrfs_file_extent_offset(leaf, fi) ||
> > -	       btrfs_file_extent_compression(leaf, fi) ||
> > -	       btrfs_file_extent_encryption(leaf, fi) ||
> > -	       btrfs_file_extent_other_encoding(leaf, fi));
> > +	if (unlikely(btrfs_file_extent_offset(leaf, fi) ||
> > +		     btrfs_file_extent_compression(leaf, fi) ||
> > +		     btrfs_file_extent_encryption(leaf, fi) ||
> > +		     btrfs_file_extent_other_encoding(leaf, fi))) {
> > +		btrfs_print_leaf(leaf);
> > +		btrfs_err(fs_info,
> > +"unexpected non-zero fields in file extent item for data reloc inode %llu offset %llu (offset/compression/encryption/other_encoding must all be 0)",
> > +			  btrfs_ino(BTRFS_I(reloc_inode)), bytenr);
> > +		return -EUCLEAN;
> 
> This looks like exactly what we did in tree-checker.
> 
> And we have enough info to do this in tree-checker, the data reloc tree 
> has a fixed id and owner in extent buffer header.
> 
> What about moving this to tree-checker where we already have 
> check_extent_data_item().

I think it would be better in the tree-checker, as this verifies the
item as it's read and not after the relocatikon is started.

The BUG_ON can be turned to an assertion though it duplicates some of
the checks, but this is otherwise harmless and cheap.