* [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
@ 2013-03-29 13:42 Wang Shilong
2013-03-30 10:08 ` Arne Jansen
0 siblings, 1 reply; 6+ messages in thread
From: Wang Shilong @ 2013-03-29 13:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: list.btrfs, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Just remove the unnecessary check and assignment.
Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
fs/btrfs/backref.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 3ca413bb..e102b48 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
if (ret)
break;
ULIST_ITER_INIT(&root_uiter);
- while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
+ while ((root_node = ulist_next(roots, &root_uiter))) {
pr_debug("root %llu references leaf %llu, data list "
"%#llx\n", root_node->val, ref_node->val,
(long long)ref_node->aux);
@@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
iterate, ctx);
}
ulist_free(roots);
- roots = NULL;
}
free_leaf_list(refs);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
2013-03-29 13:42 [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode() Wang Shilong
@ 2013-03-30 10:08 ` Arne Jansen
2013-03-30 11:47 ` Wang Shilong
2013-03-30 11:55 ` Wang Shilong
0 siblings, 2 replies; 6+ messages in thread
From: Arne Jansen @ 2013-03-30 10:08 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, list.btrfs, wangsl-fnst
On 03/29/13 14:42, Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>
> Just remove the unnecessary check and assignment.
>
> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> ---
> fs/btrfs/backref.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 3ca413bb..e102b48 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
> if (ret)
> break;
> ULIST_ITER_INIT(&root_uiter);
> - while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
> + while ((root_node = ulist_next(roots, &root_uiter))) {
It doesn't look unnecessary at all to me. ret is set in the loop and
only checked in the while condition.
> pr_debug("root %llu references leaf %llu, data list "
> "%#llx\n", root_node->val, ref_node->val,
> (long long)ref_node->aux);
> @@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
> iterate, ctx);
> }
> ulist_free(roots);
> - roots = NULL;
roots gets freed again later on. If you don't set it to NULL, it will
result in a double free.
-Arne
> }
>
> free_leaf_list(refs);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
2013-03-30 10:08 ` Arne Jansen
@ 2013-03-30 11:47 ` Wang Shilong
2013-03-30 11:55 ` Wang Shilong
1 sibling, 0 replies; 6+ messages in thread
From: Wang Shilong @ 2013-03-30 11:47 UTC (permalink / raw)
To: Arne Jansen; +Cc: linux-btrfs, list.btrfs, wangsl-fnst
Hello,
> On 03/29/13 14:42, Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> Just remove the unnecessary check and assignment.
>>
>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> ---
>> fs/btrfs/backref.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index 3ca413bb..e102b48 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>> if (ret)
>> break;
>> ULIST_ITER_INIT(&root_uiter);
>> - while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
>> + while ((root_node = ulist_next(roots, &root_uiter))) {
>
> It doesn't look unnecessary at all to me. ret is set in the loop and
> only checked in the while condition.
Yeah, you are right..
>
>> pr_debug("root %llu references leaf %llu, data list "
>> "%#llx\n", root_node->val, ref_node->val,
>> (long long)ref_node->aux);
>> @@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>> iterate, ctx);
>> }
>> ulist_free(roots);
>> - roots = NULL;
>
> roots gets freed again later on. If you don't set it to NULL, it will
> result in a double free.
If we are in the loop, 'roots' will be reallocated again, if relocation in the find_all_roots()
fails, 'roots' has been dealt in the find_all_roots(), and we have breaked out the loop.
I don't know where a double may happen? Am i missing something?
Thanks,
Wang
>
> -Arne
>
>> }
>>
>> free_leaf_list(refs);
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
2013-03-30 10:08 ` Arne Jansen
2013-03-30 11:47 ` Wang Shilong
@ 2013-03-30 11:55 ` Wang Shilong
2013-03-30 13:44 ` Arne Jansen
1 sibling, 1 reply; 6+ messages in thread
From: Wang Shilong @ 2013-03-30 11:55 UTC (permalink / raw)
To: Arne Jansen; +Cc: linux-btrfs, list.btrfs, wangsl-fnst
<snip>
> On 03/29/13 14:42, Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> Just remove the unnecessary check and assignment.
>>
>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> ---
>> fs/btrfs/backref.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index 3ca413bb..e102b48 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>> if (ret)
>> break;
>> ULIST_ITER_INIT(&root_uiter);
>> - while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
>> + while ((root_node = ulist_next(roots, &root_uiter))) {
>
> It doesn't look unnecessary at all to me. ret is set in the loop and
> only checked in the while condition.
>
>> pr_debug("root %llu references leaf %llu, data list "
>> "%#llx\n", root_node->val, ref_node->val,
>> (long long)ref_node->aux);
>> @@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>> iterate, ctx);
>> }
>> ulist_free(roots);
>> - roots = NULL;
>
> roots gets freed again later on. If you don't set it to NULL, it will
> result in a double free.
Maybe you mean this?
http://marc.info/?l=linux-btrfs&m=136456233929528&w=2
ulist_free() here is unnecessary and may cause a double free…
So we don't need to set it to NULL again..
>
> -Arne
>
>> }
>>
>> free_leaf_list(refs);
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
2013-03-30 11:55 ` Wang Shilong
@ 2013-03-30 13:44 ` Arne Jansen
2013-03-31 10:40 ` Wang Shilong
0 siblings, 1 reply; 6+ messages in thread
From: Arne Jansen @ 2013-03-30 13:44 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, list.btrfs, wangsl-fnst
On 03/30/13 12:55, Wang Shilong wrote:
> <snip>
>
>> On 03/29/13 14:42, Wang Shilong wrote:
>>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>>
>>> Just remove the unnecessary check and assignment.
>>>
>>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>> ---
>>> fs/btrfs/backref.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>> index 3ca413bb..e102b48 100644
>>> --- a/fs/btrfs/backref.c
>>> +++ b/fs/btrfs/backref.c
>>> @@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>>> if (ret)
>>> break;
>>> ULIST_ITER_INIT(&root_uiter);
>>> - while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
>>> + while ((root_node = ulist_next(roots, &root_uiter))) {
>>
>> It doesn't look unnecessary at all to me. ret is set in the loop and
>> only checked in the while condition.
>>
>>> pr_debug("root %llu references leaf %llu, data list "
>>> "%#llx\n", root_node->val, ref_node->val,
>>> (long long)ref_node->aux);
>>> @@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>>> iterate, ctx);
>>> }
>>> ulist_free(roots);
>>> - roots = NULL;
>>
>> roots gets freed again later on. If you don't set it to NULL, it will
>> result in a double free.
>
> Maybe you mean this?
>
> http://marc.info/?l=linux-btrfs&m=136456233929528&w=2
> ulist_free() here is unnecessary and may cause a double free…
> So we don't need to set it to NULL again..
Yeah, I haven't seen your other patch.
>
>
>
>>
>> -Arne
>>
>>> }
>>>
>>> free_leaf_list(refs);
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
2013-03-30 13:44 ` Arne Jansen
@ 2013-03-31 10:40 ` Wang Shilong
0 siblings, 0 replies; 6+ messages in thread
From: Wang Shilong @ 2013-03-31 10:40 UTC (permalink / raw)
To: linux-btrfs@vger.kernel.org
Cc: Arne Jansen, list.btrfs@jan-o-sch.net Schmidt,
wangsl-fnst@cn.fujitsu.com Shilong
Just ignore this patch, i have merge the correct modification
of this patch to the [patch V2] fix double free in the iterate_extent_inodes().
Thanks,
Wang
> On 03/30/13 12:55, Wang Shilong wrote:
>> <snip>
>>
>>> On 03/29/13 14:42, Wang Shilong wrote:
>>>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>>>
>>>> Just remove the unnecessary check and assignment.
>>>>
>>>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>>> ---
>>>> fs/btrfs/backref.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>>> index 3ca413bb..e102b48 100644
>>>> --- a/fs/btrfs/backref.c
>>>> +++ b/fs/btrfs/backref.c
>>>> @@ -1499,7 +1499,7 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>>>> if (ret)
>>>> break;
>>>> ULIST_ITER_INIT(&root_uiter);
>>>> - while (!ret && (root_node = ulist_next(roots, &root_uiter))) {
>>>> + while ((root_node = ulist_next(roots, &root_uiter))) {
>>>
>>> It doesn't look unnecessary at all to me. ret is set in the loop and
>>> only checked in the while condition.
>>>
>>>> pr_debug("root %llu references leaf %llu, data list "
>>>> "%#llx\n", root_node->val, ref_node->val,
>>>> (long long)ref_node->aux);
>>>> @@ -1510,7 +1510,6 @@ int iterate_extent_inodes(struct btrfs_fs_info *fs_info,
>>>> iterate, ctx);
>>>> }
>>>> ulist_free(roots);
>>>> - roots = NULL;
>>>
>>> roots gets freed again later on. If you don't set it to NULL, it will
>>> result in a double free.
>>
>> Maybe you mean this?
>>
>> http://marc.info/?l=linux-btrfs&m=136456233929528&w=2
>> ulist_free() here is unnecessary and may cause a double free…
>> So we don't need to set it to NULL again..
>
> Yeah, I haven't seen your other patch.
>
>>
>>
>>
>>>
>>> -Arne
>>>
>>>> }
>>>>
>>>> free_leaf_list(refs);
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-31 10:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-29 13:42 [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode() Wang Shilong
2013-03-30 10:08 ` Arne Jansen
2013-03-30 11:47 ` Wang Shilong
2013-03-30 11:55 ` Wang Shilong
2013-03-30 13:44 ` Arne Jansen
2013-03-31 10:40 ` Wang Shilong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox