[PATCH] xfs_repair: fix record_allocation list manipulation

[PATCH] xfs_repair: fix record_allocation list manipulation

[Eric Sandeen]

clang found this one too as a "Dead assignment"

Unless my pointer-fu is totally messed up, this function
was never actually updating the list head.

This would mean that the later free_allocations() calls in
incore_ext_teardown() and free_rt_dup_extent_tree() don't
actually free more than one item, and therefore leak memory.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

clang record:
http://sandeen.net/clang/xfsprogs/2009-09-09-1/report-1Jnl15.html#EndPath

diff --git a/repair/incore.c b/repair/incore.c
index 84626c9..77f4630 100644
--- a/repair/incore.c
+++ b/repair/incore.c
@@ -33,7 +33,7 @@ void
 record_allocation(ba_rec_t *addr, ba_rec_t *list)
 {
 	addr->next = list;
-	list = addr;
+	*list = addr;
 
 	return;
 }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs_repair: fix record_allocation list manipulation

[Eric Sandeen]

Eric Sandeen wrote:
> clang found this one too as a "Dead assignment"
> 
> Unless my pointer-fu is totally messed up, this function
> was never actually updating the list head.
> 
> This would mean that the later free_allocations() calls in
> incore_ext_teardown() and free_rt_dup_extent_tree() don't
> actually free more than one item, and therefore leak memory.
> 
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
> 
> clang record:
> http://sandeen.net/clang/xfsprogs/2009-09-09-1/report-1Jnl15.html#EndPath
> 
> diff --git a/repair/incore.c b/repair/incore.c
> index 84626c9..77f4630 100644
> --- a/repair/incore.c
> +++ b/repair/incore.c
> @@ -33,7 +33,7 @@ void
> record_allocation(ba_rec_t *addr, ba_rec_t *list)
> {
>     addr->next = list;
> -    list = addr;
> +    *list = addr;

argh that's not right, I swear I built it, oops.

Will send an update later.

> 
>     return;
> }
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] repair: replaced custom block allocation linked lists with list_heads

[Josef 'Jeff' Sipek]

The previous implementation of the linked lists was buggy, and leaked memory.

From: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>

Cc: sandeen@sandeen.net
Signed-off-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
---

Yes, Eric, it wastes an extra pointer per node, but at least it works
compared to the code it replaces :)

 repair/incore.c     |   27 ---------------------------
 repair/incore.h     |   11 -----------
 repair/incore_ext.c |   27 ++++++++++++++++-----------
 3 files changed, 16 insertions(+), 49 deletions(-)

diff --git a/repair/incore.c b/repair/incore.c
index 84626c9..27604e2 100644
--- a/repair/incore.c
+++ b/repair/incore.c
@@ -25,33 +25,6 @@
 #include "err_protos.h"
 #include "threads.h"
 
-/*
- * push a block allocation record onto list.  assumes list
- * if set to NULL if empty.
- */
-void
-record_allocation(ba_rec_t *addr, ba_rec_t *list)
-{
-	addr->next = list;
-	list = addr;
-
-	return;
-}
-
-void
-free_allocations(ba_rec_t *list)
-{
-	ba_rec_t *current = list;
-
-	while (list != NULL)  {
-		list = list->next;
-		free(current);
-		current = list;
-	}
-
-	return;
-}
-
 /* ba bmap setupstuff.  setting/getting state is in incore.h  */
 
 void
