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

[Hugh Dickins <hughd@google.com>]

On Mon, 28 Mar 2011, Andrew Morton 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.
> > ...
> > 
> > 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.

I don't think so.

> 
> 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.

No, we don't count 1 for the LRU: it always seems a little odd that
we don't, but that's how it is.  I did dive into the debugger to
check that is really still the case.  And it doesn't really matter
here, since of course we don't count -1 when taking off LRU either.

The pages here are not "in-use" as such: we're just priming the
page cache with them, so they will be found shortly afterwards
when they do come into use, when inserted into the address space.

What if memory pressure comes in and frees them before then?
Er, er, that gave me a nasty turn.  But there's a comment
just above the SetPageDirty visible in Bob's patch, saying
/* prevent the page from being discarded on memory pressure */

> 
> So allocation should do:
> 
> 	alloc_pages()

Yes, it did that (along with a split_page we can ignore here).

> 	add_to_page_cache()
> 	add_to_lru()

And those it did in the combined function add_to_page_cache_lru().

> 
> and deallocation should do
> 
> 	remove_from_lru()
> 	remove_from_page_cache()

Nowadays delete_from_page_cache(), which decrements the reference
acquired in add_to_page_cache().

> 	put_page()
> 
> If this protocol is followed correctly, there is no need to do a
> put_page() during the allocation/setup phase!

There is a get_page() when each page is mapped into the address
space, which then matches the final put_page() you show above.

> 
> I suspect that the problem in nommu really lies in the
> deallocation/teardown phase.

Hugh

--
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>