From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933372AbdKQFtZ (ORCPT ); Fri, 17 Nov 2017 00:49:25 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:11002 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933331AbdKQFtS (ORCPT ); Fri, 17 Nov 2017 00:49:18 -0500 Subject: Re: [PATCH] f2fs: let f2fs also gc atomic file to avoid loop gc To: Yunlong Song , , , CC: , , , , References: <1510108497-58331-1-git-send-email-yunlong.song@huawei.com> <239f770e-53f0-69c6-fc1d-67aeb9a1065a@huawei.com> <383339b7-528d-3085-1399-1f1694178c15@huawei.com> <415c8c26-498e-b3f6-2907-fc1ca8c63ad9@huawei.com> <05a5bbc0-c54d-8460-bf1a-678357778360@huawei.com> From: Chao Yu Message-ID: Date: Fri, 17 Nov 2017 13:47:57 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <05a5bbc0-c54d-8460-bf1a-678357778360@huawei.com> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.134.22.195] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020204.5A0E783E.009C,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 34f6a6254759286fff2958e9279a4266 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/11/17 11:30, Yunlong Song wrote: > How about add file_write_and_wait_range in __write_node_page as following: > if (atomic && !test_opt(sbi, NOBARRIER)) { > file_write_and_wait_range(file, 0, LLONG_MAX); Nope, GCed encrypted data wouldn't be cached in inode page cache. I don't think adding raw code to flush data here is a good implementation, it incurs complicated lock dependency relation. Instead, IMO, it will be better to use inmem_lock to avoid race in between GC and atomic commit flow. Thanks, > fio.op_flags |= REQ_PREFLUSH | REQ_FUA; > } > > The all the GCed data will be flushed to non-volatile before last node > write with REQ_PREFLUSH | REQ_FUA. > > On 2017/11/17 11:20, Chao Yu wrote: >> On 2017/11/17 11:04, Yunlong Song wrote: >>> The atomic commit will trigger: >>> -f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true) >>> -file_write_and_wait_range(file, 0, LLONG_MAX) >>> -fsync_node_pages >>> -__write_node_page >>> -REQ_PREFLUSH | REQ_FUA >>> >>> So data is flushed to non-volatile before last node write with > REQ_PREFLUSH | REQ_FUA, >> I mean GCed data. >> >> - file_write_and_wait_range >> - move_data_block >> - f2fs_submit_page_write >> - f2fs_update_data_blkaddr >> - set_page_dirty >> - fsync_node_pages >> >> Thanks, >> >>> we do not need to worry about the inconsistent problem. Right? >>> >>> On 2017/11/17 10:49, Chao Yu wrote: >>>> On 2017/11/17 8:58, Yunlong Song wrote: >>>>> Is there any problem if just deleting the judgement condition in this patch? >>>> IIRC, dirty node comes from data segment GC can be writebacked & flushed during >>>> atomic commit, but related data will still be in inner bio cache, after later >>>> SPOR, data would be inconsistent. >>>> >>>> Thanks, >>>> >>>>> On 2017/11/8 17:28, Chao Yu wrote: >>>>>> On 2017/11/8 10:34, Yunlong Song wrote: >>>>>>> If some files are opened with atomic flag and have not commited yet, at >>>>>>> the same time, if all the target victim segments have at least one page >>>>>>> of these atomic files, then f2fs gc will fail to do gc and hangs in the >>>>>>> process of go to gc_more, since gc_date_segment will not move any data >>>>>>> and get_valid_blocks will never be 0, then do_garbage_collect will >>>>>>> always return 0. >>>>>> Oh, I added this judgment condition to avoid ruining atomic write by data >>>>>> GC, could we find another way to solve this issue? BTW, if there is direct >>>>>> IO, we will also skip data segment GC. >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> Signed-off-by: Yunlong Song >>>>>>> --- >>>>>>> fs/f2fs/gc.c | 6 ------ >>>>>>> 1 file changed, 6 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>>>>> index 5d5bba4..3fdcd04 100644 >>>>>>> --- a/fs/f2fs/gc.c >>>>>>> +++ b/fs/f2fs/gc.c >>>>>>> @@ -621,9 +621,6 @@ static void move_data_block(struct inode *inode, block_t bidx, >>>>>>> if (!check_valid_map(F2FS_I_SB(inode), segno, off)) >>>>>>> goto out; >>>>>>> >>>>>>> - if (f2fs_is_atomic_file(inode)) >>>>>>> - goto out; >>>>>>> - >>>>>>> set_new_dnode(&dn, inode, NULL, NULL, 0); >>>>>>> err = get_dnode_of_data(&dn, bidx, LOOKUP_NODE); >>>>>>> if (err) >>>>>>> @@ -718,9 +715,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type, >>>>>>> if (!check_valid_map(F2FS_I_SB(inode), segno, off)) >>>>>>> goto out; >>>>>>> >>>>>>> - if (f2fs_is_atomic_file(inode)) >>>>>>> - goto out; >>>>>>> - >>>>>>> if (gc_type == BG_GC) { >>>>>>> if (PageWriteback(page)) >>>>>>> goto out; >>>>>>> >>>>>> . >>>>>> >>>> . >>>> >> >> . >> >