Re: [PATCH] btrfs: turn unpin_extent_cache() into a void function

[Luís Henriques]

Johannes Thumshirn <Johannes.Thumshirn@xxxxxxx> writes:

> On 18.07.23 19:39, Luís Henriques wrote:
>> The value of the 'ret' variable is never changed in function
>> unpin_extent_cache().  And since the only caller of this function doesn't
>> check the return value, it can simply be turned into a void function.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@xxxxxxx>
>
> Hmm but inside unpin_extent_cache() there is this:
>
>
> 	/* [...] */
> 	em = lookup_extent_mapping(tree, start, len);
>
> 	WARN_ON(!em || em->start != start);
>
> 	if (!em)
> 		goto out;
> 	/* [...] */
>
> out:
> 	write_unlock(&tree->lock);
> 	return ret;
>
> }
>
> Wouldn't it be better to either actually handle the error, OR
> change the WARN_ON() into an ASSERT()?
>
> Given the fact, that if the lookup fails, we've passed wrong 
> parameters somehow, an ASSERT() would be a good way IMHO.
>
> Thoughts?

OK, I guess that using ASSERT() makes sense -- it's used in several other
places where lookup_extent_mapping() is called.

Returning an error to the caller can also be done but I wonder if the only
place where it is called actually cares about it.  That's in
btrfs_finish_one_ordered(), and it basically does:


	if (test_bit(BTRFS_ORDERED_PREALLOC))
		ret = btrfs_mark_extent_written();
	else
		ret = insert_ordered_extent_file_extent();

	unpin_extent_cache();

	if (ret < 0) {
		btrfs_abort_transaction();
		goto out;
	}

Even if unpin_extent_cache() would return an error, I'd say that it is
better to try to proceed anyway rather than abort if unpinning an extent
from cache fails.  But my opinion isn't very solid ;-)

Cheers,
-- 
Luís