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

[Hou Pengyang <houpengyang@huawei.com>]

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@huawei.com>
>> ---
>>   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 */
>>
>
> .
>