* [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-kernel, linux-f2fs-devel 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 ------------------------------------------------------------------------------ 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/ ^ 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 ` 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-kernel, linux-f2fs-devel 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 ------------------------------------------------------------------------------ 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [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 ------------------------------------------------------------------------------ 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: fix to release inode page in get_new_data_page 2015-07-11 2:02 ` Chao Yu @ 2015-07-13 23:25 ` Jaegeuk Kim 2015-07-14 10:13 ` [f2fs-dev] " 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, ------------------------------------------------------------------------------ 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/ ^ 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:13 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 ` Chao Yu 2015-07-13 23:25 ` Jaegeuk Kim 2015-07-14 10:13 ` [f2fs-dev] " 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).