lkml.org 
[lkml]   [2016]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc
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 */
>>
>
> .
>


\
 
 \ /
  Last update: 2016-05-17 05:21    [W:0.066 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site