| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170222211539.GA4003@jaegeuk.local>
Date: Wed, 22 Feb 2017 13:15:39 -0800
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Chao Yu <yuchao0@...wei.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more
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 <jaegeuk@...nel.org>
> >>>>>
> >>>>> Reviewed-by: Chao Yu <yuchao0@...wei.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 <jaegeuk@...nel.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.
If we want to guarantee discarding region, we may need to issue BLKSECDISCARD.
Thanks,
>
> Thanks,
>
> >
> > Reviewed-by: Chao Yu <yuchao0@...wei.com>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.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-devel@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Powered by blists - more mailing lists