* [PATCH] f2fs: fix to release inode page in get_new_data_page @ 2015-07-09 10:20 Chao Yu 2015-07-11 0:17 ` Jaegeuk Kim 0 siblings, 1 reply; 5+ messages in thread From: Chao Yu @ 2015-07-09 10:20 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel get_new_data_page should release inode page when we encounter any error in its procedure, but there is one error path didn't cover this, fix it. Signed-off-by: Chao Yu <chao2.yu@samsung.com> --- fs/f2fs/data.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 08dfdc6..ea8898b 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode, int err; repeat: page = grab_cache_page(mapping, index); - if (!page) + if (!page) { + f2fs_put_page(ipage, 1); return ERR_PTR(-ENOMEM); + } set_new_dnode(&dn, inode, ipage, NULL, 0); err = f2fs_reserve_block(&dn, index); -- 2.4.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: fix to release inode page in get_new_data_page 2015-07-09 10:20 [PATCH] f2fs: fix to release inode page in get_new_data_page Chao Yu @ 2015-07-11 0:17 ` Jaegeuk Kim 2015-07-11 2:02 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 5+ messages in thread From: Jaegeuk Kim @ 2015-07-11 0:17 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel Hi Chao, On Thu, Jul 09, 2015 at 06:20:08PM +0800, Chao Yu wrote: > get_new_data_page should release inode page when we encounter any > error in its procedure, but there is one error path didn't cover > this, fix it. Nice catch. But, I think we should fix its caller: in init_inode_metadata(), err = make_empty_dir(); if (err) goto put_error; --------------- Thanks, > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > --- > fs/f2fs/data.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 08dfdc6..ea8898b 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode, > int err; > repeat: > page = grab_cache_page(mapping, index); > - if (!page) > + if (!page) { > + f2fs_put_page(ipage, 1); > return ERR_PTR(-ENOMEM); > + } > > set_new_dnode(&dn, inode, ipage, NULL, 0); > err = f2fs_reserve_block(&dn, index); > -- > 2.4.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page 2015-07-11 0:17 ` Jaegeuk Kim @ 2015-07-11 2:02 ` Chao Yu 2015-07-13 23:25 ` Jaegeuk Kim 0 siblings, 1 reply; 5+ messages in thread From: Chao Yu @ 2015-07-11 2:02 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-kernel, linux-f2fs-devel Hi Jaegeuk, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Saturday, July 11, 2015 8:17 AM > To: Chao Yu; Chao Yu > Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; > linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page > > Hi Chao, > > On Thu, Jul 09, 2015 at 06:20:08PM +0800, Chao Yu wrote: > > get_new_data_page should release inode page when we encounter any > > error in its procedure, but there is one error path didn't cover > > this, fix it. > > Nice catch. > But, I think we should fix its caller: > > in init_inode_metadata(), > err = make_empty_dir(); > if (err) > goto put_error; > --------------- Previously, I fixed in the same way, but I got an oops when I test the patch with xfstest suit, it shows we will meet an error in this call path IIRC: ->f2fs_mkdir ->__f2fs_add_link ->init_inode_metadata ->make_empty_dir ->get_new_data_page ->f2fs_reserve_block ->reserve_new_block ->inc_valid_block_count return -ENOSPC; ->f2fs_put_dnode ->f2fs_put_page(ipage, 1) put_error: ->f2fs_put_page(ipage, 1); f2fs_bug_on(F2FS_P_SB(page), !PageLocked(page)); And I don't think we should change error handling method of f2fs_put_dnode for just fixing this issue. How do you think? Thanks, > Thanks, > > > > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > > --- > > fs/f2fs/data.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 08dfdc6..ea8898b 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode, > > int err; > > repeat: > > page = grab_cache_page(mapping, index); > > - if (!page) > > + if (!page) { > > + f2fs_put_page(ipage, 1); > > return ERR_PTR(-ENOMEM); > > + } > > > > set_new_dnode(&dn, inode, ipage, NULL, 0); > > err = f2fs_reserve_block(&dn, index); > > -- > > 2.4.2 > > ------------------------------------------------------------------------------ > Don't Limit Your Business. Reach for the Cloud. > GigeNET's Cloud Solutions provide you with the tools and support that > you need to offload your IT needs and focus on growing your business. > Configured For All Businesses. Start Your Cloud Today. > https://www.gigenetcloud.com/ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page 2015-07-11 2:02 ` [f2fs-dev] " Chao Yu @ 2015-07-13 23:25 ` Jaegeuk Kim 2015-07-14 10:13 ` Chao Yu 0 siblings, 1 reply; 5+ messages in thread From: Jaegeuk Kim @ 2015-07-13 23:25 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel Hi Chao, On Sat, Jul 11, 2015 at 10:02:51AM +0800, Chao Yu wrote: > Hi Jaegeuk, > > > -----Original Message----- > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > > Sent: Saturday, July 11, 2015 8:17 AM > > To: Chao Yu; Chao Yu > > Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; > > linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page > > > > Hi Chao, > > > > On Thu, Jul 09, 2015 at 06:20:08PM +0800, Chao Yu wrote: > > > get_new_data_page should release inode page when we encounter any > > > error in its procedure, but there is one error path didn't cover > > > this, fix it. > > > > Nice catch. > > But, I think we should fix its caller: > > > > in init_inode_metadata(), > > err = make_empty_dir(); > > if (err) > > goto put_error; > > --------------- > > Previously, I fixed in the same way, but I got an oops when I test the > patch with xfstest suit, it shows we will meet an error in this call > path IIRC: > > ->f2fs_mkdir > ->__f2fs_add_link > ->init_inode_metadata > ->make_empty_dir > ->get_new_data_page > ->f2fs_reserve_block > ->reserve_new_block > ->inc_valid_block_count > return -ENOSPC; > ->f2fs_put_dnode > ->f2fs_put_page(ipage, 1) > > put_error: > ->f2fs_put_page(ipage, 1); > f2fs_bug_on(F2FS_P_SB(page), !PageLocked(page)); > > And I don't think we should change error handling method of f2fs_put_dnode > for just fixing this issue. > > How do you think? Indeed. I cannot think about other clean way for now. Instead, how about adding this description in the patch and some comments in the codes? Thanks, ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page 2015-07-13 23:25 ` Jaegeuk Kim @ 2015-07-14 10:13 ` Chao Yu 0 siblings, 0 replies; 5+ messages in thread From: Chao Yu @ 2015-07-14 10:13 UTC (permalink / raw) To: 'Jaegeuk Kim'; +Cc: linux-kernel, linux-f2fs-devel > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Tuesday, July 14, 2015 7:26 AM > To: Chao Yu > Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page [snip] > > And I don't think we should change error handling method of f2fs_put_dnode > > for just fixing this issue. > > > > How do you think? > > Indeed. I cannot think about other clean way for now. > Instead, how about adding this description in the patch and some comments in > the codes? OK, Please help to review the following patch. :) Thanks, ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-14 10:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-09 10:20 [PATCH] f2fs: fix to release inode page in get_new_data_page Chao Yu 2015-07-11 0:17 ` Jaegeuk Kim 2015-07-11 2:02 ` [f2fs-dev] " Chao Yu 2015-07-13 23:25 ` Jaegeuk Kim 2015-07-14 10:13 ` Chao Yu
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).