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