Re: [f2fs-dev] [PATCH 4/5] f2fs: get rid of stale fault injection code

[Yufen Yu]

On 2022/4/12 5:20, Jaegeuk Kim wrote:
> On 04/11, Chao Yu wrote:
> > On 2022/4/6 11:01, Yufen Yu via Linux-f2fs-devel wrote:
> > > Hi,
> > >
> > > On 2022/4/1 16:28, Chao Yu wrote:
> > > > On 2022/4/1 15:19, Yufen Yu via Linux-f2fs-devel wrote:
> > > > > Nowly, we can use new fault injection framework. Just delete the
> > > > > stale fault injection code.
> > > > >
> > > > > Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
> > > > > ---
> > > > >    fs/f2fs/checkpoint.c |  2 +-
> > > > >    fs/f2fs/f2fs.h       | 51 ++----------------------------------------
> > > > >    fs/f2fs/super.c      | 53 --------------------------------------------
> > > > >    fs/f2fs/sysfs.c      | 23 -------------------
> > > > >    4 files changed, 3 insertions(+), 126 deletions(-)
> > >
> > > ...
> > >
> > > > >                break;
> > > > > @@ -1963,14 +1920,6 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root)
> > > > >        if (F2FS_IO_SIZE_BITS(sbi))
> > > > >            seq_printf(seq, ",io_bits=%u",
> > > > >                    F2FS_OPTION(sbi).write_io_size_bits);
> > > > > -#ifdef CONFIG_F2FS_FAULT_INJECTION
> > > > > -    if (test_opt(sbi, FAULT_INJECTION)) {
> > > > > -        seq_printf(seq, ",fault_injection=%u",
> > > > > -                F2FS_OPTION(sbi).fault_info.inject_rate);
> > > > > -        seq_printf(seq, ",fault_type=%u",
> > > > > -                F2FS_OPTION(sbi).fault_info.inject_type);
> > > > > -    }
> > > > > -#endif
> > > >
> > > > This will cause regression due to it breaks application usage w/ -o
> > > > fault_* mountoption..., I don't think this is the right way.
> > >
> > >
> > > Thanks for catching this. I admit it's a problem. But, IMO fault_* mount
> > > option are mostly been used in test, not in actual product. So, I think
> > > it may just affect some test applications. With the common fault injection
> > > framework, it can be more easy and flexible to do fault injection test.
> > > Therefore, I want to remove the two mount options directly.
> > >
> > > If you really worried about compatibility, how about just reserving the
> > > two inject_* options but without doing any thing for them. We actually
> > > configure fault injections by debugfs in this patch.
> > >
> > > Or do you have more better suggestion?
> >
> > Could you please consider to keep original logic of f2fs fault injection
> > if user use inject_* options, otherwise following common fault injection
> > framework?
> >
> > Thoughts?
>
> I think it'd be useful to test roll-forward recovery flow by using those mount
> options, since runtime fault injection cannot enable it during mount.

Yeah, I have not catch this point before.

> BTW, what is the real benefit to use the fault injection framework?

I think fault injection framework can provide more easier and flexible
function than the current one. Furthermore, we can just following it and
don't need to maintain f2fs own fault injection cold.

Thanks,
Yufen