f2fs: fix small discards when se->valid_blocks is zero
diff mbox

Message ID 1483434117-24427-1-git-send-email-yunlong.song@huawei.com
State New
Headers show

Commit Message

Yunlong Song Jan. 3, 2017, 9:01 a.m. UTC
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(-)

Comments

Chao Yu Jan. 4, 2017, 1:54 a.m. UTC | #1
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;
>  	}
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaegeuk Kim Jan. 4, 2017, 1:55 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yunlong Song Jan. 4, 2017, 3:59 a.m. UTC | #3
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
> .
>

Patch
diff mbox

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