Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550575 > unrolled thread

Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more

Started byChao Yu <yuchao0@huawei.com>
First post2017-01-04 10:40 +0100
Last post2017-01-06 04:40 +0100
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Chao Yu <yuchao0@huawei.com> - 2017-01-04 10:40 +0100
    Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Chao Yu <yuchao0@huawei.com> - 2017-01-05 04:30 +0100
      Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Chao Yu <yuchao0@huawei.com> - 2017-01-05 10:00 +0100
        Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Jaegeuk Kim <jaegeuk@kernel.org> - 2017-01-05 21:10 +0100
      Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Jaegeuk Kim <jaegeuk@kernel.org> - 2017-01-05 20:50 +0100
        Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Chao Yu <yuchao0@huawei.com> - 2017-01-06 03:10 +0100
          Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Jaegeuk Kim <jaegeuk@kernel.org> - 2017-01-06 03:50 +0100
            Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more Chao Yu <yuchao0@huawei.com> - 2017-01-06 04:40 +0100

#1550575 — Re: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more

FromChao Yu <yuchao0@huawei.com>
Date2017-01-04 10:40 +0100
SubjectRe: [f2fs-dev] [PATCH 08/10] f2fs: relax async discard commands more
Message-ID<sVQd3-2M8-23@gated-at.bofh.it>
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@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

One comment below,

> ---
>  fs/f2fs/checkpoint.c |  3 ---
>  fs/f2fs/f2fs.h       |  4 +++-
>  fs/f2fs/segment.c    | 25 +++++++++++++++++++++----
>  fs/f2fs/super.c      |  3 +++
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 34bfe2b494ae..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;
>  	}
> @@ -1278,8 +1277,6 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	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..f2f40fce9d31 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;
> +	unsigned int start_segno;
> +	unsigned int end_segno;
>  	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 *, unsigned int);
>  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 6d197f0c8151..9a38424c3c1f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -624,20 +624,24 @@ 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, unsigned int start_segno,
> +					unsigned int end_segno)
>  {
>  	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->start_segno = start_segno;
> +	be->end_segno = end_segno;
>  	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, 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 ?

Thanks,

> +					segno == NULL_SEGNO)
> +				wait_for_completion_io(&be->event);
> +			else
> +				continue;
> +		}
> +
>  		err = be->error;
>  		if (err == -EOPNOTSUPP)
>  			err = 0;
> @@ -674,6 +686,8 @@ 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;
> +	unsigned int start_segno = GET_SEGNO(sbi, blkstart);
> +	unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen);
>  	int err;
>  
>  	trace_f2fs_issue_discard(sbi->sb, blkstart, blklen);
> @@ -688,7 +702,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,
> +					start_segno, end_segno);
>  
>  		bio->bi_private = be;
>  		bio->bi_end_io = f2fs_submit_bio_wait_endio;
> @@ -1574,6 +1589,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, GET_SEGNO(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..16e2bc5209bb 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_SEGNO);
> +
>  	/* write_checkpoint can update stat informaion */
>  	f2fs_destroy_stats(sbi);
>  
>

[toc] | [next] | [standalone]

#1551593

FromChao Yu <yuchao0@huawei.com>
Date2017-01-05 04:30 +0100
Message-ID<sW6Uy-5xP-23@gated-at.bofh.it>
In reply to#1550575
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@kernel.org>
> 
> Reviewed-by: Chao Yu <yuchao0@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 ?

Can you check this?

Thanks,

[toc] | [prev] | [next] | [standalone]

#1551770

FromChao Yu <yuchao0@huawei.com>
Date2017-01-05 10:00 +0100
Message-ID<sWc3U-pg-29@gated-at.bofh.it>
In reply to#1551593
Hi Jaegeuk,

I can see patch named ("f2fs: call f2fs_wait_all_discard_bio for an error case")
was merged in dev-test, but I think it's no needed to change error case handling
like this since f2fs_wait_all_discard_bio should always be called after
clear_prefree_segments.

Thanks,

On 2017/1/5 11:19, 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@kernel.org>
>>
>> Reviewed-by: Chao Yu <yuchao0@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 ?
> 
> Can you check this?
> 
> Thanks,
>

[toc] | [prev] | [next] | [standalone]

#1552293

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2017-01-05 21:10 +0100
Message-ID<sWmwi-7Lg-33@gated-at.bofh.it>
In reply to#1551770
Hi Chao,

On 01/05, Chao Yu wrote:
> Hi Jaegeuk,
> 
> I can see patch named ("f2fs: call f2fs_wait_all_discard_bio for an error case")
> was merged in dev-test, but I think it's no needed to change error case handling
> like this since f2fs_wait_all_discard_bio should always be called after
> clear_prefree_segments.

Indeed, it's right. ;)

Thanks,

> 
> Thanks,
> 
> On 2017/1/5 11:19, 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@kernel.org>
> >>
> >> Reviewed-by: Chao Yu <yuchao0@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 ?
> > 
> > Can you check this?
> > 
> > Thanks,
> >

[toc] | [prev] | [next] | [standalone]

#1552287

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2017-01-05 20:50 +0100
Message-ID<sWmcW-7os-23@gated-at.bofh.it>
In reply to#1551593
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@kernel.org>
> > 
> > Reviewed-by: Chao Yu <yuchao0@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 ?
> 
> Can you check this?

The be->start_segno and be->end_segno are assigned by:

unsigned int start_segno = GET_SEGNO(sbi, blkstart);
unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen);

in __f2fs_issue_discard_async().

So, the problem comes when, for example, blkstart = 0 and blklen = 512.

I should have got the end_segno by:

unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen - 1);

Thanks,

[toc] | [prev] | [next] | [standalone]

#1552472

FromChao Yu <yuchao0@huawei.com>
Date2017-01-06 03:10 +0100
Message-ID<sWs8G-3cr-9@gated-at.bofh.it>
In reply to#1552287
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@kernel.org>
>>>
>>> Reviewed-by: Chao Yu <yuchao0@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.

Thanks,

>>
>> Can you check this?
> 
> The be->start_segno and be->end_segno are assigned by:
> 
> unsigned int start_segno = GET_SEGNO(sbi, blkstart);
> unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen);
> 
> in __f2fs_issue_discard_async().
> 
> So, the problem comes when, for example, blkstart = 0 and blklen = 512.
> 
> I should have got the end_segno by:
> 
> unsigned int end_segno = GET_SEGNO(sbi, blkstart + blklen - 1);
> 
> Thanks,
> 
> .
>

[toc] | [prev] | [next] | [standalone]

#1552484

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2017-01-06 03:50 +0100
Message-ID<sWsLn-3qX-23@gated-at.bofh.it>
In reply to#1552472
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@kernel.org>
> >>>
> >>> Reviewed-by: Chao Yu <yuchao0@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 <jaegeuk@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

Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@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);
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1552498

FromChao Yu <yuchao0@huawei.com>
Date2017-01-06 04:40 +0100
Message-ID<sWtxM-45k-5@gated-at.bofh.it>
In reply to#1552484
Hi Jaegeuk,

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@kernel.org>
>>>>>
>>>>> Reviewed-by: Chao Yu <yuchao0@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?

Looks good to me. Nice work!

Thanks,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web