linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix small discards when se->valid_blocks is zero
@ 2017-01-03  9:01 Yunlong Song
  2017-01-04  1:54 ` Chao Yu
  2017-01-04  1:55 ` Jaegeuk Kim
  0 siblings, 2 replies; 6+ messages in thread
From: Yunlong Song @ 2017-01-03  9:01 UTC (permalink / raw)
  To: jaegeuk, cm224.lee, yuchao0, chao, sylinux, yunlong.song
  Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

In the small discard case, when se->valid_blocks is zero, the add_discard_addrs
will directly return without __add_discard_entry. This will cause the file
delete have no small discard. The case is like this:

1. Allocate free 2M segment
2. Write a file (size n blocks < 512) in that 2M segment, se->valid_blocks = n
3. Delete that file, se->valid_blocks = 0, add_discard_addrs will return without
sending any discard of that file, and forever due to cur_map[i] ^ ckpt_map[i] =
0 after that checkpoint

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 0738f48..8610f14 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -838,7 +838,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		return;
 
 	if (!force) {
-		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
+		if (!test_opt(sbi, DISCARD) ||
 		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
 			return;
 	}
-- 
1.8.5.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: fix small discards when se->valid_blocks is zero
  2017-01-03  9:01 [PATCH] f2fs: fix small discards when se->valid_blocks is zero Yunlong Song
@ 2017-01-04  1:54 ` Chao Yu
  2017-01-04  1:55 ` Jaegeuk Kim
  1 sibling, 0 replies; 6+ messages in thread
From: Chao Yu @ 2017-01-04  1:54 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, cm224.lee, chao, sylinux
  Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

On 2017/1/3 17:01, Yunlong Song wrote:
> In the small discard case, when se->valid_blocks is zero, the add_discard_addrs
> will directly return without __add_discard_entry. This will cause the file
> delete have no small discard. The case is like this:
> 
> 1. Allocate free 2M segment
> 2. Write a file (size n blocks < 512) in that 2M segment, se->valid_blocks = n
> 3. Delete that file, se->valid_blocks = 0, add_discard_addrs will return without
> sending any discard of that file, and forever due to cur_map[i] ^ ckpt_map[i] =
> 0 after that checkpoint

Wouldn't it be discarded as prefree segment?

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 0738f48..8610f14 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -838,7 +838,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		return;
>  
>  	if (!force) {
> -		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
> +		if (!test_opt(sbi, DISCARD) ||
>  		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
>  			return;
>  	}
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: fix small discards when se->valid_blocks is zero
  2017-01-03  9:01 [PATCH] f2fs: fix small discards when se->valid_blocks is zero Yunlong Song
  2017-01-04  1:54 ` Chao Yu
@ 2017-01-04  1:55 ` Jaegeuk Kim
  2017-01-04  3:59   ` Yunlong Song
  1 sibling, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2017-01-04  1:55 UTC (permalink / raw)
  To: Yunlong Song
  Cc: cm224.lee, yuchao0, chao, sylinux, bintian.wang, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Hi Yunlong,

On 01/03, Yunlong Song wrote:
> In the small discard case, when se->valid_blocks is zero, the add_discard_addrs
> will directly return without __add_discard_entry. This will cause the file
> delete have no small discard. The case is like this:
> 
> 1. Allocate free 2M segment
> 2. Write a file (size n blocks < 512) in that 2M segment, se->valid_blocks = n
> 3. Delete that file, se->valid_blocks = 0, add_discard_addrs will return without
> sending any discard of that file, and forever due to cur_map[i] ^ ckpt_map[i] =
> 0 after that checkpoint

During this checkpoint, that'll be discarded as a prefree segment, no?
Note that, if this is a current segment, f2fs won't discard it until it is
fully allocated.

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 0738f48..8610f14 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -838,7 +838,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		return;
>  
>  	if (!force) {
> -		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
> +		if (!test_opt(sbi, DISCARD) ||
>  		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
>  			return;
>  	}
> -- 
> 1.8.5.2

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: fix small discards when se->valid_blocks is zero
  2017-01-04  1:55 ` Jaegeuk Kim
@ 2017-01-04  3:59   ` Yunlong Song
  2017-01-04 22:36     ` Jaegeuk Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Yunlong Song @ 2017-01-04  3:59 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: cm224.lee, yuchao0, chao, sylinux, bintian.wang, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Hi Kim,
    Although the blocks of that file will finally be discarded when it is not current segment any more and almost fully invalidate,
but the point is that the blocks of that file can only be discarded along with the whole segment now, which violates the meaning
of small discard. Look at the case I said in last mail, if the segment which owns the deleted file has no more changing after the file
deleting, and its validate blocks are perhaps over 95%, and it may not be easy to be selected as a gc victim. In this case, FTL can
not know the "file delete" on time, and the invalidate blocks of that file can not be discarded in FTL layer on time.

On 2017/1/4 9:55, Jaegeuk Kim wrote:
> Hi Yunlong,
>
> On 01/03, Yunlong Song wrote:
>> In the small discard case, when se->valid_blocks is zero, the add_discard_addrs
>> will directly return without __add_discard_entry. This will cause the file
>> delete have no small discard. The case is like this:
>>
>> 1. Allocate free 2M segment
>> 2. Write a file (size n blocks < 512) in that 2M segment, se->valid_blocks = n
>> 3. Delete that file, se->valid_blocks = 0, add_discard_addrs will return without
>> sending any discard of that file, and forever due to cur_map[i] ^ ckpt_map[i] =
>> 0 after that checkpoint
> During this checkpoint, that'll be discarded as a prefree segment, no?
> Note that, if this is a current segment, f2fs won't discard it until it is
> fully allocated.
>
> Thanks,
>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>  fs/f2fs/segment.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 0738f48..8610f14 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -838,7 +838,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>  		return;
>>  
>>  	if (!force) {
>> -		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
>> +		if (!test_opt(sbi, DISCARD) ||
>>  		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
>>  			return;
>>  	}
>> -- 
>> 1.8.5.2
> .
>


-- 
Thanks,
Yunlong Song

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: fix small discards when se->valid_blocks is zero
  2017-01-04  3:59   ` Yunlong Song
@ 2017-01-04 22:36     ` Jaegeuk Kim
  2017-01-05  2:38       ` Yunlong Song
  0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2017-01-04 22:36 UTC (permalink / raw)
  To: Yunlong Song
  Cc: cm224.lee, yuchao0, chao, sylinux, bintian.wang, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

On 01/04, Yunlong Song wrote:
> Hi Kim,
>     Although the blocks of that file will finally be discarded when it is not current segment any more and almost fully invalidate,
> but the point is that the blocks of that file can only be discarded along with the whole segment now, which violates the meaning
> of small discard. Look at the case I said in last mail, if the segment which owns the deleted file has no more changing after the file
> deleting, and its validate blocks are perhaps over 95%, and it may not be easy to be selected as a gc victim. In this case, FTL can
> not know the "file delete" on time, and the invalidate blocks of that file can not be discarded in FTL layer on time.

Correction: current active segment is also treated as a candidate for small
discards in add_discard_addrs().

So, it seems you want to discard small invalid chunks in the current active
segments, right? If so, how do you think this change?

---
 fs/f2fs/segment.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 82078734f379..394a6fef7f82 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -853,11 +853,10 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
 	if (se->valid_blocks == max_blocks || !f2fs_discard_en(sbi))
 		return false;
 
-	if (!force) {
-		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
-		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
-			return false;
-	}
+	if (!force && (!test_opt(sbi, DISCARD) ||
+		(!se->valid_blocks && !IS_CURSEG(sbi, cpc->trim_start)) ||
+		SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards))
+		return false;
 
 	/* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
 	for (i = 0; i < entries; i++)
-- 
2.11.0


> 
> On 2017/1/4 9:55, Jaegeuk Kim wrote:
> > Hi Yunlong,
> >
> > On 01/03, Yunlong Song wrote:
> >> In the small discard case, when se->valid_blocks is zero, the add_discard_addrs
> >> will directly return without __add_discard_entry. This will cause the file
> >> delete have no small discard. The case is like this:
> >>
> >> 1. Allocate free 2M segment
> >> 2. Write a file (size n blocks < 512) in that 2M segment, se->valid_blocks = n
> >> 3. Delete that file, se->valid_blocks = 0, add_discard_addrs will return without
> >> sending any discard of that file, and forever due to cur_map[i] ^ ckpt_map[i] =
> >> 0 after that checkpoint
> > During this checkpoint, that'll be discarded as a prefree segment, no?
> > Note that, if this is a current segment, f2fs won't discard it until it is
> > fully allocated.
> >
> > Thanks,
> >
> >> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> >> ---
> >>  fs/f2fs/segment.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >> index 0738f48..8610f14 100644
> >> --- a/fs/f2fs/segment.c
> >> +++ b/fs/f2fs/segment.c
> >> @@ -838,7 +838,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>  		return;
> >>  
> >>  	if (!force) {
> >> -		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
> >> +		if (!test_opt(sbi, DISCARD) ||
> >>  		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
> >>  			return;
> >>  	}
> >> -- 
> >> 1.8.5.2
> > .
> >
> 
> 
> -- 
> Thanks,
> Yunlong Song
> 

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: fix small discards when se->valid_blocks is zero
  2017-01-04 22:36     ` Jaegeuk Kim
@ 2017-01-05  2:38       ` Yunlong Song
  0 siblings, 0 replies; 6+ messages in thread
From: Yunlong Song @ 2017-01-05  2:38 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: cm224.lee, yuchao0, chao, sylinux, bintian.wang, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Hi Kim,
    I am afraid your fix still has some problems when the section/segment != 1, when the segment has zero valid blocks but the section does not,
the blocks of that deleted file can still not notify the FTL by the small discards in your fix. And even in the case section/segment == 1, the blocks
of that deleted file can only be discarded in a range of single 2M rather than its real blocks range, which disturbs the semantic of "small discards".
    By the way, there is still a problem of "double discard". When the small discard option is on, the blocks of the deleted file will send small discard
via add_discard_addrs, and also will send the section-level discard when the whole section is free later. I will think about to avoid the section-level
discard, then the small discard of that deleted file is needed although "!se->valid_blocks && !IS_CURSEG(sbi, cpc->trim_start)"  is true, since the other
blocks of that segment has already sent small discard when they got invalid before, and we just need to send the discard of that deleted file which
is the last one taking up in the segment.

On 2017/1/5 6:36, Jaegeuk Kim wrote:
> On 01/04, Yunlong Song wrote:
>> Hi Kim,
>>     Although the blocks of that file will finally be discarded when it is not current segment any more and almost fully invalidate,
>> but the point is that the blocks of that file can only be discarded along with the whole segment now, which violates the meaning
>> of small discard. Look at the case I said in last mail, if the segment which owns the deleted file has no more changing after the file
>> deleting, and its validate blocks are perhaps over 95%, and it may not be easy to be selected as a gc victim. In this case, FTL can
>> not know the "file delete" on time, and the invalidate blocks of that file can not be discarded in FTL layer on time.
> Correction: current active segment is also treated as a candidate for small
> discards in add_discard_addrs().
>
> So, it seems you want to discard small invalid chunks in the current active
> segments, right? If so, how do you think this change?
>
> ---
>  fs/f2fs/segment.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 82078734f379..394a6fef7f82 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -853,11 +853,10 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>  	if (se->valid_blocks == max_blocks || !f2fs_discard_en(sbi))
>  		return false;
>  
> -	if (!force) {
> -		if (!test_opt(sbi, DISCARD) || !se->valid_blocks ||
> -		    SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards)
> -			return false;
> -	}
> +	if (!force && (!test_opt(sbi, DISCARD) ||
> +		(!se->valid_blocks && !IS_CURSEG(sbi, cpc->trim_start)) ||
> +		SM_I(sbi)->nr_discards >= SM_I(sbi)->max_discards))
> +		return false;
>  
>  	/* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>  	for (i = 0; i < entries; i++)


-- 
Thanks,
Yunlong Song

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-05  2:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-03  9:01 [PATCH] f2fs: fix small discards when se->valid_blocks is zero Yunlong Song
2017-01-04  1:54 ` Chao Yu
2017-01-04  1:55 ` Jaegeuk Kim
2017-01-04  3:59   ` Yunlong Song
2017-01-04 22:36     ` Jaegeuk Kim
2017-01-05  2:38       ` Yunlong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).