Wang Shilong
2013-Mar-29 13:42 UTC
[PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Arne Jansen
2013-Mar-30 10:08 UTC
Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
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); >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Mar-30 11:47 UTC
Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
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); >> >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Mar-30 11:55 UTC
Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
<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); >> >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arne Jansen
2013-Mar-30 13:44 UTC
Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
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); >>> >> >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Mar-31 10:40 UTC
Re: [PATCH] Btrfs: cleanup to remove reduplicate code in iterate_extent_inode()
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); >>>> >>> >> >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html