* [PATCH] shmem: put inode if alloc_file failed @ 2011-03-08 9:14 Bob Liu 2011-03-08 9:15 ` [PATCH] shmem: using goto to replace several return Bob Liu 2011-03-09 22:58 ` [PATCH] shmem: put inode if alloc_file failed Andrew Morton 0 siblings, 2 replies; 7+ messages in thread From: Bob Liu @ 2011-03-08 9:14 UTC (permalink / raw) To: akpm; +Cc: linux-mm, viro, hch, hughd, npiggin, Bob Liu Currently if alloc_file failed, inode willn't be put which may cause small memory leak, this patch fix it. Signed-off-by: Bob Liu <lliubbo@gmail.com> --- mm/shmem.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/mm/shmem.c b/mm/shmem.c index 7c9cdc6..1e2bea7 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2756,17 +2756,19 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags #ifndef CONFIG_MMU error = ramfs_nommu_expand_for_mapping(inode, size); if (error) - goto put_dentry; + goto put_inode; #endif error = -ENFILE; file = alloc_file(&path, FMODE_WRITE | FMODE_READ, &shmem_file_operations); if (!file) - goto put_dentry; + goto put_inode; return file; +put_inode: + iput(inode); put_dentry: path_put(&path); put_memory: -- 1.6.3.3 -- 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> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] shmem: using goto to replace several return 2011-03-08 9:14 [PATCH] shmem: put inode if alloc_file failed Bob Liu @ 2011-03-08 9:15 ` Bob Liu 2011-03-09 23:01 ` Andrew Morton 2011-03-09 22:58 ` [PATCH] shmem: put inode if alloc_file failed Andrew Morton 1 sibling, 1 reply; 7+ messages in thread From: Bob Liu @ 2011-03-08 9:15 UTC (permalink / raw) To: akpm; +Cc: linux-mm, viro, hch, hughd, npiggin, Bob Liu Code clean, use goto to replace several return. Signed-off-by: Bob Liu <lliubbo@gmail.com> --- mm/shmem.c | 32 +++++++++++++++----------------- 1 files changed, 15 insertions(+), 17 deletions(-) diff --git a/mm/shmem.c b/mm/shmem.c index 7c9cdc6..99f5915 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1847,17 +1847,13 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev) &dentry->d_name, NULL, NULL, NULL); if (error) { - if (error != -EOPNOTSUPP) { - iput(inode); - return error; - } + if (error != -EOPNOTSUPP) + goto failed_iput; } #ifdef CONFIG_TMPFS_POSIX_ACL error = generic_acl_init(inode, dir); - if (error) { - iput(inode); - return error; - } + if (error) + goto failed_iput; #else error = 0; #endif @@ -1866,6 +1862,9 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev) d_instantiate(dentry, inode); dget(dentry); /* Extra count - pin the dentry in core */ } + +failed_iput: + iput(inode); return error; } @@ -1987,10 +1986,8 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s error = security_inode_init_security(inode, dir, &dentry->d_name, NULL, NULL, NULL); if (error) { - if (error != -EOPNOTSUPP) { - iput(inode); - return error; - } + if (error != -EOPNOTSUPP) + goto failed_iput; error = 0; } @@ -2002,10 +1999,8 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s inode->i_op = &shmem_symlink_inline_operations; } else { error = shmem_getpage(inode, 0, &page, SGP_WRITE, NULL); - if (error) { - iput(inode); - return error; - } + if (error) + goto failed_iput; inode->i_mapping->a_ops = &shmem_aops; inode->i_op = &shmem_symlink_inode_operations; kaddr = kmap_atomic(page, KM_USER0); @@ -2019,7 +2014,10 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s dir->i_ctime = dir->i_mtime = CURRENT_TIME; d_instantiate(dentry, inode); dget(dentry); - return 0; + +failed_iput: + iput(inode); + return error; } static void *shmem_follow_link_inline(struct dentry *dentry, struct nameidata *nd) -- 1.6.3.3 -- 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> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] shmem: using goto to replace several return 2011-03-08 9:15 ` [PATCH] shmem: using goto to replace several return Bob Liu @ 2011-03-09 23:01 ` Andrew Morton 0 siblings, 0 replies; 7+ messages in thread From: Andrew Morton @ 2011-03-09 23:01 UTC (permalink / raw) To: Bob Liu; +Cc: linux-mm, viro, hch, hughd, npiggin On Tue, 8 Mar 2011 17:15:00 +0800 Bob Liu <lliubbo@gmail.com> wrote: > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1847,17 +1847,13 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev) > &dentry->d_name, NULL, > NULL, NULL); > if (error) { > - if (error != -EOPNOTSUPP) { > - iput(inode); > - return error; > - } > + if (error != -EOPNOTSUPP) > + goto failed_iput; > } > #ifdef CONFIG_TMPFS_POSIX_ACL > error = generic_acl_init(inode, dir); > - if (error) { > - iput(inode); > - return error; > - } > + if (error) > + goto failed_iput; > #else > error = 0; > #endif > @@ -1866,6 +1862,9 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev) > d_instantiate(dentry, inode); > dget(dentry); /* Extra count - pin the dentry in core */ > } > + > +failed_iput: > + iput(inode); > return error; > } This adds a big bug: we newly do iput() on the non-error path. Please, we need much more care and testing than this. -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] shmem: put inode if alloc_file failed 2011-03-08 9:14 [PATCH] shmem: put inode if alloc_file failed Bob Liu 2011-03-08 9:15 ` [PATCH] shmem: using goto to replace several return Bob Liu @ 2011-03-09 22:58 ` Andrew Morton 2011-03-10 0:03 ` Al Viro 1 sibling, 1 reply; 7+ messages in thread From: Andrew Morton @ 2011-03-09 22:58 UTC (permalink / raw) To: Bob Liu; +Cc: linux-mm, viro, hch, hughd, npiggin On Tue, 8 Mar 2011 17:14:59 +0800 Bob Liu <lliubbo@gmail.com> wrote: > Currently if alloc_file failed, inode willn't be put which may cause small > memory leak, this patch fix it. > > Signed-off-by: Bob Liu <lliubbo@gmail.com> > --- > mm/shmem.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 7c9cdc6..1e2bea7 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2756,17 +2756,19 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags > #ifndef CONFIG_MMU > error = ramfs_nommu_expand_for_mapping(inode, size); > if (error) > - goto put_dentry; > + goto put_inode; > #endif > > error = -ENFILE; > file = alloc_file(&path, FMODE_WRITE | FMODE_READ, > &shmem_file_operations); > if (!file) > - goto put_dentry; > + goto put_inode; > > return file; > > +put_inode: > + iput(inode); > put_dentry: > path_put(&path); > put_memory: Is this correct? We've linked the inode to the dentry with d_instantiate(), so the d_put() will do the iput() on the inode. -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] shmem: put inode if alloc_file failed 2011-03-09 22:58 ` [PATCH] shmem: put inode if alloc_file failed Andrew Morton @ 2011-03-10 0:03 ` Al Viro 2011-03-11 10:12 ` Bob Liu 2011-03-11 10:20 ` Bob Liu 0 siblings, 2 replies; 7+ messages in thread From: Al Viro @ 2011-03-10 0:03 UTC (permalink / raw) To: Andrew Morton; +Cc: Bob Liu, linux-mm, hch, hughd, npiggin On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote: > > +put_inode: > > + iput(inode); > > put_dentry: > > path_put(&path); > > put_memory: > > Is this correct? We've linked the inode to the dentry with > d_instantiate(), so the d_put() will do the iput() on the inode. ITYM path_put() and yes, it will. There's no leak. -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] shmem: put inode if alloc_file failed 2011-03-10 0:03 ` Al Viro @ 2011-03-11 10:12 ` Bob Liu 2011-03-11 10:20 ` Bob Liu 1 sibling, 0 replies; 7+ messages in thread From: Bob Liu @ 2011-03-11 10:12 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-mm, hch, hughd, npiggin On Thu, Mar 10, 2011 at 8:03 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote: > >> > +put_inode: >> > + iput(inode); >> > put_dentry: >> > path_put(&path); >> > put_memory: >> >> Is this correct? We've linked the inode to the dentry with >> d_instantiate(), so the d_put() will do the iput() on the inode. > > > ITYM path_put() and yes, it will. There's no leak. > Hi, Yes, I also think path_put() should, But it seems iput() can't free that memory during NO-MMU arch. Would you please take a look at thread: [BUG?] shmem: memory leak on NO-MMU arch ? http://www.spinics.net/lists/linux-mm/msg15668.html Thanks -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] shmem: put inode if alloc_file failed 2011-03-10 0:03 ` Al Viro 2011-03-11 10:12 ` Bob Liu @ 2011-03-11 10:20 ` Bob Liu 1 sibling, 0 replies; 7+ messages in thread From: Bob Liu @ 2011-03-11 10:20 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-mm, hch, hughd, npiggin On Thu, Mar 10, 2011 at 8:03 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote: > >> > +put_inode: >> > + iput(inode); >> > put_dentry: >> > path_put(&path); >> > put_memory: >> >> Is this correct? We've linked the inode to the dentry with >> d_instantiate(), so the d_put() will do the iput() on the inode. > > > ITYM path_put() and yes, it will. There's no leak. > In my opinion, on no-mmu arch iput() will call generate_delete_inode() > truncate_page_range() which just delete the page from pagecache. But pages allocated by ramfs_nommu_expand_for_mapping() have page->_count = 2 (one by alloc_pages + one by pagecache). The page->_count is still 1 after iput(), so the memory can't be freed. Is there something I am wrong? Thanks. -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-03-11 10:20 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-08 9:14 [PATCH] shmem: put inode if alloc_file failed Bob Liu 2011-03-08 9:15 ` [PATCH] shmem: using goto to replace several return Bob Liu 2011-03-09 23:01 ` Andrew Morton 2011-03-09 22:58 ` [PATCH] shmem: put inode if alloc_file failed Andrew Morton 2011-03-10 0:03 ` Al Viro 2011-03-11 10:12 ` Bob Liu 2011-03-11 10:20 ` Bob Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).