Re: [PATCH] btrfs: Don't panic when we can't find a root key

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: dsterba@suse.cz, Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: Don't panic when we can't find a root key
Date: Mon, 25 Feb 2019 20:47:29 +0800	[thread overview]
Message-ID: <de131fa7-f87e-7be9-39de-bc0475c86c7c@gmx.com> (raw)
In-Reply-To: <20190225122611.GO9874@twin.jikos.cz>

[-- Attachment #1.1: Type: text/plain, Size: 1866 bytes --]

On 2019/2/25 下午8:26, David Sterba wrote:
> On Mon, Feb 25, 2019 at 01:11:06PM +0800, Qu Wenruo wrote:
>> When we failed to find a root key in btrfs_update_root(), we just panic.
>>
>> That's definitely not cool, fix it by aborting current transaction and
>> return an error value.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/root-tree.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
>> index 65bda0682928..c48a3e18866d 100644
>> --- a/fs/btrfs/root-tree.c
>> +++ b/fs/btrfs/root-tree.c
>> @@ -137,11 +137,15 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
>>  		goto out;
>>  	}
>>  
>> -	if (ret != 0) {
>> +	if (ret > 0) {
>>  		btrfs_print_leaf(path->nodes[0]);
> 
> You leave the leaf dump here, which I believe was related to the BUG_ON
> so there's information for later analysis. This is not usually done
> where transaction is aborted and error returned. I'm not saying it's not
> worth keeping it here, as the logic that would lead to this error is
> in the 'never happens' category. But we know such things happen due to
> the bitflips and random memory corruptions so a dump might make sense.
> 
> If you think this makes sense, please put it to the changelog and write
> a comment before the leaf dump. This will be a good pattern to follow as
> there are many instances of leaf_dump/BUG to clean up or review.

I think the dump makes sense, but didn't think so deep to make it as an
example for later cleanup.

If we're going to use this as an example for later cleanups.
I won't keep it.

The message itself should be enough to locate the cause, and the tree
dump doesn't help as much as we expected.

So I'm going to remove it in next update.

Thanks,
Qu

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]