Re: [Findutils-patches] [PATCH] find memory leak

[Bernhard Voelker]

On 12/15/2016 10:19 PM, Goffredo Baroncelli wrote:
> Hi All,
> 
> (please put me in CC in case of reply because I am not subscribed to the 
> mailing list)
> 
> I find a memory leak in the find command. In the function 
> free_file_system_list() (file fstype.c) the field me_mntroot of the struct 
> mount_entry is never free-ed. 
> 
> Commit c6148bca89e9465fd6ba3a10d273ec4cb58c2dbe in gnulibs added this field; 
> this field is filled by the function read_file_system_list(). To correct this 
> problem, we should call the free_mount_entry() function instead of doing the 
> free of the fields.
> 
> What seems strange to me is that this patch is quite old (August 2015), but 
> nobody noticed that. On my machine, find required up to 3GB during the 
> "updatedb" !!!
> 
> E.g.
> 
> # Without the patch, this command requires up to 602MB of memory !!!
> $ /usr/bin/time ./find /lib/modules/ -ignore_readdir_race      \( -fstype NFS 
> -o -fstype nfs -o -fstype nfs4 -o -fstype afs -o -fstype binfmt_misc -o 
> -fstype proc -o -fstype smbfs -o -fstype autofs -o -fstype iso9660 -o -fstype 
> ncpfs -o -fstype coda -o -fstype devpts -o -fstype ftpfs -o -fstype devfs -o 
> -fstype mfs -o -fstype shfs -o -fstype sysfs -o -fstype cifs -o -fstype 
> lustre_lite -o -fstype tmpfs -o -fstype usbfs -o -fstype udf -o -fstype ocfs2 
> -o      -type d -false  \) -prune -o -print0  | wc -l
> 31.59user 27.17system 0:58.82elapsed 99%CPU (0avgtext+0avgdata 
> 602476maxresident)k
> 0inputs+0outputs (0major+150129minor)pagefaults 0swaps
> 
> # With the patch, the same command as above requires only 2MB of memory !!!
> $ /usr/bin/time ./find /lib/modules/ -ignore_readdir_race      \( -fstype NFS 
> -o -fstype nfs -o -fstype nfs4 -o -fstype afs -o -fstype binfmt_misc -o 
> -fstype proc -o -fstype smbfs -o -fstype autofs -o -fstype iso9660 -o -fstype 
> ncpfs -o -fstype coda -o -fstype devpts -o -fstype ftpfs -o -fstype devfs -o 
> -fstype mfs -o -fstype shfs -o -fstype sysfs -o -fstype cifs -o -fstype 
> lustre_lite -o -fstype tmpfs -o -fstype usbfs -o -fstype udf -o -fstype ocfs2 
> -o      -type d -false  \) -prune -o -print0  | wc -l
> 30.94user 26.14system 0:57.14elapsed 99%CPU (0avgtext+0avgdata 
> 2764maxresident)k
> 0inputs+0outputs (0major+165minor)pagefaults 0swaps
> 
> 
> # for curiosity my /lib/modules contains
> $ find /lib/modules/ | wc -l
> 68101
> 
> Finally, what about caching the value of read_file_system_list() instead of 
> recomputing it every time ? On the basis of my tests I found an impressive 
> gain in terms of cpu usage. But what are the cons ?
> 
> 
> BR
> G.Baroncelli

I cannot reproduce here - i.e. I do not see such a huge memory leak here ... 
even
if I add some 8 bind-mounts so that /proc/self/mountinfo has 30k.
How does your test system look like? Is there anything special about your 
directory
structure?  What does 'valgrind --leak-check=full --show-leak-kinds=all ./find 
...'
tell you about the leak(s)? How many lost blocks do you have?
However, your patch is fine, of course; I wrapped it into a nice commit:

  [PATCH 1/5] find: fix memory leak in mount list handling

I also added a wrapper around gnulib's read_file_system_list() in order to
always free the list in the next invocation.  By this, we still get the same
semantics.

  [PATCH 2/5] maint: remove unused get_mounted_filesystems
  [PATCH 3/5] maint: inline now last use of must_read_fs_list
  [PATCH 4/5] maint: move is_used_fs_type to fstype.c
  [PATCH 5/5] find: avoid unneccessary reading of the mount list

One could argument that reading the mount list just once would be
enough, yet this would maybe change the result for automounted file systems.
Well, we could still do this further optimization at a later step.

Have a nice day,
Berny

0001-find-fix-memory-leak-in-mount-list-handling.patch
Description: Text Data

0002-maint-remove-unused-get_mounted_filesystems.patch
Description: Text Data

0003-maint-inline-now-last-use-of-must_read_fs_list.patch
Description: Text Data

0004-maint-move-is_used_fs_type-to-fstype.c.patch
Description: Text Data

0005-find-avoid-unneccessary-reading-of-the-mount-list.patch
Description: Text Data