[PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-29 Thread Wang Shilong 
From: Wang Shilong 

Just remove the unnecessary check and assignment.

Signed-off-by: Wang Shilong 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-30 Thread Arne Jansen 
On 03/29/13 14:42, Wang Shilong wrote:
> From: Wang Shilong 
> 
> Just remove the unnecessary check and assignment.
> 
> Signed-off-by: Wang Shilong 
> ---
>  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);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-30 Thread Wang Shilong 

Hello,

> On 03/29/13 14:42, Wang Shilong wrote:
>> From: Wang Shilong 
>> 
>> Just remove the unnecessary check and assignment.
>> 
>> Signed-off-by: Wang Shilong 
>> ---
>> 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);
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-30 Thread Wang Shilong 


> On 03/29/13 14:42, Wang Shilong wrote:
>> From: Wang Shilong 
>> 
>> Just remove the unnecessary check and assignment.
>> 
>> Signed-off-by: Wang Shilong 
>> ---
>> 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);
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-30 Thread Arne Jansen 
On 03/30/13 12:55, Wang Shilong wrote:
> 
> 
>> On 03/29/13 14:42, Wang Shilong wrote:
>>> From: Wang Shilong 
>>>
>>> Just remove the unnecessary check and assignment.
>>>
>>> Signed-off-by: Wang Shilong 
>>> ---
>>> 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);
>>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()

2013-03-31 Thread Wang 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:
>> 
>> 
>>> On 03/29/13 14:42, Wang Shilong wrote:
 From: Wang Shilong 
 
 Just remove the unnecessary check and assignment.
 
 Signed-off-by: Wang Shilong 
 ---
 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);
 
>>> 
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html