diff --git a/repair/incore.h b/repair/incore.h
index 1f0f45a..a22ef0f 100644
--- a/repair/incore.h
+++ b/repair/incore.h
@@ -26,17 +26,6 @@
  */
 
 /*
- * block allocation lists
- */
-typedef struct ba_rec  {
-	void		*addr;
-	struct ba_rec	*next;
-} ba_rec_t;
-
-void			record_allocation(ba_rec_t *addr, ba_rec_t *list);
-void			free_allocations(ba_rec_t *list);
-
-/*
  * block bit map defs -- track state of each filesystem block.
  * ba_bmap is an array of bitstrings declared in the globals.h file.
  * the bitstrings are broken up into 64-bit chunks.  one bitstring per AG.
diff --git a/repair/incore_ext.c b/repair/incore_ext.c
index a2acbf4..d0b8cdc 100644
--- a/repair/incore_ext.c
+++ b/repair/incore_ext.c
@@ -31,12 +31,12 @@
  * paranoia -- account for any weird padding, 64/32-bit alignment, etc.
  */
 typedef struct extent_alloc_rec  {
-	ba_rec_t		alloc_rec;
+	struct list_head	list;
 	extent_tree_node_t	extents[ALLOC_NUM_EXTS];
 } extent_alloc_rec_t;
 
 typedef struct rt_extent_alloc_rec  {
-	ba_rec_t		alloc_rec;
+	struct list_head	list;
 	rt_extent_tree_node_t	extents[ALLOC_NUM_EXTS];
 } rt_extent_alloc_rec_t;
 
@@ -89,8 +89,8 @@ static avltree_desc_t	**extent_bcnt_ptrs;	/*
 /*
  * list of allocated "blocks" for easy freeing later
  */
-static ba_rec_t		*ba_list;
-static ba_rec_t		*rt_ba_list;
+static struct list_head	ba_list;
+static struct list_head	rt_ba_list;
 
 /*
  * locks.
@@ -120,7 +120,7 @@ mk_extent_tree_nodes(xfs_agblock_t new_startblock,
 			do_error(
 			_("couldn't allocate new extent descriptors.\n"));
 
-		record_allocation(&rec->alloc_rec, ba_list);
+		list_add(&rec->list, &ba_list);
 
 		new = &rec->extents[0];
 
@@ -678,7 +678,7 @@ mk_rt_extent_tree_nodes(xfs_drtbno_t new_startblock,
 			do_error(
 			_("couldn't allocate new extent descriptors.\n"));
 
-		record_allocation(&rec->alloc_rec, rt_ba_list);
+		list_add(&rec->list, &rt_ba_list);
 
 		new = &rec->extents[0];
 
@@ -755,12 +755,15 @@ release_rt_extent_tree()
 void
 free_rt_dup_extent_tree(xfs_mount_t *mp)
 {
+	rt_extent_alloc_rec_t *cur, *tmp;
+
 	ASSERT(mp->m_sb.sb_rblocks != 0);
 
-	free_allocations(rt_ba_list);
+	list_for_each_entry_safe(cur, tmp, &rt_ba_list, list)
+		free(cur);
+
 	free(rt_ext_tree_ptr);
 
-	rt_ba_list = NULL;
 	rt_ext_tree_ptr = NULL;
 
 	return;
@@ -895,8 +898,8 @@ incore_ext_init(xfs_mount_t *mp)
 	int i;
 	xfs_agnumber_t agcount = mp->m_sb.sb_agcount;
 
-	ba_list = NULL;
-	rt_ba_list = NULL;
+	list_head_init(&ba_list);
+	list_head_init(&rt_ba_list);
 	pthread_mutex_init(&ext_flist_lock, NULL);
 	pthread_mutex_init(&rt_ext_tree_lock, NULL);
 	pthread_mutex_init(&rt_ext_flist_lock, NULL);
@@ -954,9 +957,11 @@ incore_ext_init(xfs_mount_t *mp)
 void
 incore_ext_teardown(xfs_mount_t *mp)
 {
+	extent_alloc_rec_t *cur, *tmp;
 	xfs_agnumber_t i;
 
-	free_allocations(ba_list);
+	list_for_each_entry_safe(cur, tmp, &ba_list, list)
+		free(cur);
 
 	for (i = 0; i < mp->m_sb.sb_agcount; i++)  {
 		free(extent_tree_ptrs[i]);
-- 
1.6.2.107.ge47ee

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] xfs_repair: fix record_allocation list manipulation

[Eric Sandeen]

clang found this one too as a "Dead assignment"

Unless my pointer-fu is totally messed up, this function
was never actually updating the list head.

This would mean that the later free_allocations() calls in
incore_ext_teardown() and free_rt_dup_extent_tree() don't
actually free any items, and therefore leak memory.

V2: now with correct pointer-fu.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

(We could use Jeff's doubly-linked list patch too, though we
really don't need both pointers, and there are still other
singly-linked lists throughout repair...)

clang record:

http://sandeen.net/clang/xfsprogs/2009-09-09-1/report-1Jnl15.html#EndPath

diff --git a/repair/incore.c b/repair/incore.c
index 84626c9..0fd0e89 100644
--- a/repair/incore.c
+++ b/repair/incore.c
@@ -30,10 +30,10 @@
  * if set to NULL if empty.
  */
 void
