[PATCH 0/4] xfsprogs: fix some xfs_repair memory leaks

[PATCH 0/4] xfsprogs: fix some xfs_repair memory leaks

[Eric Sandeen]

This fixes the fairly trivial "definitely lost" leaks spotted by valgrind
on a trivial xfs_repair run, two of which are coverity-spotted.

Dave, I *did* do a quick test via ./check -g quick, this time.  :)

Thanks,
-Eric

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

[PATCH 1/4] xfsprogs: Free all data in libxfs_umount; call from xfs_copy as well

[Eric Sandeen]

libxfs_umount was failing to free a handful of resources; fix that up.
Call it from xfs_copy as well, while we're at it; every other libxfs_mount
has a libxfs_umount counterpart, at least on a clean exit.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 copy/xfs_copy.c |    2 ++
 libxfs/init.c   |    9 +++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index e13f468..4497b7f 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -1165,6 +1165,8 @@ main(int argc, char **argv)
 	}
 
 	check_errors();
+	libxfs_umount(mp);
+
 	return 0;
 }
 
diff --git a/libxfs/init.c b/libxfs/init.c
index 2859f94..c7f9dc8 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -832,6 +832,15 @@ libxfs_umount(xfs_mount_t *mp)
 		pag = radix_tree_delete(&mp->m_perag_tree, agno);
 		kmem_free(pag);
 	}
