Re: [PATCH] ramfs: fix memleak on no-mmu arch

[Bob Liu <lliubbo@gmail.com>]

Hi, Andrew

cc'd some folks working on nommu.

On Tue, Mar 29, 2011 at 8:02 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 28 Mar 2011 13:32:35 +0800
> Bob Liu <lliubbo@gmail.com> wrote:
>
>> On no-mmu arch, there is a memleak duirng shmem test.
>> The cause of this memleak is ramfs_nommu_expand_for_mapping() added page
>> refcount to 2 which makes iput() can't free that pages.
>>
>> The simple test file is like this:
>> int main(void)
>> {
>>       int i;
>>       key_t k = ftok("/etc", 42);
>>
>>       for ( i=0; i<100; ++i) {
>>               int id = shmget(k, 10000, 0644|IPC_CREAT);
>>               if (id == -1) {
>>                       printf("shmget error\n");
>>               }
>>               if(shmctl(id, IPC_RMID, NULL ) == -1) {
>>                       printf("shm  rm error\n");
>>                       return -1;
>>               }
>>       }
>>       printf("run ok...\n");
>>       return 0;
>> }
>>
>> ...
>>
>> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
>> index 9eead2c..fbb0b47 100644
>> --- a/fs/ramfs/file-nommu.c
>> +++ b/fs/ramfs/file-nommu.c
>> @@ -112,6 +112,7 @@ int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
>>               SetPageDirty(page);
>>
>>               unlock_page(page);
>> +             put_page(page);
>>       }
>>
>>       return 0;
>
> Something is still wrong here.
>
> A live, in-use page should have a refcount of three.  One for the
> existence of the page, one for its presence on the page LRU and one for
> its existence in the pagecache radix tree.
>
> So allocation should do:
>
>        alloc_pages()
>        add_to_page_cache()
>        add_to_lru()
>
> and deallocation should do
>
>        remove_from_lru()
>        remove_from_page_cache()
>        put_page()
>
> If this protocol is followed correctly, there is no need to do a
> put_page() during the allocation/setup phase!
>
> I suspect that the problem in nommu really lies in the
> deallocation/teardown phase.
>

What about below patch ?

BTW: It seems that in MMU cases shmem pages are freed during memory reclaim,
since I didn't find the direct free place.
I am not sure maybe I got something wrong ?

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 fs/ramfs/file-nommu.c |    1 +
 fs/ramfs/inode.c      |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index fbb0b47..11f48eb 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -114,6 +114,7 @@ int ramfs_nommu_expand_for_mapping(struct inode
*inode, size_t newsize)
 		unlock_page(page);
 		put_page(page);
 	}
+	inode->i_private = pages;

 	return 0;

diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index eacb166..e446d9f 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -151,9 +151,31 @@ static const struct inode_operations
ramfs_dir_inode_operations = {
 	.rename		= simple_rename,
 };

+#ifndef CONFIG_MMU
+static void ramfs_evict_inode(struct inode *inode)
+{
+	int i;
+	struct page *free_pages = (struct page *)inode->i_private;
+
+	/*
+	 * for nommu arch, need an extra put_page so that pages gotten
+	 * by ramfs_nommu_expand_for_mapping() can be freed
+	 */
+	for (i = 0; i < inode->i_data.nrpages; i++)
+		put_page(free_pages + i);
+
+	if (inode->i_data.nrpages)
+		truncate_inode_pages(&inode->i_data, 0);
+	end_writeback(inode);
+}
+#endif
+
 static const struct super_operations ramfs_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
+#ifndef CONFIG_MMU
+	.evict_inode    = ramfs_evict_inode,
+#endif
 	.show_options	= generic_show_options,
 };

-- 
1.6.3.3

-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>