On 01/31, Chao Yu wrote: > On 2018/1/31 10:59, Jaegeuk Kim wrote: > > On 01/31, Gaoxiang (OS) wrote: > >> > >> > >> On 2018/1/31 10:52, Jaegeuk Kim wrote: > >>> On 01/31, Gaoxiang (OS) wrote: > >>>> Hi Jaegeuk, > >>>> > >>>> On 2018/1/31 10:39, Jaegeuk Kim wrote: > >>>>> On 01/31, Gaoxiang (OS) wrote: > >>>>>> Hi Jaegeuk, > >>>>>> > >>>>>> On 2018/1/31 10:18, Jaegeuk Kim wrote: > >>>>>>> On 01/26, Gaoxiang (OS) wrote: > >>>>>>>> Hi Jaegeuk and Chao, > >>>>>>>> > >>>>>>>> On 2018/1/26 9:36, Chao Yu wrote: > >>>>>>>>> On 2018/1/26 6:06, Jaegeuk Kim wrote: > >>>>>>>>>> > >>>>>>>>>> Then, we don't need to wait for this as well as wait_on_all_pages_writeback() > >>>>>>>>>> in the early stage in do_checkpoint()? > >>>>>>>>>> > >>>>>>>>>> So, it seems like we can modify like below: > >>>>>>>>>> > >>>>>>>>>> --- > >>>>>>>>>> 1. while (get_pages()) > >>>>>>>>>> sync_meta_pages() > >>>>>>>>>> 2. if (enabled_nat_bits()) > >>>>>>>>>> while (get_pages()) > >>>>>>>>>> sync_meta_pages() > >>>>>>>>>> > >>>>>>>>>> 3. wait_on_all_pages_writeback() > >>>>>>>>>> -> remove > >>>>>>>>> > >>>>>>>>> Would meta area across two devices? if it would, we need to wait all meta > >>>>>>>>> be persisted in second device before f2fs_flush_device_cache? > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> 4. f2fs_flush_device_cache() > >>>>>>> > >>>>>>> -> remove > >>>>>>> > >>>>>>>>>> > >>>>>>>>>> 5. update_meta_page() <- for first cp_block > >>>>>>>>>> > >>>>>>>>>> 6. update_meta_page()... <- payload > >>>>>>>>>> > >>>>>>>>>> 7. orphan writes > >>>>>>>>>> > >>>>>>>>>> 8. node_summary writes > >>>>>>>>>> > >>>>>>>>>> 9. update_meta_page() <- for last cp_block > >>>>>>>>>> -> remove > >>>>>>>>> > >>>>>>>> > >>>>>>>> - /* writeout checkpoint block */ > >>>>>>>> - update_meta_page(sbi, ckpt, start_blk); > >>>>>>>> - > >>>>>>>> - /* wait for previous submitted node/meta pages writeback */ > >>>>>>>> - wait_on_all_pages_writeback(sbi); > >>>>>>>> - > >>>>>>>> - if (unlikely(f2fs_cp_error(sbi))) > >>>>>>>> - return -EIO; > >>>>>>>> - > >>>>>>>> Could also be removed, too? > >>>>>>>> > >>>>>>>> filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX); > >>>>>>>> filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX); > >>>>>>> > >>>>>>> -> remove > >>>>>>> > >>>>>>> Hmm, think so. > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>>> 9.1 sync_meta_pages(META) to make sure all meta IOs are issued. > >>>>>>>>> > >>>>>>>> > >>>>>>>> If I understand correctly, I have the same questions with Chao. > >>>>>>>> It seems that META doesn't have another flush mechanism (eg. flush > >>>>>>>> thread) other than sync_meta_pages? > >>>>>>> > >>>>>>> 9.2 f2fs_flush_device_cache(), if we have multiple devices. > >>>>>>> > >>>>>>>> > >>>>>>>>>> > >>>>>>>>>> 10. wait_on_all_pages_writeback() > >>>>>>> > >>>>>>> 10.1. (f2fs_cp_error()) > >>>>>>> return -EIO; > >>>>>>> > >>>>>>>>>> > >>>>>>>>>> ---- > >>>>>>>>>> Add) 11. commit_checkpoint() > >>>>>>>>>> - update_meta_page() <- for last cp_block > >>>>>>>>>> - sync_meta_pages(META_FLUSH) > >>>>>>>>>> > >>>>>>>>>> We don't need to wait for page_writeback any more. > >>>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> Apart from that, I think we should "wait_on_all_pages_writeback(sbi);" > >>>>>>>> after META_FLUSH in case for pulluting the next checkpoint when the last > >>>>>>>> cp block is failed to write with FUA? > >>>>>>> > >>>>>>> Next cp block won't be written by 10.1. > >>>>>>> > >>>>>> > >>>>>> I think that 10.1 ensures the cp pack before the last cp_block was done. > >>>>>> However, what if the last cp block writes fail later without FUA? > >>>>> > >>>>> Without FUA? The last cp_block is written by FUA, no? > >>>> > >>>> quote " > >>>> Apart from that, I think we should "wait_on_all_pages_writeback(sbi);" > >>>> after META_FLUSH in case for pulluting the next checkpoint when the last > >>>> cp block is failed to write with FUA? > >>>> " > >>>> > >>>> what I meant is that the last cp_block should be written by FUA. > >>>> we need to use META_FLUSH to write last cp_block, right? :) > >>> > >>> Add) 11. commit_checkpoint() > >>> - update_meta_page() <- for last cp_block > >>> - sync_meta_pages(META_FLUSH) > >>> > >>> What do you mean? I added META_FLUSH. > >>> > >>> 9.1 sync_meta_pages(META); > >>> -> 10. wait_on_all_pages_writeback(); > >>> -> 11. sync_meta_pages(META_FLUSH); > >>> > >>> -> 9.1 sync_meta_pages(META); > >>> -> 10. wait_on_all_pages_writeback(); > >>> -> 10.1 f2fs_cp_error() -> return -EIO; > >>> > >> > >> What I mean is > >> should we need to ensure FUA writing to medium (using the last > >> "wait_on_all_pages_writeback(sbi)") and then unblock_operation > >> ,quit write_checkpoint, go on fs operations > >> > >> 11. commit_checkpoint() > >> - update_meta_page() <- for last cp_block > >> - sync_meta_pages(META_FLUSH) > >> *12. wait_on_all_pages_writeback() * > > > > I'm saying we don't need this. > > I think we need this, because w/o this end_io can be called after > f2fs_cp_error, then we can not be aware of result of last IO in > current checkpoint. Sigh, why do we have to get this error? The next checkpoint won't be succeeded. > > Thanks, > > > > >> > >> Sorry about my expression is not clear. > >> > >> Thanks, > >> > >>>> > >>>> Thanks, > >>>> > >>>>> > >>>>>> Should we need to ensure the last cp block going into device medium > >>>>>> rather than device cache before switching to go into the next checkpoint > >>>>>> (I mean we need to ensure writing to medium and then unblock_operation > >>>>>> and quit write_checkpoint and go on fs operations)? > >>>>>> > >>>>>> Other parts seems OK for me :), I will sort out and resent a new patch. > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> Thanks all, > >>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> @@ -1313,10 +1337,15 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc) > >>>>>>>>>>> sbi->last_valid_block_count = sbi->total_valid_block_count; > >>>>>>>>>>> percpu_counter_set(&sbi->alloc_valid_block_count, 0); > >>>>>>>>>>> > >>>>>>>>>>> - /* Here, we only have one bio having CP pack */ > >>>>>>>>>>> - sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO); > >>>>>>>>>>> + /* Here, we have one bio having CP pack except cp pack 2 page */ > >>>>>>>>>>> + sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO); > >>>>>>>>>>> > >>>>>>>>>>> /* wait for previous submitted meta pages writeback */ > >>>>>>>>>>> + if (!test_opt(sbi, NOBARRIER)) > >>>>>>>>>> > >>>>>>>>>> The above has nothing to do with this patch. > >>>>>>>>> > >>>>>>>>> We only need to use wait_on_all_pages_writeback to keep writeback order of > >>>>>>>>> previous metadata and last cp pack metadata if barrier is on? > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> + wait_on_all_pages_writeback(sbi); > >>>>>>>>>>> + > >>>>>>>>>>> + /* barrier and flush checkpoint cp pack 2 page */ > >>>>>>>>>>> + commit_checkpoint(sbi, ckpt, start_blk); > >>>>>>>>>>> wait_on_all_pages_writeback(sbi); > >>>>>>>>>>> > >>>>>>>>>>> release_ino_entry(sbi, false); > >>>>>>>>>>> -- > >>>>>>>>>>> 2.1.4 > >>>>>>>>>> > >>>>>>>>>> . > >>>>>>>>>> > >>>>>>>>> > > > > . > >
