Re: [PATCH] Btrfs: cleanup of error processing in btree_get_extent()

[Tsutomu Itoh <t-itoh@jp.fujitsu.com>]

Hi, Stefan,

(2012/09/12 19:37), Stefan Behrens wrote:
> On Wed, 12 Sep 2012 17:26:43 +0900, Tsutomu Itoh wrote:
>> This patch simplifies a little complex error processing in
>> btree_get_extent().
>>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> ---
>>   fs/btrfs/disk-io.c |   14 +++++---------
>>   1 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 29c69e6..27d0ebe 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -222,21 +222,17 @@ static struct extent_map *btree_get_extent(struct inode *inode,
>>
>>   		free_extent_map(em);
>>   		em = lookup_extent_mapping(em_tree, start, len);
>> -		if (em) {
>> -			ret = 0;
>> -		} else {
>> -			em = lookup_extent_mapping(em_tree, failed_start,
>> -						   failed_len);
>> -			ret = -EIO;
>> +		if (!em) {
>> +			lookup_extent_mapping(em_tree, failed_start,
>> +					      failed_len);
>> +			em = ERR_PTR(-EIO);
>
> The patch itself looks good and doesn't change the behavior while it
> simplifies the code. But I think that it identifies an old error in the
> code. It is eye-catching the the return value of lookup_extent_mapping()
> is not used. Therefore the call can either be removed or it has
> side-effects which are required. lookup_extent_mapping() has the
> side-effect to increment the extent map's ref count that it returns. I
> cannot believe that this incremented ref count is correct when the
> extent map itself is not used. IMO, either the EIO and ignoring the
> returned extent map is wrong, or the incremented ref count is wrong.

Thank you for your review.
I think lookup_extent_mapping() using failed_start is not needed.
So, I will remake my patch later.

Thanks,
Tsutomu

>
>>   		}
>>   	} else if (ret) {
>>   		free_extent_map(em);
>> -		em = NULL;
>> +		em = ERR_PTR(ret);
>>   	}
>>   	write_unlock(&em_tree->lock);
>>
>> -	if (ret)
>> -		em = ERR_PTR(ret);
>>   out:
>>   	return em;
>>   }
>
>
>