Messages in this thread | | | Subject | Re: [f2fs-dev] [PATCH] f2fs: fix ctx->pos in f2fs_read_inline_dir() | From | Chao Yu <> | Date | Sat, 17 Jul 2021 22:30:42 +0800 |
| |
On 2021/7/17 21:25, 李扬韬 wrote: > HI Chao, > > From: Chao Yu <chao@kernel.org> > Date: 2021-07-17 16:56:01 > To: Yangtao Li <frank.li@vivo.com>,jaegeuk@kernel.org > Cc: linux-kernel@vger.kernel.org,linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH] f2fs: fix ctx->pos in f2fs_read_inline_dir()>On 2021/7/17 11:49, 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. >> >> Nice catch! >> >>> >>> And returning early in f2fs_read_inline_dir will cause the process to be unable to see >>> the changes before reopening the file. >>> >>> So don't return early and remove the modification of ctx->pos in f2fs_read_inline_dir(). >>> >>> Signed-off-by: Yangtao Li <frank.li@vivo.com> >>> --- >>> fs/f2fs/inline.c | 5 ----- >>> 1 file changed, 5 deletions(-) >>> >>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c >>> index 56a20d5c15da..fc6551139a3d 100644 >>> --- a/fs/f2fs/inline.c >>> +++ b/fs/f2fs/inline.c >>> @@ -729,9 +729,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, >>> >>> make_dentry_ptr_inline(inode, &d, inline_dentry); >>> >>> - if (ctx->pos == d.max) >>> - return 0; >>> - >>> ipage = f2fs_get_node_page(F2FS_I_SB(inode), inode->i_ino); >>> if (IS_ERR(ipage)) >>> return PTR_ERR(ipage); >>> @@ -747,8 +744,6 @@ int f2fs_read_inline_dir(struct file *file, struct dir_context *ctx, >>> make_dentry_ptr_inline(inode, &d, inline_dentry); >>> >>> err = f2fs_fill_dentries(ctx, &d, 0, fstr); >> >> After this function, ctx->pos will point to start position of first free slot after >> last dir_entry, we can't guarantee that the free slot won't be used in above race >> condition, right? >> >> Moreover, w/o inline conversion, the race condition still can happen as below, right? >> >> dir_entry1: A >> dir_entry2: B >> dir_entry3: C >> free slot: _ >> >> Before: >> AAAABBBB___ >> ^ >> Thread B delete dir_entry2, and create dir_entry3. >> >> After: >> AAAACCCCC__ >> ^ > > Taking into account the above situations, I think this case where de->name_len is 0 in f2fs_fill_dentries() > should be normal and there is no way to avoid it. Maybe we shouldn't set fsck flag at this time? > Because the file system is not damaged.
Yangtao,
IMO, it should bypass tagging FSCK flag only if: a) bit_pos (:= ctx->pos % d->max) is non-zero & b) before bit_pos moves to first valid dir_entry.
Thanks,
> > MBR / Yangtao > >
| |