xfsprogs: fix some xfs_repair memory leaks
cover xfsprogs: fix some xfs_repair memory leaks
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/4 xfsprogs: Free all data in libxfs_umount; call from xfs_copy as well
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.
--- copy/xfs_copy.c | 2 ++ libxfs/init.c | 9 +++++++++ 2 files changed, 11 insertions(+), 0 deletions(-)
copy/xfs_copy.c +2 -0
@@ -1165,6 +1165,8 @@ main(int argc, char **argv) }check_errors (); +libxfs_umount (mp); +return 0 ; }
libxfs/init.c +9 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/4] xfsprogs: Free all data in libxfs_umount; call from xfs_copy as well
Quoted 3 lines · Eric Sandeen
libxfs_umountwas failing to free a handful of resources; fix that up. Call it fromxfs_copyas well, while we're at it; every otherlibxfs_mounthas alibxfs_umountcounterpart, 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/4 xfs_repair: free msgbuf on exit
In reply to Eric SandeenJust to keep valgrind less noisy, and make it easiser to spot more things that actually matter ...
--- repair/xfs_repair.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
repair/xfs_repair.c +2 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/4 xfs_repair: call IRELE(ip) after libxfs_trans_iget calls
In reply to Eric SandeenCommit 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
--- repair/phase6.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
repair/phase6.c +6 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/4 xfs_repair: unconditionally free blockmaps when threads complete
In reply to Eric Sandeenblkmap_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.
--- repair/bmap.c | 17 ++++++++++++++++- repair/bmap.h | 1 + repair/phase3.c | 2 ++ repair/phase4.c | 1 + 4 files changed, 20 insertions(+), 1 deletions(-)
repair/bmap.c +16 -1
@@ -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. */
repair/bmap.h +1 -0
@@ -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);
repair/phase3.c +2 -0
@@ -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); }
repair/phase4.c +1 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs