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

Groups > linux.kernel > #1594699 > unrolled thread

[PATCH 1/4] f2fs: fix wrong error injection for evict_inode

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] f2fs: fix wrong error injection for evict_inode Jaegeuk Kim <jaegeuk@kernel.org> - 2017-03-07 23:30 +0100
    [PATCH 2/4] f2fs: build stat_info before orphan inode recovery Jaegeuk Kim <jaegeuk@kernel.org> - 2017-03-07 23:30 +0100
      Re: [f2fs-dev] [PATCH 2/4] f2fs: build stat_info before orphan inode  recovery Chao Yu <yuchao0@huawei.com> - 2017-03-09 10:20 +0100
        Re: [f2fs-dev] [PATCH 2/4] f2fs: build stat_info before orphan inode  recovery Jaegeuk Kim <jaegeuk@kernel.org> - 2017-03-10 00:50 +0100
      Re: [PATCH 2/4 v2] f2fs: build stat_info before orphan inode recovery Jaegeuk Kim <jaegeuk@kernel.org> - 2017-03-10 00:30 +0100
    [PATCH 3/4] f2fs: show more precise message on orphan recovery failure Jaegeuk Kim <jaegeuk@kernel.org> - 2017-03-07 23:30 +0100
    Re: [f2fs-dev] [PATCH 1/4] f2fs: fix wrong error injection for  evict_inode Chao Yu <yuchao0@huawei.com> - 2017-03-09 10:00 +0100

#1594699 — [PATCH 1/4] f2fs: fix wrong error injection for evict_inode

The previous one was not a proper location to inject an error, since there
is no point to get errors. Instead, we can emulate EIO during truncation,
and the below logic should handle it correctly.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index ef8610bf950f..2520fa72b23f 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -372,13 +372,6 @@ void f2fs_evict_inode(struct inode *inode)
 	if (inode->i_nlink || is_bad_inode(inode))
 		goto no_delete;
 
-#ifdef CONFIG_F2FS_FAULT_INJECTION
-	if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
-		f2fs_show_injection_info(FAULT_EVICT_INODE);
-		goto no_delete;
-	}
-#endif
-
 	remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
 	remove_ino_entry(sbi, inode->i_ino, UPDATE_INO);
 
@@ -389,6 +382,12 @@ void f2fs_evict_inode(struct inode *inode)
 	if (F2FS_HAS_BLOCKS(inode))
 		err = f2fs_truncate(inode);
 
+#ifdef CONFIG_F2FS_FAULT_INJECTION
+	if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
+		f2fs_show_injection_info(FAULT_EVICT_INODE);
+		err = -EIO;
+	}
+#endif
 	if (!err) {
 		f2fs_lock_op(sbi);
 		err = remove_inode_page(inode);
-- 
2.11.0

[toc] | [next] | [standalone]

#1594701 — [PATCH 2/4] f2fs: build stat_info before orphan inode recovery

f2fs_sync_fs() -> write_checkpoint() calls stat_inc_cp_count(sbi->stat_info),
which needs stat_info allocation.
Otherwise, we can hit:

[254042.598623]  ? count_shadow_nodes+0xa0/0xa0
[254042.598633]  f2fs_sync_fs+0x65/0xd0 [f2fs]
[254042.598645]  f2fs_balance_fs_bg+0xe4/0x1c0 [f2fs]
[254042.598657]  f2fs_write_node_pages+0x34/0x1a0 [f2fs]
[254042.598664]  ? pagevec_lookup_entries+0x1e/0x30
[254042.598673]  do_writepages+0x1e/0x30
[254042.598682]  __writeback_single_inode+0x45/0x330
[254042.598688]  writeback_single_inode+0xd7/0x190
[254042.598694]  write_inode_now+0x86/0xa0
[254042.598699]  iput+0x122/0x200
[254042.598709]  f2fs_fill_super+0xd4a/0x14d0 [f2fs]
[254042.598717]  mount_bdev+0x184/0x1c0
[254042.598934]  ? f2fs_commit_super+0x100/0x100 [f2fs]
[254042.599142]  f2fs_mount+0x15/0x20 [f2fs]
[254042.599349]  mount_fs+0x39/0x160
[254042.599554]  ? __alloc_percpu+0x15/0x20
[254042.599759]  vfs_kern_mount+0x67/0x110
[254042.599972]  do_mount+0x1bb/0xc80
[254042.600175]  ? memdup_user+0x42/0x60
[254042.600380]  SyS_mount+0x83/0xd0
[254042.600583]  entry_SYSCALL_64_fastpath+0x1e/0xad

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 96fe8ed73100..cfb40d3fd875 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2022,6 +2022,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 
 	f2fs_join_shrinker(sbi);
 
+	err = f2fs_build_stats(sbi);
+	if (err)
+		goto free_nm;
+
 	/* if there are nt orphan nodes free them */
 	err = recover_orphan_inodes(sbi);
 	if (err)
@@ -2046,10 +2050,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_root_inode;
 	}
 
-	err = f2fs_build_stats(sbi);
-	if (err)
-		goto free_root_inode;
-
 	if (f2fs_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
 
@@ -2143,7 +2143,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		remove_proc_entry("segment_bits", sbi->s_proc);
 		remove_proc_entry(sb->s_id, f2fs_proc_root);
 	}
-	f2fs_destroy_stats(sbi);
 free_root_inode:
 	dput(sb->s_root);
 	sb->s_root = NULL;
@@ -2161,6 +2160,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	truncate_inode_pages_final(META_MAPPING(sbi));
 	iput(sbi->node_inode);
 	mutex_unlock(&sbi->umount_mutex);
+	f2fs_destroy_stats(sbi);
 free_nm:
 	destroy_node_manager(sbi);
 free_sm:
-- 
2.11.0

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

#1595822 — Re: [f2fs-dev] [PATCH 2/4] f2fs: build stat_info before orphan inode recovery

On 2017/3/8 6:13, Jaegeuk Kim wrote:
> f2fs_sync_fs() -> write_checkpoint() calls stat_inc_cp_count(sbi->stat_info),
> which needs stat_info allocation.
> Otherwise, we can hit:
> 
> [254042.598623]  ? count_shadow_nodes+0xa0/0xa0
> [254042.598633]  f2fs_sync_fs+0x65/0xd0 [f2fs]
> [254042.598645]  f2fs_balance_fs_bg+0xe4/0x1c0 [f2fs]
> [254042.598657]  f2fs_write_node_pages+0x34/0x1a0 [f2fs]
> [254042.598664]  ? pagevec_lookup_entries+0x1e/0x30
> [254042.598673]  do_writepages+0x1e/0x30
> [254042.598682]  __writeback_single_inode+0x45/0x330
> [254042.598688]  writeback_single_inode+0xd7/0x190
> [254042.598694]  write_inode_now+0x86/0xa0
> [254042.598699]  iput+0x122/0x200
> [254042.598709]  f2fs_fill_super+0xd4a/0x14d0 [f2fs]
> [254042.598717]  mount_bdev+0x184/0x1c0
> [254042.598934]  ? f2fs_commit_super+0x100/0x100 [f2fs]
> [254042.599142]  f2fs_mount+0x15/0x20 [f2fs]
> [254042.599349]  mount_fs+0x39/0x160
> [254042.599554]  ? __alloc_percpu+0x15/0x20
> [254042.599759]  vfs_kern_mount+0x67/0x110
> [254042.599972]  do_mount+0x1bb/0xc80
> [254042.600175]  ? memdup_user+0x42/0x60
> [254042.600380]  SyS_mount+0x83/0xd0
> [254042.600583]  entry_SYSCALL_64_fastpath+0x1e/0xad
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/super.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 96fe8ed73100..cfb40d3fd875 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2022,6 +2022,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	f2fs_join_shrinker(sbi);
>  
> +	err = f2fs_build_stats(sbi);
> +	if (err)
> +		goto free_nm;

Both node_inode and shrinker need to be released and detached.

How about relocate f2fs_build_stats in front of build_segment_manager?

Thanks,

> +
>  	/* if there are nt orphan nodes free them */
>  	err = recover_orphan_inodes(sbi);
>  	if (err)
> @@ -2046,10 +2050,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  		goto free_root_inode;
>  	}
>  
> -	err = f2fs_build_stats(sbi);
> -	if (err)
> -		goto free_root_inode;
> -
>  	if (f2fs_proc_root)
>  		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
>  
> @@ -2143,7 +2143,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  		remove_proc_entry("segment_bits", sbi->s_proc);
>  		remove_proc_entry(sb->s_id, f2fs_proc_root);
>  	}
> -	f2fs_destroy_stats(sbi);
>  free_root_inode:
>  	dput(sb->s_root);
>  	sb->s_root = NULL;
> @@ -2161,6 +2160,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>  	truncate_inode_pages_final(META_MAPPING(sbi));
>  	iput(sbi->node_inode);
>  	mutex_unlock(&sbi->umount_mutex);
> +	f2fs_destroy_stats(sbi);
>  free_nm:
>  	destroy_node_manager(sbi);
>  free_sm:
> 

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

#1596486 — Re: [f2fs-dev] [PATCH 2/4] f2fs: build stat_info before orphan inode recovery

On 03/09, Chao Yu wrote:
> On 2017/3/8 6:13, Jaegeuk Kim wrote:
> > f2fs_sync_fs() -> write_checkpoint() calls stat_inc_cp_count(sbi->stat_info),
> > which needs stat_info allocation.
> > Otherwise, we can hit:
> > 
> > [254042.598623]  ? count_shadow_nodes+0xa0/0xa0
> > [254042.598633]  f2fs_sync_fs+0x65/0xd0 [f2fs]
> > [254042.598645]  f2fs_balance_fs_bg+0xe4/0x1c0 [f2fs]
> > [254042.598657]  f2fs_write_node_pages+0x34/0x1a0 [f2fs]
> > [254042.598664]  ? pagevec_lookup_entries+0x1e/0x30
> > [254042.598673]  do_writepages+0x1e/0x30
> > [254042.598682]  __writeback_single_inode+0x45/0x330
> > [254042.598688]  writeback_single_inode+0xd7/0x190
> > [254042.598694]  write_inode_now+0x86/0xa0
> > [254042.598699]  iput+0x122/0x200
> > [254042.598709]  f2fs_fill_super+0xd4a/0x14d0 [f2fs]
> > [254042.598717]  mount_bdev+0x184/0x1c0
> > [254042.598934]  ? f2fs_commit_super+0x100/0x100 [f2fs]
> > [254042.599142]  f2fs_mount+0x15/0x20 [f2fs]
> > [254042.599349]  mount_fs+0x39/0x160
> > [254042.599554]  ? __alloc_percpu+0x15/0x20
> > [254042.599759]  vfs_kern_mount+0x67/0x110
> > [254042.599972]  do_mount+0x1bb/0xc80
> > [254042.600175]  ? memdup_user+0x42/0x60
> > [254042.600380]  SyS_mount+0x83/0xd0
> > [254042.600583]  entry_SYSCALL_64_fastpath+0x1e/0xad
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/super.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 96fe8ed73100..cfb40d3fd875 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -2022,6 +2022,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  
> >  	f2fs_join_shrinker(sbi);
> >  
> > +	err = f2fs_build_stats(sbi);
> > +	if (err)
> > +		goto free_nm;
> 
> Both node_inode and shrinker need to be released and detached.
> 
> How about relocate f2fs_build_stats in front of build_segment_manager?

I just got another panic to try this, since sys/../f2fs/status needs them.

Thanks,

> 
> Thanks,
> 
> > +
> >  	/* if there are nt orphan nodes free them */
> >  	err = recover_orphan_inodes(sbi);
> >  	if (err)
> > @@ -2046,10 +2050,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  		goto free_root_inode;
> >  	}
> >  
> > -	err = f2fs_build_stats(sbi);
> > -	if (err)
> > -		goto free_root_inode;
> > -
> >  	if (f2fs_proc_root)
> >  		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
> >  
> > @@ -2143,7 +2143,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  		remove_proc_entry("segment_bits", sbi->s_proc);
> >  		remove_proc_entry(sb->s_id, f2fs_proc_root);
> >  	}
> > -	f2fs_destroy_stats(sbi);
> >  free_root_inode:
> >  	dput(sb->s_root);
> >  	sb->s_root = NULL;
> > @@ -2161,6 +2160,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >  	truncate_inode_pages_final(META_MAPPING(sbi));
> >  	iput(sbi->node_inode);
> >  	mutex_unlock(&sbi->umount_mutex);
> > +	f2fs_destroy_stats(sbi);
> >  free_nm:
> >  	destroy_node_manager(sbi);
> >  free_sm:
> > 

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

#1596467 — Re: [PATCH 2/4 v2] f2fs: build stat_info before orphan inode recovery

Change log from v1:
 o move its location up more

From e993253fef6c4a07a65e6038c2c0cae684844047 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Tue, 7 Mar 2017 13:41:22 -0800
Subject: [PATCH] f2fs: build stat_info before orphan inode recovery

f2fs_sync_fs() -> write_checkpoint() calls stat_inc_cp_count(sbi->stat_info),
which needs stat_info allocation.
Otherwise, we can hit:

[254042.598623]  ? count_shadow_nodes+0xa0/0xa0
[254042.598633]  f2fs_sync_fs+0x65/0xd0 [f2fs]
[254042.598645]  f2fs_balance_fs_bg+0xe4/0x1c0 [f2fs]
[254042.598657]  f2fs_write_node_pages+0x34/0x1a0 [f2fs]
[254042.598664]  ? pagevec_lookup_entries+0x1e/0x30
[254042.598673]  do_writepages+0x1e/0x30
[254042.598682]  __writeback_single_inode+0x45/0x330
[254042.598688]  writeback_single_inode+0xd7/0x190
[254042.598694]  write_inode_now+0x86/0xa0
[254042.598699]  iput+0x122/0x200
[254042.598709]  f2fs_fill_super+0xd4a/0x14d0 [f2fs]
[254042.598717]  mount_bdev+0x184/0x1c0
[254042.598934]  ? f2fs_commit_super+0x100/0x100 [f2fs]
[254042.599142]  f2fs_mount+0x15/0x20 [f2fs]
[254042.599349]  mount_fs+0x39/0x160
[254042.599554]  ? __alloc_percpu+0x15/0x20
[254042.599759]  vfs_kern_mount+0x67/0x110
[254042.599972]  do_mount+0x1bb/0xc80
[254042.600175]  ? memdup_user+0x42/0x60
[254042.600380]  SyS_mount+0x83/0xd0
[254042.600583]  entry_SYSCALL_64_fastpath+0x1e/0xad

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 96fe8ed73100..939cb7bfa7bc 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -789,9 +789,6 @@ static void f2fs_put_super(struct super_block *sb)
 	/* be sure to wait for any on-going discard commands */
 	f2fs_wait_discard_bio(sbi, NULL_ADDR);
 
-	/* write_checkpoint can update stat informaion */
-	f2fs_destroy_stats(sbi);
-
 	/*
 	 * normally superblock is clean, so we need to release this.
 	 * In addition, EIO will skip do checkpoint, we need this as well.
@@ -811,6 +808,9 @@ static void f2fs_put_super(struct super_block *sb)
 	destroy_node_manager(sbi);
 	destroy_segment_manager(sbi);
 
+	/* write_checkpoint can update stat informaion */
+	f2fs_destroy_stats(sbi);
+
 	kfree(sbi->ckpt);
 	kobject_put(&sbi->s_kobj);
 	wait_for_completion(&sbi->s_kobj_unregister);
