linux-kernel - Re: [PATCH] btrfs: turn unpin_extent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87lefbvtc9.fsf@suse.de>
Date:   Thu, 20 Jul 2023 10:13:26 +0100
From:   Luís Henriques <lhenriques@...e.de>
To:     Johannes Thumshirn <Johannes.Thumshirn@....com>
Cc:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs: turn unpin_extent_cache() into a void function

Johannes Thumshirn <Johannes.Thumshirn@....com> 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@...e.de>
>
> 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