Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212035 > unrolled thread

[PATCH 2/2] f2fs: fix to release inode correctly

Back to article view | Back to linux.kernel

Contents

  [PATCH 2/2] f2fs: fix to release inode correctly Chao Yu <chao2.yu@samsung.com> - 2015-08-24 11:50 +0200
    Re: [PATCH 2/2] f2fs: fix to release inode correctly Jaegeuk Kim <jaegeuk@kernel.org> - 2015-08-24 19:00 +0200
      Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly Jaegeuk Kim <jaegeuk@kernel.org> - 2015-08-25 01:00 +0200
        RE: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly Chao Yu <chao2.yu@samsung.com> - 2015-08-25 08:50 +0200
          Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly Jaegeuk Kim <jaegeuk@kernel.org> - 2015-08-26 00:50 +0200
            RE: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly Chao Yu <chao2.yu@samsung.com> - 2015-08-26 14:50 +0200
      RE: [PATCH 2/2] f2fs: fix to release inode correctly Chao Yu <chao2.yu@samsung.com> - 2015-08-25 03:50 +0200
        Re: [PATCH 2/2] f2fs: fix to release inode correctly Jaegeuk Kim <jaegeuk@kernel.org> - 2015-08-26 00:20 +0200

#1212035 — [PATCH 2/2] f2fs: fix to release inode correctly

In following call stack, if unfortunately we lose all chances to truncate
inode page in remove_inode_page, eventually we will add the nid allocated
previously into free nid cache, this nid is with NID_NEW status and with
NEW_ADDR in its blkaddr pointer:

 - f2fs_create
  - f2fs_add_link
   - __f2fs_add_link
    - init_inode_metadata
     - new_inode_page
      - new_node_page
       - set_node_addr(, NEW_ADDR)
     - f2fs_init_acl   failed
     - remove_inode_page  failed
  - handle_failed_inode
   - remove_inode_page  failed
   - iput
    - f2fs_evict_inode
     - remove_inode_page  failed
     - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR

This may not only cause resource leak of previous inode, but also may cause
incorrect use of the previous blkaddr which is located in NO.nid node entry
when this nid is reused by others.

This patch tries to add this inode to orphan list if we fail to truncate
inode, so that we can obtain a second chance to release it in orphan
recovery flow.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 fs/f2fs/f2fs.h  |  2 +-
 fs/f2fs/inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
 fs/f2fs/node.c  | 14 +++++++++-----
 3 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 806439f..69827ee 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1687,7 +1687,7 @@ int get_dnode_of_data(struct dnode_of_data *, pgoff_t, int);
 int truncate_inode_blocks(struct inode *, pgoff_t);
 int truncate_xattr_node(struct inode *, struct page *);
 int wait_on_node_pages_writeback(struct f2fs_sb_info *, nid_t);
-void remove_inode_page(struct inode *);
+int remove_inode_page(struct inode *);
 struct page *new_inode_page(struct inode *);
 struct page *new_node_page(struct dnode_of_data *, unsigned int, struct page *);
 void ra_node_page(struct f2fs_sb_info *, nid_t);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index d1b03d0..35aae65 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -317,6 +317,7 @@ void f2fs_evict_inode(struct inode *inode)
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	nid_t xnid = fi->i_xattr_nid;
+	int err = 0;
 
 	/* some remained atomic pages should discarded */
 	if (f2fs_is_atomic_file(inode))
@@ -342,11 +343,13 @@ void f2fs_evict_inode(struct inode *inode)
 	i_size_write(inode, 0);
 
 	if (F2FS_HAS_BLOCKS(inode))
-		f2fs_truncate(inode, true);
+		err = f2fs_truncate(inode, true);
 
-	f2fs_lock_op(sbi);
-	remove_inode_page(inode);
-	f2fs_unlock_op(sbi);
+	if (!err) {
+		f2fs_lock_op(sbi);
+		err = remove_inode_page(inode);
+		f2fs_unlock_op(sbi);
+	}
 
 	sb_end_intwrite(inode->i_sb);
 no_delete:
