On 02/23, Chao Yu wrote:
> On 2017/2/23 5:15, Jaegeuk Kim wrote:
> > On 02/22, Chao Yu wrote:
> >> On 2017/1/6 10:42, Jaegeuk Kim wrote:
> >>> Hi Chao,
> >>>
> >>> On 01/06, Chao Yu wrote:
> >>>> On 2017/1/6 3:46, Jaegeuk Kim wrote:
> >>>>> On 01/05, Chao Yu wrote:
> >>>>>> On 2017/1/4 17:29, Chao Yu wrote:
> >>>>>>> On 2016/12/31 2:51, Jaegeuk Kim wrote:
> >>>>>>>> This patch relaxes async discard commands to avoid waiting its
> >>>>>>>> end_io during
> >>>>>>>> checkpoint.
> >>>>>>>> Instead of waiting them during checkpoint, it will be done when
> >>>>>>>> actually reusing
> >>>>>>>> them.
> >>>>>>>>
> >>>>>>>> Test on initial partition of nvme drive.
> >>>>>>>>
> >>>>>>>> # time fstrim /mnt/test
> >>>>>>>>
> >>>>>>>> Before : 6.158s
> >>>>>>>> After : 4.822s
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> >>>>>>>
> >>>>>>> Reviewed-by: Chao Yu <yuch...@huawei.com>
> >>>>>>>
> >>>>>>> One comment below,
> >>>>>>
> >>>>>> I still have a comment on this patch.
> >>>>>>
> >>>>>>>> -void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
> >>>>>>>> +/* This should be covered by global mutex, &sit_i->sentry_lock */
> >>>>>>>> +void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, unsigned int
> >>>>>>>> segno)
> >>>>>>>> {
> >>>>>>>> struct list_head *wait_list = &(SM_I(sbi)->wait_list);
> >>>>>>>> struct bio_entry *be, *tmp;
> >>>>>>>> @@ -646,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct
> >>>>>>>> f2fs_sb_info *sbi)
> >>>>>>>> struct bio *bio = be->bio;
> >>>>>>>> int err;
> >>>>>>>>
> >>>>>>>> - wait_for_completion_io(&be->event);
> >>>>>>>> + if (!completion_done(&be->event)) {
> >>>>>>>> + if ((be->start_segno >= segno &&
> >>>>>>>> + be->end_segno <= segno) ||
> >>>>>>>
> >>>>>>> segno >= be->start_segno && segno < be->end_segno ?
> >>>>
> >>>> Still can not understand this judgment condition, we should wait
> >>>> completion of
> >>>> discard command only when segno is locate in range of [start_segno,
> >>>> end_segno]?
> >>>>
> >>>> But now, this condition can be true only when segno, start_segno,
> >>>> end_segno have
> >>>> equal value.
> >>>>
> >>>> Please correct me if I'm wrong.
> >>>
> >>> Urg. I rewrote it to use block addresses.
> >>>
> >>> How about this?
> >>>
> >>> >From fe24461eedd62815e0c56317f010a3a6e3004434 Mon Sep 17 00:00:00 2001
> >>> From: Jaegeuk Kim <jaeg...@kernel.org>
> >>> Date: Thu, 29 Dec 2016 14:07:53 -0800
> >>> Subject: [PATCH] f2fs: relax async discard commands more
> >>>
> >>> This patch relaxes async discard commands to avoid waiting its end_io
> >>> during
> >>> checkpoint.
> >>> Instead of waiting them during checkpoint, it will be done when actually
> >>> reusing
> >>> them.
> >>>
> >>> Test on initial partition of nvme drive.
> >>>
> >>> # time fstrim /mnt/test
> >>>
> >>> Before : 6.158s
> >>> After : 4.822s
> >>
> >> We should wait for completion of all discard IOs in fstrim, since we can't
> >> guarantee discard command can be handled successfully by device after we
> >> return
> >> success to caller. Right?
> >
> > Do we have to guarantee? I think trim commands just try to give a hint to
> > flash
> > storage for its healthier status, since it doesn't hurt filesystem
> > consistency
> > and device/filesystem can freely ignore the commands as well.
>
> In manual of fstrim/BLKDISCARD, there is no explicit description that we
> should
> guarantee that discard ioctl being designed as synchronization, but trim
> interface in both block layer and most filesystems is designed as
> synchronization, so what about keep consistent with them now until there is
> requirement? then we can add a new async trim interface in ioctl.
Well, I don't think we need to follow this. The only problem happens from sudden
power-cut, but after boot-up, use can trigger another fstrim.
Thanks,
>
> > If we want to guarantee discarding region, we may need to issue
> > BLKSECDISCARD.
>
> Hmm.. I know that BLKSECDISCARD will trigger erasing operation in lower layer
> FTL, but now it is also being designed as synchronized interface.
>
> Thanks,
>
> >
> > Thanks,
> >
> >>
> >> Thanks,
> >>
> >>>
> >>> Reviewed-by: Chao Yu <yuch...@huawei.com>
> >>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> >>> ---
> >>> fs/f2fs/checkpoint.c | 7 ++-----
> >>> fs/f2fs/f2fs.h | 4 +++-
> >>> fs/f2fs/segment.c | 23 +++++++++++++++++++----
> >>> fs/f2fs/super.c | 3 +++
> >>> 4 files changed, 27 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index f73ee9534d83..1a9ba69a22ba 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -1254,7 +1254,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi,
> >>> struct cp_control *cpc)
> >>> f2fs_bug_on(sbi, prefree_segments(sbi));
> >>> flush_sit_entries(sbi, cpc);
> >>> clear_prefree_segments(sbi, cpc);
> >>> - f2fs_wait_all_discard_bio(sbi);
> >>> unblock_operations(sbi);
> >>> goto out;
> >>> }
> >>> @@ -1273,12 +1272,10 @@ int write_checkpoint(struct f2fs_sb_info *sbi,
> >>> struct cp_control *cpc)
> >>>
> >>> /* unlock all the fs_lock[] in do_checkpoint() */
> >>> err = do_checkpoint(sbi, cpc);
> >>> - if (err) {
> >>> + if (err)
> >>> release_discard_addrs(sbi);
> >>> - } else {
> >>> + else
> >>> clear_prefree_segments(sbi, cpc);
> >>> - f2fs_wait_all_discard_bio(sbi);
> >>> - }
> >>>
> >>> unblock_operations(sbi);
> >>> stat_inc_cp_count(sbi->stat_info);
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index bdcfe2a9b532..e0db895fd84c 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -183,6 +183,8 @@ struct discard_entry {
> >>>
> >>> struct bio_entry {
> >>> struct list_head list;
> >>> + block_t lstart;
> >>> + block_t len;
> >>> struct bio *bio;
> >>> struct completion event;
> >>> int error;
> >>> @@ -2111,7 +2113,7 @@ void destroy_flush_cmd_control(struct f2fs_sb_info
> >>> *, bool);
> >>> void invalidate_blocks(struct f2fs_sb_info *, block_t);
> >>> bool is_checkpointed_data(struct f2fs_sb_info *, block_t);
> >>> void refresh_sit_entry(struct f2fs_sb_info *, block_t, block_t);
> >>> -void f2fs_wait_all_discard_bio(struct f2fs_sb_info *);
> >>> +void f2fs_wait_discard_bio(struct f2fs_sb_info *, block_t);
> >>> void clear_prefree_segments(struct f2fs_sb_info *, struct cp_control *);
> >>> void release_discard_addrs(struct f2fs_sb_info *);
> >>> int npages_for_summary_flush(struct f2fs_sb_info *, bool);
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index 485b9118b418..672bcdefe836 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -625,20 +625,23 @@ static void locate_dirty_segment(struct
> >>> f2fs_sb_info *sbi, unsigned int segno)
> >>> }
> >>>
> >>> static struct bio_entry *__add_bio_entry(struct f2fs_sb_info *sbi,
> >>> - struct bio *bio)
> >>> + struct bio *bio, block_t lstart, block_t len)
> >>> {
> >>> struct list_head *wait_list = &(SM_I(sbi)->wait_list);
> >>> struct bio_entry *be = f2fs_kmem_cache_alloc(bio_entry_slab, GFP_NOFS);
> >>>
> >>> INIT_LIST_HEAD(&be->list);
> >>> be->bio = bio;
> >>> + be->lstart = lstart;
> >>> + be->len = len;
> >>> init_completion(&be->event);
> >>> list_add_tail(&be->list, wait_list);
> >>>
> >>> return be;
> >>> }
> >>>
> >>> -void f2fs_wait_all_discard_bio(struct f2fs_sb_info *sbi)
> >>> +/* This should be covered by global mutex, &sit_i->sentry_lock */
> >>> +void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> >>> {
> >>> struct list_head *wait_list = &(SM_I(sbi)->wait_list);
> >>> struct bio_entry *be, *tmp;
> >>> @@ -647,7 +650,15 @@ void f2fs_wait_all_discard_bio(struct f2fs_sb_info
> >>> *sbi)
> >>> struct bio *bio = be->bio;
> >>> int err;
> >>>
> >>> - wait_for_completion_io(&be->event);
> >>> + if (!completion_done(&be->event)) {
> >>> + if ((be->lstart <= blkaddr &&
> >>> + blkaddr < be->lstart + be->len) ||
> >>> + blkaddr == NULL_ADDR)
> >>> + wait_for_completion_io(&be->event);
> >>> + else
> >>> + continue;
> >>> + }
> >>> +
> >>> err = be->error;
> >>> if (err == -EOPNOTSUPP)
> >>> err = 0;
> >>> @@ -675,6 +686,7 @@ static int __f2fs_issue_discard_async(struct
> >>> f2fs_sb_info *sbi,
> >>> struct block_device *bdev, block_t blkstart, block_t blklen)
> >>> {
> >>> struct bio *bio = NULL;
> >>> + block_t lblkstart = blkstart;
> >>> int err;
> >>>
> >>> trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);
> >>> @@ -689,7 +701,8 @@ static int __f2fs_issue_discard_async(struct
> >>> f2fs_sb_info *sbi,
> >>> SECTOR_FROM_BLOCK(blklen),
> >>> GFP_NOFS, 0, &bio);
> >>> if (!err && bio) {
> >>> - struct bio_entry *be = __add_bio_entry(sbi, bio);
> >>> + struct bio_entry *be = __add_bio_entry(sbi, bio,
> >>> + lblkstart, blklen);
> >>>
> >>> bio->bi_private = be;
> >>> bio->bi_end_io = f2fs_submit_bio_wait_endio;
> >>> @@ -1575,6 +1588,8 @@ void allocate_data_block(struct f2fs_sb_info *sbi,
> >>> struct page *page,
> >>>
> >>> *new_blkaddr = NEXT_FREE_BLKADDR(sbi, curseg);
> >>>
> >>> + f2fs_wait_discard_bio(sbi, *new_blkaddr);
> >>> +
> >>> /*
> >>> * __add_sum_entry should be resided under the curseg_mutex
> >>> * because, this function updates a summary entry in the
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index f3697f97e527..461c29043aec 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -770,6 +770,9 @@ static void f2fs_put_super(struct super_block *sb)
> >>> write_checkpoint(sbi, &cpc);
> >>> }
> >>>
> >>> + /* be sure to wait for any on-going discard commands */
> >>> + f2fs_wait_discard_bio(sbi, NULL_ADDR);
> >>> +
> >>> /* write_checkpoint can update stat informaion */
> >>> f2fs_destroy_stats(sbi);
> >>>
> >>>
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Check out the vibrant tech community on one of the world's most
> >> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> linux-f2fs-de...@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> > .
> >
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> linux-f2fs-de...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
