* [PATCH] Btrfs: cleanup of error processing in btree_get_extent()
@ 2012-09-12 8:26 Tsutomu Itoh
2012-09-12 10:37 ` Stefan Behrens
0 siblings, 1 reply; 3+ messages in thread
From: Tsutomu Itoh @ 2012-09-12 8:26 UTC (permalink / raw)
To: linux-btrfs
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);
}
} 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;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: cleanup of error processing in btree_get_extent()
2012-09-12 8:26 [PATCH] Btrfs: cleanup of error processing in btree_get_extent() Tsutomu Itoh
@ 2012-09-12 10:37 ` Stefan Behrens
2012-09-13 1:08 ` Tsutomu Itoh
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Behrens @ 2012-09-12 10:37 UTC (permalink / raw)
To: Tsutomu Itoh; +Cc: linux-btrfs
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.
> }
> } 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;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: cleanup of error processing in btree_get_extent()
2012-09-12 10:37 ` Stefan Behrens
@ 2012-09-13 1:08 ` Tsutomu Itoh
0 siblings, 0 replies; 3+ messages in thread
From: Tsutomu Itoh @ 2012-09-13 1:08 UTC (permalink / raw)
To: Stefan Behrens; +Cc: linux-btrfs
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;
>> }
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-09-13 1:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12 8:26 [PATCH] Btrfs: cleanup of error processing in btree_get_extent() Tsutomu Itoh
2012-09-12 10:37 ` Stefan Behrens
2012-09-13 1:08 ` Tsutomu Itoh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).