Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1402891

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

Show all headers | View raw

On 2016/5/18 1:23, Jaegeuk Kim wrote:
> On Tue, May 17, 2016 at 11:00:53AM +0800, Hou Pengyang wrote:
>> 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
>
> Seems there is no problem.
>
> After f2fs_evict_inode(ino),
>
> f2fs_iget(ino)
>   - iget_failed()                          f2fs_create()
>                                             - f2fs_new_inode()
>                                             - ino = alloc_nid()
>                                             - insert_inode_locked()
>                                              *** spin_lock(&inode_hash_lock)
>                                              - spin_lock(&old->i_lock)
>                                              - __iget(old)
>    - make_bad_inode()                        - spin_unlock(&old->i_lock)
>     - remove_inode_hash()                    - spin_unlock(&inode_hash_lock)
>      - spin_lock(&inode_hash_lock)           - wait_on_inode(old)

oh.. wait_on_inode. OK, Kim, get it, thanks for your answer.

>      - spin_lock(&inode->i_lock)
>      - list_del
>      - spin_unlock(&inode->i_lock)
>      - spin_unlock(&inode_hash_lock)
>     - unlock_new_inode()
>      - wake_up_bit(&inode->i_state, __I_NEW) --> wake up!
>                                              - iput(old) whish was unhashed.
>                                              - goto to ***
>     - iput()
>
>> 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;
>
> No, we should do gc for data blocks owned by *evicted* inodes as well.
>
> Thanks,
>
>> 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 */
>>>>
>>>
>>> .
>>>
>>
>>
>>
>> ------------------------------------------------------------------------------
>> Mobile security can be enabling, not merely restricting. Employees who
>> bring their own devices (BYOD) to work are irked by the imposition of MDM
>> restrictions. Mobile Device Manager Plus allows you to control only the
>> apps on BYO-devices by containerizing them, leaving personal data untouched!
>> https://ad.doubleclick.net/ddm/clk/304595813;131938128;j
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[RFC] f2fs: fix a race condition between evict & gc Hou Pengyang <houpengyang@huawei.com> - 2016-05-16 12:50 +0200
  Re: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc Chao Yu <chao@kernel.org> - 2016-05-16 17:20 +0200
    Re: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc Hou Pengyang <houpengyang@huawei.com> - 2016-05-17 05:10 +0200
      Re: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc Jaegeuk Kim <jaegeuk@kernel.org> - 2016-05-17 19:30 +0200
        Re: [f2fs-dev] [RFC] f2fs: fix a race condition between evict & gc Hou Pengyang <houpengyang@huawei.com> - 2016-05-18 13:00 +0200

csiph-web