Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] f2fs: Reduce the scope of setting fsck tag when de->name_len is zero | From | Chao Yu <> | Date | Tue, 20 Jul 2021 12:08:53 +0800 |
| |
On 2021/7/19 18:14, Yangtao Li wrote: > I recently found a case where de->name_len is 0 in f2fs_fill_dentries() easily reproduced, > and finally set the fsck flag. > > Thread A Thread B > > f2fs_readdir > f2fs_read_inline_dir > ctx->pos = d.max > f2fs_add_dentry > f2fs_add_inline_entry > do_convert_inline_dir > f2fs_add_regular_entry > f2fs_readdir > f2fs_fill_dentries > set_sbi_flag(sbi, SBI_NEED_FSCK) > > Process A opens the folder, and has been reading without closing it. During this period, > Process B created a file under the folder (occupying multiple f2fs_dir_entry, exceeding > the d.max of the inline dir). After creation, process A uses the d.max of inline dir to > read it again, and it will read that de->name_len is 0. > > And Chao pointed out that w/o inline conversion, the race condition still can happen as below > > dir_entry1: A > dir_entry2: B > dir_entry3: C > free slot: _
ctx->pos: ^
> > Before: > AAAABBBB___ > ^
AAAABBBB___ ^
> Thread B delete dir_entry2, and create dir_entry3. > > After: > AAAACCCCC__ > ^
AAAACCCCC__ ^
> > In these scenarios, the file system is not damaged, and it's hard to avoid it. But we can bypass > tagging FSCK flag if: > a) bit_pos (:= ctx->pos % d->max) is non-zero & b) before bit_pos moves to first > valid dir_entry. > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/f2fs/dir.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 456651682daf..3e2a61a3600c 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -1000,6 +1000,7 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d, > struct f2fs_sb_info *sbi = F2FS_I_SB(d->inode); > struct blk_plug plug; > bool readdir_ra = sbi->readdir_ra == 1; > + bool find_valid_dentry = false;
found_valid_dirent?
> int err = 0; > > bit_pos = ((unsigned long)ctx->pos % d->max); > @@ -1014,13 +1015,15 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d, > > de = &d->dentry[bit_pos]; > if (de->name_len == 0) { > + if (!(ctx->pos % d->max != 0 && find_valid_dentry == false)) {
if (found_valid_dirent || !bit_pos)) will be more clean?
> + printk_ratelimited( > + "%sF2FS-fs (%s): invalid namelen(0), ino:%u, run fsck to fix.", > + KERN_WARNING, sbi->sb->s_id, > + le32_to_cpu(de->ino)); > + set_sbi_flag(sbi, SBI_NEED_FSCK); > + } > bit_pos++; > ctx->pos = start_pos + bit_pos; > - printk_ratelimited( > - "%sF2FS-fs (%s): invalid namelen(0), ino:%u, run fsck to fix.", > - KERN_WARNING, sbi->sb->s_id, > - le32_to_cpu(de->ino)); > - set_sbi_flag(sbi, SBI_NEED_FSCK); > continue; > } > > @@ -1063,6 +1066,7 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d, > f2fs_ra_node_page(sbi, le32_to_cpu(de->ino)); > > ctx->pos = start_pos + bit_pos; > + find_valid_dentry = true;
found_valid_dirent = true;
Thanks,
> } > out: > if (readdir_ra) >
| |