@@ -1985,6 +1985,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 
 	init_ino_entry_info(sbi);
 
+	err = f2fs_build_stats(sbi);
+	if (err)
+		goto free_devices;
+
 	/* setup f2fs internal modules */
 	err = build_segment_manager(sbi);
 	if (err) {
@@ -2046,10 +2050,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_root_inode;
 	}
 
-	err = f2fs_build_stats(sbi);
-	if (err)
-		goto free_root_inode;
-
 	if (f2fs_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
 
@@ -2143,7 +2143,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		remove_proc_entry("segment_bits", sbi->s_proc);
 		remove_proc_entry(sb->s_id, f2fs_proc_root);
 	}
-	f2fs_destroy_stats(sbi);
 free_root_inode:
 	dput(sb->s_root);
 	sb->s_root = NULL;
@@ -2165,6 +2164,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	destroy_node_manager(sbi);
 free_sm:
 	destroy_segment_manager(sbi);
+	f2fs_destroy_stats(sbi);
 free_devices:
 	destroy_device_list(sbi);
 	kfree(sbi->ckpt);
-- 
2.11.0

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

#1594702 — [PATCH 3/4] f2fs: show more precise message on orphan recovery failure

This case is not caused by fsck.f2fs. User needs to retry mount.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 91734cc13f71..36ccc30ed54b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -568,7 +568,7 @@ static int recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
 	if (ni.blk_addr != NULL_ADDR) {
 		set_sbi_flag(sbi, SBI_NEED_FSCK);
 		f2fs_msg(sbi->sb, KERN_WARNING,
-			"%s: orphan failed (ino=%x), run fsck to fix.",
+			"%s: orphan failed (ino=%x) by kernel, retry mount.",
 				__func__, ino);
 		return -EIO;
 	}
-- 
2.11.0

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

#1595809 — Re: [f2fs-dev] [PATCH 1/4] f2fs: fix wrong error injection for evict_inode

Hi Jaegeuk,

On 2017/3/8 6:13, Jaegeuk Kim wrote:
> The previous one was not a proper location to inject an error, since there
> is no point to get errors. Instead, we can emulate EIO during truncation,
> and the below logic should handle it correctly.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/inode.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ef8610bf950f..2520fa72b23f 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -372,13 +372,6 @@ void f2fs_evict_inode(struct inode *inode)
>  	if (inode->i_nlink || is_bad_inode(inode))
>  		goto no_delete;
>  
> -#ifdef CONFIG_F2FS_FAULT_INJECTION
> -	if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
> -		f2fs_show_injection_info(FAULT_EVICT_INODE);
> -		goto no_delete;
> -	}
> -#endif
> -
>  	remove_ino_entry(sbi, inode->i_ino, APPEND_INO);
>  	remove_ino_entry(sbi, inode->i_ino, UPDATE_INO);
>  
> @@ -389,6 +382,12 @@ void f2fs_evict_inode(struct inode *inode)
>  	if (F2FS_HAS_BLOCKS(inode))

In addition, how about add below code to emulate error came from f2fs_truncate?

	if (F2FS_HAS_BLOCKS(inode)) {
#ifdef CONFIG_F2FS_FAULT_INJECTION
		if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
			f2fs_show_injection_info(FAULT_EVICT_INODE);
			err = -EIO;
		}
#endif
		if (!err)
			err = f2fs_truncate(inode);
	}

Thanks,

>  		err = f2fs_truncate(inode);
>  
> +#ifdef CONFIG_F2FS_FAULT_INJECTION
> +	if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
> +		f2fs_show_injection_info(FAULT_EVICT_INODE);
> +		err = -EIO;
> +	}
> +#endif
>  	if (!err) {
>  		f2fs_lock_op(sbi);
>  		err = remove_inode_page(inode);
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web