@@ -362,9 +365,26 @@ no_delete:
 	if (is_inode_flag_set(fi, FI_UPDATE_WRITE))
 		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
 	if (is_inode_flag_set(fi, FI_FREE_NID)) {
-		alloc_nid_failed(sbi, inode->i_ino);
+		if (err && err != -ENOENT)
+			alloc_nid_done(sbi, inode->i_ino);
+		else
+			alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(fi, FI_FREE_NID);
 	}
+
+	if (err && err != -ENOENT) {
+		if (!exist_written_data(sbi, inode->i_ino, ORPHAN_INO)) {
+			/*
+			 * get here because we failed to release resource
+			 * of inode previously, reminder our user to run fsck
+			 * for fixing.
+			 */
+			set_sbi_flag(sbi, SBI_NEED_FSCK);
+			f2fs_msg(sbi->sb, KERN_WARNING,
+				"inode (ino:%lu) resource leak, run fsck "
+				"to fix this issue!", inode->i_ino);
+		}
+	}
 out_clear:
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
 	if (fi->i_crypt_info)
@@ -377,6 +397,7 @@ out_clear:
 void handle_failed_inode(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	int err = 0;
 
 	clear_nlink(inode);
 	make_bad_inode(inode);
@@ -384,9 +405,27 @@ void handle_failed_inode(struct inode *inode)
 
 	i_size_write(inode, 0);
 	if (F2FS_HAS_BLOCKS(inode))
-		f2fs_truncate(inode, false);
+		err = f2fs_truncate(inode, false);
+
+	if (!err)
+		err = remove_inode_page(inode);
 
-	remove_inode_page(inode);
+	/*
+	 * if we skip truncate_node in remove_inode_page bacause we failed
+	 * before, it's better to find another way to release resource of
+	 * this inode (e.g. valid block count, node block or nid). Here we
+	 * choose to add this inode to orphan list, so that we can call iput
+	 * for releasing in orphan recovery flow.
+	 *
+	 * Note: we should add inode to orphan list before f2fs_unlock_op()
+	 * so we can prevent losing this orphan when encoutering checkpoint
+	 * and following suddenly power-off.
+	 */
+	if (err && err != -ENOENT) {
+		err = acquire_orphan_inode(sbi);
+		if (!err)
+			add_orphan_inode(sbi, inode->i_ino);
+	}
 
 	set_inode_flag(F2FS_I(inode), FI_FREE_NID);
 	f2fs_unlock_op(sbi);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 0867325..27d1a74 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -902,17 +902,20 @@ int truncate_xattr_node(struct inode *inode, struct page *page)
  * Caller should grab and release a rwsem by calling f2fs_lock_op() and
  * f2fs_unlock_op().
  */
-void remove_inode_page(struct inode *inode)
+int remove_inode_page(struct inode *inode)
 {
 	struct dnode_of_data dn;
+	int err;
 
 	set_new_dnode(&dn, inode, NULL, NULL, inode->i_ino);
-	if (get_dnode_of_data(&dn, 0, LOOKUP_NODE))
-		return;
+	err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
+	if (err)
+		return err;
 
-	if (truncate_xattr_node(inode, dn.inode_page)) {
+	err = truncate_xattr_node(inode, dn.inode_page);
+	if (err) {
 		f2fs_put_dnode(&dn);
-		return;
+		return err;
 	}
 
 	/* remove potential inline_data blocks */
@@ -926,6 +929,7 @@ void remove_inode_page(struct inode *inode)
 
 	/* will put inode & node pages */
 	truncate_node(&dn);
+	return 0;
 }
 
 struct page *new_inode_page(struct inode *inode)
-- 
2.4.2


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212349

On Mon, Aug 24, 2015 at 05:40:45PM +0800, Chao Yu wrote:
> In following call stack, if unfortunately we lose all chances to truncate
> inode page in remove_inode_page, eventually we will add the nid allocated
> previously into free nid cache, this nid is with NID_NEW status and with
> NEW_ADDR in its blkaddr pointer:
> 
>  - f2fs_create
>   - f2fs_add_link
>    - __f2fs_add_link
>     - init_inode_metadata
>      - new_inode_page
>       - new_node_page
>        - set_node_addr(, NEW_ADDR)
>      - f2fs_init_acl   failed
>      - remove_inode_page  failed
>   - handle_failed_inode
>    - remove_inode_page  failed
>    - iput
>     - f2fs_evict_inode
>      - remove_inode_page  failed
>      - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR
> 
> This may not only cause resource leak of previous inode, but also may cause
> incorrect use of the previous blkaddr which is located in NO.nid node entry
> when this nid is reused by others.
> 
> This patch tries to add this inode to orphan list if we fail to truncate
> inode, so that we can obtain a second chance to release it in orphan
> recovery flow.
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  fs/f2fs/f2fs.h  |  2 +-
>  fs/f2fs/inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  fs/f2fs/node.c  | 14 +++++++++-----
>  3 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 806439f..69827ee 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1687,7 +1687,7 @@ int get_dnode_of_data(struct dnode_of_data *, pgoff_t, int);
>  int truncate_inode_blocks(struct inode *, pgoff_t);
>  int truncate_xattr_node(struct inode *, struct page *);
>  int wait_on_node_pages_writeback(struct f2fs_sb_info *, nid_t);
> -void remove_inode_page(struct inode *);
> +int remove_inode_page(struct inode *);
>  struct page *new_inode_page(struct inode *);
>  struct page *new_node_page(struct dnode_of_data *, unsigned int, struct page *);
>  void ra_node_page(struct f2fs_sb_info *, nid_t);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index d1b03d0..35aae65 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -317,6 +317,7 @@ void f2fs_evict_inode(struct inode *inode)
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	nid_t xnid = fi->i_xattr_nid;
> +	int err = 0;
>  
>  	/* some remained atomic pages should discarded */
>  	if (f2fs_is_atomic_file(inode))
> @@ -342,11 +343,13 @@ void f2fs_evict_inode(struct inode *inode)
>  	i_size_write(inode, 0);
>  
>  	if (F2FS_HAS_BLOCKS(inode))
> -		f2fs_truncate(inode, true);
> +		err = f2fs_truncate(inode, true);
>  
> -	f2fs_lock_op(sbi);
> -	remove_inode_page(inode);
> -	f2fs_unlock_op(sbi);
> +	if (!err) {
> +		f2fs_lock_op(sbi);
> +		err = remove_inode_page(inode);
> +		f2fs_unlock_op(sbi);
> +	}
>  
>  	sb_end_intwrite(inode->i_sb);
>  no_delete:
> @@ -362,9 +365,26 @@ no_delete:
>  	if (is_inode_flag_set(fi, FI_UPDATE_WRITE))
>  		add_dirty_inode(sbi, inode->i_ino, UPDATE_INO);
>  	if (is_inode_flag_set(fi, FI_FREE_NID)) {
> -		alloc_nid_failed(sbi, inode->i_ino);
> +		if (err && err != -ENOENT)
> +			alloc_nid_done(sbi, inode->i_ino);
> +		else
> +			alloc_nid_failed(sbi, inode->i_ino);
>  		clear_inode_flag(fi, FI_FREE_NID);
>  	}
> +
> +	if (err && err != -ENOENT) {
> +		if (!exist_written_data(sbi, inode->i_ino, ORPHAN_INO)) {
> +			/*
> +			 * get here because we failed to release resource
> +			 * of inode previously, reminder our user to run fsck
> +			 * for fixing.
> +			 */
> +			set_sbi_flag(sbi, SBI_NEED_FSCK);
> +			f2fs_msg(sbi->sb, KERN_WARNING,
> +				"inode (ino:%lu) resource leak, run fsck "
> +				"to fix this issue!", inode->i_ino);
> +		}
> +	}
>  out_clear:
>  #ifdef CONFIG_F2FS_FS_ENCRYPTION
>  	if (fi->i_crypt_info)
> @@ -377,6 +397,7 @@ out_clear:
>  void handle_failed_inode(struct inode *inode)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	int err = 0;
>  
>  	clear_nlink(inode);
>  	make_bad_inode(inode);
> @@ -384,9 +405,27 @@ void handle_failed_inode(struct inode *inode)
>  
>  	i_size_write(inode, 0);
>  	if (F2FS_HAS_BLOCKS(inode))
> -		f2fs_truncate(inode, false);
> +		err = f2fs_truncate(inode, false);
> +
> +	if (!err)
> +		err = remove_inode_page(inode);
>  
> -	remove_inode_page(inode);
> +	/*
> +	 * if we skip truncate_node in remove_inode_page bacause we failed
> +	 * before, it's better to find another way to release resource of
> +	 * this inode (e.g. valid block count, node block or nid). Here we
> +	 * choose to add this inode to orphan list, so that we can call iput
> +	 * for releasing in orphan recovery flow.
> +	 *
> +	 * Note: we should add inode to orphan list before f2fs_unlock_op()
> +	 * so we can prevent losing this orphan when encoutering checkpoint
> +	 * and following suddenly power-off.
> +	 */
> +	if (err && err != -ENOENT) {
> +		err = acquire_orphan_inode(sbi);
> +		if (!err)
> +			add_orphan_inode(sbi, inode->i_ino);

		Need this too?

		if (err)
			set_sbi_flag(sbi, SBI_NEED_FSCK);

Thanks,

> +	}
>  
>  	set_inode_flag(F2FS_I(inode), FI_FREE_NID);
>  	f2fs_unlock_op(sbi);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 0867325..27d1a74 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -902,17 +902,20 @@ int truncate_xattr_node(struct inode *inode, struct page *page)
>   * Caller should grab and release a rwsem by calling f2fs_lock_op() and
>   * f2fs_unlock_op().
>   */
> -void remove_inode_page(struct inode *inode)
> +int remove_inode_page(struct inode *inode)
>  {
>  	struct dnode_of_data dn;
> +	int err;
>  
>  	set_new_dnode(&dn, inode, NULL, NULL, inode->i_ino);
> -	if (get_dnode_of_data(&dn, 0, LOOKUP_NODE))
> -		return;
> +	err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
> +	if (err)
> +		return err;
>  
> -	if (truncate_xattr_node(inode, dn.inode_page)) {
> +	err = truncate_xattr_node(inode, dn.inode_page);
> +	if (err) {
>  		f2fs_put_dnode(&dn);
> -		return;
> +		return err;
>  	}
>  
>  	/* remove potential inline_data blocks */
> @@ -926,6 +929,7 @@ void remove_inode_page(struct inode *inode)
>  
>  	/* will put inode & node pages */
>  	truncate_node(&dn);
> +	return 0;
>  }
>  
>  struct page *new_inode_page(struct inode *inode)
> -- 
> 2.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212567 — Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly

Hi Chao,

On Mon, Aug 24, 2015 at 09:54:23AM -0700, Jaegeuk Kim wrote:
> On Mon, Aug 24, 2015 at 05:40:45PM +0800, Chao Yu wrote:
> > In following call stack, if unfortunately we lose all chances to truncate
> > inode page in remove_inode_page, eventually we will add the nid allocated
> > previously into free nid cache, this nid is with NID_NEW status and with
> > NEW_ADDR in its blkaddr pointer:
> > 
> >  - f2fs_create
> >   - f2fs_add_link
> >    - __f2fs_add_link
> >     - init_inode_metadata
> >      - new_inode_page
> >       - new_node_page
> >        - set_node_addr(, NEW_ADDR)
> >      - f2fs_init_acl   failed
> >      - remove_inode_page  failed
> >   - handle_failed_inode
> >    - remove_inode_page  failed
> >    - iput
> >     - f2fs_evict_inode
> >      - remove_inode_page  failed
> >      - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR

Unfortunately, this couldn't fix my bug case.
I'm still struggling to find out something tho.
Meanwhile, let's stay with both of the patches.

Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212734 — RE: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, August 25, 2015 6:53 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly
> 
> Hi Chao,
> 
> On Mon, Aug 24, 2015 at 09:54:23AM -0700, Jaegeuk Kim wrote:
> > On Mon, Aug 24, 2015 at 05:40:45PM +0800, Chao Yu wrote:
> > > In following call stack, if unfortunately we lose all chances to truncate
> > > inode page in remove_inode_page, eventually we will add the nid allocated
> > > previously into free nid cache, this nid is with NID_NEW status and with
> > > NEW_ADDR in its blkaddr pointer:
> > >
> > >  - f2fs_create
> > >   - f2fs_add_link
> > >    - __f2fs_add_link
> > >     - init_inode_metadata
> > >      - new_inode_page
> > >       - new_node_page
> > >        - set_node_addr(, NEW_ADDR)
> > >      - f2fs_init_acl   failed
> > >      - remove_inode_page  failed
> > >   - handle_failed_inode
> > >    - remove_inode_page  failed
> > >    - iput
> > >     - f2fs_evict_inode
> > >      - remove_inode_page  failed
> > >      - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR
> 
> Unfortunately, this couldn't fix my bug case.

Another thing I note is that: we do not cover free_nid_list_lock with build_lock,
so when we are building free nid cache, we can change the status of free nid
cache, so I guess it is one possible suspect who cause our nid issue.

And, could you share me the information for reproducing the nid reallocation
issue? So I can reproduce in my environment for invistigating.

> I'm still struggling to find out something tho.
> Meanwhile, let's stay with both of the patches.

OK.

Thanks,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213415 — Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly

On Tue, Aug 25, 2015 at 02:38:00PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Tuesday, August 25, 2015 6:53 AM
> > To: Chao Yu
> > Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly
> > 
> > Hi Chao,
> > 
> > On Mon, Aug 24, 2015 at 09:54:23AM -0700, Jaegeuk Kim wrote:
> > > On Mon, Aug 24, 2015 at 05:40:45PM +0800, Chao Yu wrote:
> > > > In following call stack, if unfortunately we lose all chances to truncate
> > > > inode page in remove_inode_page, eventually we will add the nid allocated
> > > > previously into free nid cache, this nid is with NID_NEW status and with
> > > > NEW_ADDR in its blkaddr pointer:
> > > >
> > > >  - f2fs_create
> > > >   - f2fs_add_link
> > > >    - __f2fs_add_link
> > > >     - init_inode_metadata
> > > >      - new_inode_page
> > > >       - new_node_page
> > > >        - set_node_addr(, NEW_ADDR)
> > > >      - f2fs_init_acl   failed
> > > >      - remove_inode_page  failed
> > > >   - handle_failed_inode
> > > >    - remove_inode_page  failed
> > > >    - iput
> > > >     - f2fs_evict_inode
> > > >      - remove_inode_page  failed
> > > >      - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR
> > 
> > Unfortunately, this couldn't fix my bug case.
> 
> Another thing I note is that: we do not cover free_nid_list_lock with build_lock,
> so when we are building free nid cache, we can change the status of free nid
> cache, so I guess it is one possible suspect who cause our nid issue.

But, when building the free nids, it doesn't add any candidate if there is
an existing entry in the list which might be set as NID_ALLOC.

> 
> And, could you share me the information for reproducing the nid reallocation
> issue? So I can reproduce in my environment for invistigating.

Sure. I've running ubuntu on virtualbox equipped with an 8GB virtual partition
with 1GB main memory.
The issue occurs during sometimes xfstests, or sometimes fsstress which fills up
the partition to 100%.
With my quick fix, I couldn't meet that bug for two days run.

Thanks,

> 
> > I'm still struggling to find out something tho.
> > Meanwhile, let's stay with both of the patches.
> 
> OK.
> 
> Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213824 — RE: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, August 26, 2015 6:40 AM
> To: Chao Yu
> Cc: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly
> 
> On Tue, Aug 25, 2015 at 02:38:00PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Tuesday, August 25, 2015 6:53 AM
> > > To: Chao Yu
> > > Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> > > Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to release inode correctly
> > >
> > > Hi Chao,
> > >
> > > On Mon, Aug 24, 2015 at 09:54:23AM -0700, Jaegeuk Kim wrote:
> > > > On Mon, Aug 24, 2015 at 05:40:45PM +0800, Chao Yu wrote:
> > > > > In following call stack, if unfortunately we lose all chances to truncate
> > > > > inode page in remove_inode_page, eventually we will add the nid allocated
> > > > > previously into free nid cache, this nid is with NID_NEW status and with
> > > > > NEW_ADDR in its blkaddr pointer:
> > > > >
> > > > >  - f2fs_create
> > > > >   - f2fs_add_link
> > > > >    - __f2fs_add_link
> > > > >     - init_inode_metadata
> > > > >      - new_inode_page
> > > > >       - new_node_page
> > > > >        - set_node_addr(, NEW_ADDR)
> > > > >      - f2fs_init_acl   failed
> > > > >      - remove_inode_page  failed
> > > > >   - handle_failed_inode
> > > > >    - remove_inode_page  failed
> > > > >    - iput
> > > > >     - f2fs_evict_inode
> > > > >      - remove_inode_page  failed
> > > > >      - alloc_nid_failed   cache a nid with valid blkaddr: NEW_ADDR
> > >
> > > Unfortunately, this couldn't fix my bug case.
> >
> > Another thing I note is that: we do not cover free_nid_list_lock with build_lock,
> > so when we are building free nid cache, we can change the status of free nid
> > cache, so I guess it is one possible suspect who cause our nid issue.
> 
> But, when building the free nids, it doesn't add any candidate if there is
> an existing entry in the list which might be set as NID_ALLOC.
> 
> >
> > And, could you share me the information for reproducing the nid reallocation
> > issue? So I can reproduce in my environment for invistigating.
> 
> Sure. I've running ubuntu on virtualbox equipped with an 8GB virtual partition
> with 1GB main memory.
> The issue occurs during sometimes xfstests, or sometimes fsstress which fills up
> the partition to 100%.
> With my quick fix, I couldn't meet that bug for two days run.

Thank you for the hint, :) I'm trying to reproduce the issue.

Thanks,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212628

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, August 25, 2015 12:54 AM
> To: Chao Yu
> Cc: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] f2fs: fix to release inode correctly
> 

[snip]

> > +	 * if we skip truncate_node in remove_inode_page bacause we failed
> > +	 * before, it's better to find another way to release resource of
> > +	 * this inode (e.g. valid block count, node block or nid). Here we
> > +	 * choose to add this inode to orphan list, so that we can call iput
> > +	 * for releasing in orphan recovery flow.
> > +	 *
> > +	 * Note: we should add inode to orphan list before f2fs_unlock_op()
> > +	 * so we can prevent losing this orphan when encoutering checkpoint
> > +	 * and following suddenly power-off.
> > +	 */
> > +	if (err && err != -ENOENT) {
> > +		err = acquire_orphan_inode(sbi);
> > +		if (!err)
> > +			add_orphan_inode(sbi, inode->i_ino);
> 
> 		Need this too?
> 
> 		if (err)
> 			set_sbi_flag(sbi, SBI_NEED_FSCK);

We have another chance to release inode resource in following path:
 - handle_failed_inode
  - iput
   - f2fs_evict_inode
    - f2fs_truncate
    - remove_inode_page

So I choose to set SBI_NEED_FSCK in the end of f2fs_evict_inode.

Thanks,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213406

On Tue, Aug 25, 2015 at 09:39:55AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Tuesday, August 25, 2015 12:54 AM
> > To: Chao Yu
> > Cc: linux-f2fs-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] f2fs: fix to release inode correctly
> > 
> 
> [snip]
> 
> > > +	 * if we skip truncate_node in remove_inode_page bacause we failed
> > > +	 * before, it's better to find another way to release resource of
> > > +	 * this inode (e.g. valid block count, node block or nid). Here we
> > > +	 * choose to add this inode to orphan list, so that we can call iput
> > > +	 * for releasing in orphan recovery flow.
> > > +	 *
> > > +	 * Note: we should add inode to orphan list before f2fs_unlock_op()
> > > +	 * so we can prevent losing this orphan when encoutering checkpoint
> > > +	 * and following suddenly power-off.
> > > +	 */
> > > +	if (err && err != -ENOENT) {
> > > +		err = acquire_orphan_inode(sbi);
> > > +		if (!err)
> > > +			add_orphan_inode(sbi, inode->i_ino);
> > 
> > 		Need this too?
> > 
> > 		if (err)
> > 			set_sbi_flag(sbi, SBI_NEED_FSCK);
> 
> We have another chance to release inode resource in following path:
>  - handle_failed_inode
>   - iput
>    - f2fs_evict_inode
>     - f2fs_truncate
>     - remove_inode_page
> 
> So I choose to set SBI_NEED_FSCK in the end of f2fs_evict_inode.

Oh, got it. :)

> 
> Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web