+
+	kmem_free(mp->m_attr_geo);
+	kmem_free(mp->m_dir_geo);
+
+	kmem_free(mp->m_rtdev_targp);
+	if (mp->m_logdev_targp != mp->m_ddev_targp)
+		kmem_free(mp->m_logdev_targp);
+	kmem_free(mp->m_ddev_targp);
+	
 }
 
 /*
-- 1.7.1


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

[PATCH 2/4] xfs_repair: free msgbuf on exit

[Eric Sandeen]

Just to keep valgrind less noisy, and make it easiser to spot
more things that actually matter ...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/xfs_repair.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index db703d0..85a012b 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -946,5 +946,7 @@ _("Repair of readonly mount complete.  Immediate reboot encouraged.\n"));
 
 	pftrace_done();
 
+	free(msgbuf);
+
 	return (0);
 }
-- 1.7.1

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

[PATCH 3/4] xfs_repair: call IRELE(ip) after libxfs_trans_iget calls

[Eric Sandeen]

Commit 260c85e libxfs: dont free xfs_inode until complete
changed the alloc/free convention a bit:

    Originally, the xfs_inode are released upon the first
    call to xfs_trans_cancel, xfs_trans_commit, or
    inode_item_done.
    <snip>
    This patch does the following:
     1) Removes the iput from the transaction completion and
        requires that the xfs_inode allocators call IRELE()
        when they are done with the pointer.

But that change missed several callers in xfs_repair phase6;
fix that up.

Addresses-Coverity-Id: 1315100
Addresses-Coverity-Id: 1315101
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/phase6.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 9cfedbf..04638c2 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -585,6 +585,7 @@ mk_rbmino(xfs_mount_t *mp)
 			error);
 	}
 	libxfs_trans_commit(tp);
+	IRELE(ip);
 }
 
 static int
@@ -654,6 +655,7 @@ _("can't access block %" PRIu64 " (fsbno %" PRIu64 ") of realtime bitmap inode %
 	}
 
 	libxfs_trans_commit(tp);
+	IRELE(ip);
 	return(0);
 }
 
@@ -714,6 +716,7 @@ fill_rsumino(xfs_mount_t *mp)
 			do_warn(
 _("can't access block %" PRIu64 " (fsbno %" PRIu64 ") of realtime summary inode %" PRIu64 "\n"),
 				bno, map.br_startblock, mp->m_sb.sb_rsumino);
+			IRELE(ip);
 			return(1);
 		}
 
@@ -726,6 +729,7 @@ _("can't access block %" PRIu64 " (fsbno %" PRIu64 ") of realtime summary inode
 	}
 
 	libxfs_trans_commit(tp);
+	IRELE(ip);
 	return(0);
 }
 
@@ -846,6 +850,7 @@ mk_rsumino(xfs_mount_t *mp)
 			error);
 	}
 	libxfs_trans_commit(tp);
+	IRELE(ip);
 }
 
 /*
@@ -920,6 +925,7 @@ mk_root_dir(xfs_mount_t *mp)
 	libxfs_dir_init(tp, ip, ip);
 
 	libxfs_trans_commit(tp);
+	IRELE(ip);
 
 	irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino),
 				XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rootino));
-- 1.7.1


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

[PATCH 4/4] xfs_repair: unconditionally free blockmaps when threads complete

[Eric Sandeen]

blkmap_free() doesn't actually free the block map unless it's
inordinately large; this keeps us from constantly freeing
and re-allocating blockmaps for each inode, which makes sense.

However, once the threads which have allocated these structures
exit, we should actually free them; they can grow up to 2MB
for each of the data and attr maps, for each thread, and not
be freed through the normal blkmap_free() test.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/bmap.c   |   17 ++++++++++++++++-
 repair/bmap.h   |    1 +
 repair/phase3.c |    2 ++
 repair/phase4.c |    1 +
 4 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/repair/bmap.c b/repair/bmap.c
index 2655632..abe9f48 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -82,7 +82,8 @@ blkmap_alloc(
  * extents) then free it to release the memory. This prevents us from pinning
  * large tracts of memory due to corrupted fork values or one-off fragmented
  * files. Otherwise we have nothing to do but keep the memory around for the
- * next inode
+ * next inode.
+ * When the thread is done, it should do an unconditional, final free.
  */
 void
 blkmap_free(
@@ -103,6 +104,20 @@ blkmap_free(
 	free(blkmap);
 }
 
+void
+blkmap_free_final(void)
+{
+	blkmap_t	*blkmap;
+
+	blkmap = pthread_getspecific(dblkmap_key);
+	pthread_setspecific(dblkmap_key, NULL);
+	free(blkmap);
+
+	blkmap = pthread_getspecific(ablkmap_key);
+	pthread_setspecific(ablkmap_key, NULL);
+	free(blkmap);
+}
+
 /*
  * Get one entry from a block map.
  */
diff --git a/repair/bmap.h b/repair/bmap.h
index 973081a..501ef6b 100644
--- a/repair/bmap.h
+++ b/repair/bmap.h
@@ -58,6 +58,7 @@ extern pthread_key_t ablkmap_key;
 
 blkmap_t	*blkmap_alloc(xfs_extnum_t nex, int whichfork);
 void		blkmap_free(blkmap_t *blkmap);
+void		blkmap_free_final(void);
 
 int		blkmap_set_ext(blkmap_t **blkmapp, xfs_fileoff_t o,
 			       xfs_fsblock_t b, xfs_filblks_t c);
diff --git a/repair/phase3.c b/repair/phase3.c
index 20786af..76c9440 100644
--- a/repair/phase3.c
+++ b/repair/phase3.c
@@ -27,6 +27,7 @@
 #include "err_protos.h"
 #include "dinode.h"
 #include "progress.h"
+#include "bmap.h"
 
 static void
 process_agi_unlinked(
@@ -75,6 +76,7 @@ process_ag_func(
 	wait_for_inode_prefetch(arg);
 	do_log(_("        - agno = %d\n"), agno);
 	process_aginodes(wq->mp, arg, agno, 1, 0, 1);
+	blkmap_free_final();
 	cleanup_inode_prefetch(arg);
 }
 
diff --git a/repair/phase4.c b/repair/phase4.c
index e0571e8..1a7d7b5 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -138,6 +138,7 @@ process_ag_func(
 	wait_for_inode_prefetch(arg);
 	do_log(_("        - agno = %d\n"), agno);
 	process_aginodes(wq->mp, arg, agno, 0, 1, 0);
+	blkmap_free_final();
 	cleanup_inode_prefetch(arg);
 
 	/*
-- 1.7.1

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

Re: [PATCH 1/4] xfsprogs: Free all data in libxfs_umount; call from xfs_copy as well

[Dave Chinner]

On Thu, Aug 20, 2015 at 01:08:27PM -0500, Eric Sandeen wrote:
> libxfs_umount was failing to free a handful of resources; fix that up.
> Call it from xfs_copy as well, while we're at it; every other libxfs_mount
> has a libxfs_umount counterpart, at least on a clean exit.

xfs/077 6s ... - output mismatch (see /home/dave/src/xfstests-dev/results//xfs/077.out.bad)
    --- tests/xfs/077.out       2015-08-05 15:00:15.000000000 +1000
    +++ /home/dave/src/xfstests-dev/results//xfs/077.out.bad    2015-08-24 11:35:42.000000000 +1000
    @@ -18,4 +18,6 @@
     writing all SBs
     new UUID = <EXACTUUID>
     == xfs_copy with new UUID
    +cache_purge: shake on cache 0x64b2e0 left 1 nodes!?
     == xfs_copy with duplicate UUID
    +cache_purge: shake on cache 0x64b2e0 left 1 nodes!?
    ...

It would seem that this exposes a buffer refcount issue in xfs_copy?

Oh, yeah, the superblock. Patch modified to include this hunk:

@@ -696,6 +696,7 @@ main(int argc, char **argv)
 	sbp = libxfs_readbuf(mbuf.m_ddev_targp, XFS_SB_DADDR,
 			     1 << (sb->sb_sectlog - BBSHIFT),
 			     0, &xfs_sb_buf_ops);
+	libxfs_putbuf(sbp);
 
 	mp = libxfs_mount(&mbuf, sb, xargs.ddev, xargs.logdev, xargs.rtdev, 0);
 	if (mp == NULL) {

Which makes the immediate problem go away.

Eric, can you send another patch here to abort xfs_copy if the
superblock is detected as corrupt by the verifier?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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