Re: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc

[Hou Pengyang]

On 2016/5/16 23:10, Chao Yu wrote:
Hi chao,
> Hi Pengyang,
>
> On 2016/5/16 18:40, Hou Pengyang wrote:
> > When collecting data segment(gc_data_segment), there is a race condition
> > between evict and phases of gc:
> > 0) ra_node_page(dnode)
> > 1) ra_node_page(inode)
> > 		<--- evict the inode
> > 2) f2fs_iget get the inode and add it to gc_list
> > 3) move_data_page
> >
> > In step 2), f2fs_iget does NOT find the inode and allocs a new inode as result,
>
> If inode was unlinked and then be evicted, f2fs_iget should fail when reading
> inode's page as blkaddr of this node is null.
agree, after do_read_inode fail, the newly created inode would be
freed as a bad inode and f2fs_iget fails. But this may lead to create
file fail:
gc:iget_locked
       <---- touch/mkdir(reuse the evicted ino)
gc:free the bad inode

during the bad inode allocated and freed in gc, the inode is reserved
in the global inode_hash, while the ino is a free nid in free_nid tree.

touch/mkdir may reuse the ino, during the touch/mkdir path, the global
inode_hash would be checked if the ino file exists. Under this
scenario, because of the gc bad inode in inode_hash, touch/mkdir would
fail.

ilookup seems better, as no need to alloc and free a bad inode.

if ilookup fails, that exactly means inode has been evicted and no need
to gc;
if ilookup success, before phase 3, is_alive to deal with the ino reuse
scenario;

Do I miss anything else?
thanks
> If inode still have non-zero nlink value and then be evicted, we should allow gc
> thread to reference this inode for moving its data pages.
>
> Thanks,
>
> > which is not resonable.
> >
> > This patch changes f2fs_iget to ilookup. when no inode is found, no new inode is
> > created.
> >
> > Signed-off-by: Hou Pengyang <houpengyang@xxxxxxxxxx>
> > ---
> >   fs/f2fs/gc.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 38d56f6..6e73193 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -717,8 +717,8 @@ next_step:
> >   		ofs_in_node = le16_to_cpu(entry->ofs_in_node);
> >
> >   		if (phase == 2) {
> > -			inode = f2fs_iget(sb, dni.ino);
> > -			if (IS_ERR(inode) || is_bad_inode(inode))
> > +			inode = ilookup(sb, dni.ino);
> > +			if (!inode || IS_ERR(inode) || is_bad_inode(inode))
> >   				continue;
> >
> >   			/* if encrypted inode, let's go phase 3 */
>
> .