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. > > 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 > >>>>>>>> > >>>>>>>> . > >>>>>>>> > >>>>>>>