-record_allocation(ba_rec_t *addr, ba_rec_t *list)
+record_allocation(ba_rec_t *addr, ba_rec_t **list)
 {
-	addr->next = list;
-	list = addr;
+	addr->next = *list;
+	*list = addr;
 
 	return;
 }
diff --git a/repair/incore.h b/repair/incore.h
index 1f0f45a..4f90dd6 100644
--- a/repair/incore.h
+++ b/repair/incore.h
@@ -33,7 +33,7 @@ typedef struct ba_rec  {
 	struct ba_rec	*next;
 } ba_rec_t;
 
-void			record_allocation(ba_rec_t *addr, ba_rec_t *list);
+void			record_allocation(ba_rec_t *addr, ba_rec_t **list);
 void			free_allocations(ba_rec_t *list);
 
 /*
diff --git a/repair/incore_ext.c b/repair/incore_ext.c
index a2acbf4..90d2074 100644
--- a/repair/incore_ext.c
+++ b/repair/incore_ext.c
@@ -120,7 +120,7 @@ mk_extent_tree_nodes(xfs_agblock_t new_startblock,
 			do_error(
 			_("couldn't allocate new extent descriptors.\n"));
 
-		record_allocation(&rec->alloc_rec, ba_list);
+		record_allocation(&rec->alloc_rec, &ba_list);
 
 		new = &rec->extents[0];
 
@@ -678,7 +678,7 @@ mk_rt_extent_tree_nodes(xfs_drtbno_t new_startblock,
 			do_error(
 			_("couldn't allocate new extent descriptors.\n"));
 
-		record_allocation(&rec->alloc_rec, rt_ba_list);
+		record_allocation(&rec->alloc_rec, &rt_ba_list);
 
 		new = &rec->extents[0];
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs_repair: fix record_allocation list manipulation

[Christoph Hellwig]

On Sat, Sep 19, 2009 at 09:42:04AM -0500, Eric Sandeen wrote:
> clang found this one too as a "Dead assignment"
>
> Unless my pointer-fu is totally messed up, this function
> was never actually updating the list head.
>
> This would mean that the later free_allocations() calls in
> incore_ext_teardown() and free_rt_dup_extent_tree() don't
> actually free any items, and therefore leak memory.
>
> V2: now with correct pointer-fu.

Barry already had this in his repair speedups patchkit, but I left it
out for now because I wasn't too sure how this could work at all.
After reviewing it again I noticed that it can actually work because the
addr pointer in the ba_rec_t is unused, and we make use of the fact that
the ba_rec_t is the first field in the structure to be tacked.  Entirely
to subtile for my taste.  Id' prefer to just put a list_head into the
extent_alloc_rec_t and rt_extent_alloc_rec_t and openconde the
tracking/freeing of the beast.  The list_head if just as large as the
ba_rec_t and make sure the list handlinjg is right, and the openconding
gets rid of the annoying assumption that the ba_rec_t is the first thing
in the structure to be tracked.  It should also be a net-removal of
code.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs_repair: fix record_allocation list manipulation

[Eric Sandeen]

Christoph Hellwig wrote:
> On Sat, Sep 19, 2009 at 09:42:04AM -0500, Eric Sandeen wrote:
>> clang found this one too as a "Dead assignment"
>>
>> Unless my pointer-fu is totally messed up, this function
>> was never actually updating the list head.
>>
>> This would mean that the later free_allocations() calls in
>> incore_ext_teardown() and free_rt_dup_extent_tree() don't
>> actually free any items, and therefore leak memory.
>>
>> V2: now with correct pointer-fu.
> 
> Barry already had this in his repair speedups patchkit, but I left it
> out for now because I wasn't too sure how this could work at all.

Hm, the patch as reposted does indeed free the allocations; I double
checked .... on a fairly large filesystem I saw about 10MB of memory
that was lost otherwise; not huge.

> After reviewing it again I noticed that it can actually work 

the original code can work?

> because the
> addr pointer in the ba_rec_t is unused, and we make use of the fact that
> the ba_rec_t is the first field in the structure to be tacked.  Entirely
> to subtile for my taste.  Id' prefer to just put a list_head into the
> extent_alloc_rec_t and rt_extent_alloc_rec_t and openconde the
> tracking/freeing of the beast.  The list_head if just as large as the
> ba_rec_t and make sure the list handlinjg is right, and the openconding
> gets rid of the annoying assumption that the ba_rec_t is the first thing
> in the structure to be tracked.  It should also be a net-removal of
> code.

Yeah, that sounds better.

IF barry's speedups stuff obsoletes this work should I just put it on
the shelf for now?

Sorry; you're probably at linuxcon, I'm having a hard time parsing all
of the quick reply ;)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfs_repair: fix record_allocation list manipulation

[Christoph Hellwig]

On Tue, Sep 22, 2009 at 10:21:36AM -0500, Eric Sandeen wrote:
> > After reviewing it again I noticed that it can actually work 
> 
> the original code can work?

Well, could in theory if fixed as in your patch ;-)

> IF barry's speedups stuff obsoletes this work should I just put it on
> the shelf for now?

No, it doesn't.  As I mentioned his original patches contained a fix
like yours, but I left it out because I didn't understand it yet.

I think the patch will cause some minor merge pain as it touches
incore.c which gets more or less fully rewritten as part of the
patch series.  I think it's useful enough to be put in, but so are
the other repair patches :)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] repair: replaced custom block allocation linked lists with list_heads

[Eric Sandeen]

Josef 'Jeff' Sipek wrote:
> The previous implementation of the linked lists was buggy, and leaked memory.
> 
> From: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
> 
> Cc: sandeen@sandeen.net
> Signed-off-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>

Yeah, ok, this looks better to me.  I just had to rise to the challenge
of fixing it as it was written, I guess ;)

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> ---
> 
> Yes, Eric, it wastes an extra pointer per node, but at least it works
> compared to the code it replaces :)
> 
>  repair/incore.c     |   27 ---------------------------
>  repair/incore.h     |   11 -----------
>  repair/incore_ext.c |   27 ++++++++++++++++-----------
>  3 files changed, 16 insertions(+), 49 deletions(-)
> 
> diff --git a/repair/incore.c b/repair/incore.c
> index 84626c9..27604e2 100644
> --- a/repair/incore.c
> +++ b/repair/incore.c
> @@ -25,33 +25,6 @@
>  #include "err_protos.h"
>  #include "threads.h"
>  
> -/*
> - * push a block allocation record onto list.  assumes list
> - * if set to NULL if empty.
> - */
> -void
> -record_allocation(ba_rec_t *addr, ba_rec_t *list)
> -{
> -	addr->next = list;
> -	list = addr;
> -
> -	return;
> -}
> -
> -void
> -free_allocations(ba_rec_t *list)
> -{
> -	ba_rec_t *current = list;
> -
> -	while (list != NULL)  {
> -		list = list->next;
> -		free(current);
> -		current = list;
> -	}
> -
> -	return;
> -}
> -
>  /* ba bmap setupstuff.  setting/getting state is in incore.h  */
>  
>  void
> diff --git a/repair/incore.h b/repair/incore.h
> index 1f0f45a..a22ef0f 100644
> --- a/repair/incore.h
> +++ b/repair/incore.h
> @@ -26,17 +26,6 @@
>   */
>  
>  /*
> - * block allocation lists
> - */
> -typedef struct ba_rec  {
> -	void		*addr;
> -	struct ba_rec	*next;
> -} ba_rec_t;
> -
> -void			record_allocation(ba_rec_t *addr, ba_rec_t *list);
> -void			free_allocations(ba_rec_t *list);
> -
> -/*
>   * block bit map defs -- track state of each filesystem block.
>   * ba_bmap is an array of bitstrings declared in the globals.h file.
>   * the bitstrings are broken up into 64-bit chunks.  one bitstring per AG.
> diff --git a/repair/incore_ext.c b/repair/incore_ext.c
> index a2acbf4..d0b8cdc 100644
> --- a/repair/incore_ext.c
> +++ b/repair/incore_ext.c
> @@ -31,12 +31,12 @@
>   * paranoia -- account for any weird padding, 64/32-bit alignment, etc.
>   */
>  typedef struct extent_alloc_rec  {
> -	ba_rec_t		alloc_rec;
> +	struct list_head	list;
>  	extent_tree_node_t	extents[ALLOC_NUM_EXTS];
>  } extent_alloc_rec_t;
>  
>  typedef struct rt_extent_alloc_rec  {
> -	ba_rec_t		alloc_rec;
> +	struct list_head	list;
>  	rt_extent_tree_node_t	extents[ALLOC_NUM_EXTS];
>  } rt_extent_alloc_rec_t;
>  
> @@ -89,8 +89,8 @@ static avltree_desc_t	**extent_bcnt_ptrs;	/*
>  /*
>   * list of allocated "blocks" for easy freeing later
>   */
> -static ba_rec_t		*ba_list;
> -static ba_rec_t		*rt_ba_list;
> +static struct list_head	ba_list;
> +static struct list_head	rt_ba_list;
>  
>  /*
>   * locks.
> @@ -120,7 +120,7 @@ mk_extent_tree_nodes(xfs_agblock_t new_startblock,
>  			do_error(
>  			_("couldn't allocate new extent descriptors.\n"));
>  
> -		record_allocation(&rec->alloc_rec, ba_list);
> +		list_add(&rec->list, &ba_list);
>  
>  		new = &rec->extents[0];
>  
> @@ -678,7 +678,7 @@ mk_rt_extent_tree_nodes(xfs_drtbno_t new_startblock,
>  			do_error(
>  			_("couldn't allocate new extent descriptors.\n"));
>  
> -		record_allocation(&rec->alloc_rec, rt_ba_list);
> +		list_add(&rec->list, &rt_ba_list);
>  
>  		new = &rec->extents[0];
>  
> @@ -755,12 +755,15 @@ release_rt_extent_tree()
>  void
>  free_rt_dup_extent_tree(xfs_mount_t *mp)
>  {
> +	rt_extent_alloc_rec_t *cur, *tmp;
> +
>  	ASSERT(mp->m_sb.sb_rblocks != 0);
>  
> -	free_allocations(rt_ba_list);
> +	list_for_each_entry_safe(cur, tmp, &rt_ba_list, list)
> +		free(cur);
> +
>  	free(rt_ext_tree_ptr);
>  
> -	rt_ba_list = NULL;
>  	rt_ext_tree_ptr = NULL;
>  
>  	return;
> @@ -895,8 +898,8 @@ incore_ext_init(xfs_mount_t *mp)
>  	int i;
>  	xfs_agnumber_t agcount = mp->m_sb.sb_agcount;
>  
> -	ba_list = NULL;
> -	rt_ba_list = NULL;
> +	list_head_init(&ba_list);
> +	list_head_init(&rt_ba_list);
>  	pthread_mutex_init(&ext_flist_lock, NULL);
>  	pthread_mutex_init(&rt_ext_tree_lock, NULL);
>  	pthread_mutex_init(&rt_ext_flist_lock, NULL);
> @@ -954,9 +957,11 @@ incore_ext_init(xfs_mount_t *mp)
>  void
>  incore_ext_teardown(xfs_mount_t *mp)
>  {
> +	extent_alloc_rec_t *cur, *tmp;
>  	xfs_agnumber_t i;
>  
> -	free_allocations(ba_list);
> +	list_for_each_entry_safe(cur, tmp, &ba_list, list)
> +		free(cur);
>  
>  	for (i = 0; i < mp->m_sb.sb_agcount; i++)  {
>  		free(extent_tree_ptrs[i]);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs