Messages in this thread Patch in this message | | | Date | Tue, 17 May 2016 11:00:53 +0800 | From | Hou Pengyang <> | Subject | Re: [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 */ >> > > . >
| |