Re: [f2fs-dev] [PATCH RESEND] f2fs: Modify do_garbage_collect() to collect all the segs in once

[Gu Zheng <guz.fnst@cn.fujitsu.com>]

On 07/15/2013 07:56 AM, Jaegeuk Kim wrote:

> Hi Gu,
> 

Hi Kim,

> Sorry for the delay.
> Could you explain why this should be applied?

> 
> IMO, nothing was different: no removal of loops, no better code
> readability, no performance improvement.
> Rather than that, it seems that it'd be better that do_garbage_collect
> conducts GC just for one segment.
> Any opinion?

Yeah, in the default case, per section contains one segment. But we can set more
than one segments in one section, in this case, we need to call
do_garbage_collect() many times to collect all the segments. If we move the loop
into do_garbage_collect(), we only need to call do_garbage_collect() one time,
no matter how many segments in one section. I think this is a improvement,
though it seems tiny.:)
If you think this change is needless, please feel free to ignore it.:)

Best regards,
Gu  

> 
> Thanks,
> 
> 2013-07-12 (금), 13:51 +0800, Gu Zheng:
>> Current do_garbage_collect() collect per segment per time. If there are more
>> than one segments in section, we need to call do_garbage_collect() many times to
>> collect all the segments(current is a for loop). We can move the loop into the
>> do_garbage_collect(), so that we can collect all the segs of section in one time.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  fs/f2fs/gc.c |   59 ++++++++++++++++++++++++++++++++-------------------------
>>  1 files changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 35f9b1a..9589ffe 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -634,42 +634,50 @@ static int __get_victim(struct f2fs_sb_info *sbi, unsigned
>> int *victim,
>>  	return ret;
>>  }
>>
>> -static void do_garbage_collect(struct f2fs_sb_info *sbi, unsigned int segno,
>> -				struct list_head *ilist, int gc_type)
>> +static void do_garbage_collect(struct f2fs_sb_info *sbi,
>> +		unsigned int start_segno, struct list_head *ilist, int gc_type)
>>  {
>> -	struct page *sum_page;
>> -	struct f2fs_summary_block *sum;
>> -	struct blk_plug plug;
>> +	unsigned int segno = start_segno;
>>
>> -	/* read segment summary of victim */
>> -	sum_page = get_sum_page(sbi, segno);
>> -	if (IS_ERR(sum_page))
>> -		return;
>> +	for (; sbi->segs_per_sec--; segno++) {
>> +		struct page *sum_page;
>> +		struct f2fs_summary_block *sum;
>> +		struct blk_plug plug;
>>
>> -	blk_start_plug(&plug);
>> +		/* read segment summary of victim */
>> +		sum_page = get_sum_page(sbi, segno);
>> +		if (IS_ERR(sum_page))
>> +			continue;
>>
>> -	sum = page_address(sum_page);
>> +		blk_start_plug(&plug);
>>
>> -	switch (GET_SUM_TYPE((&sum->footer))) {
>> -	case SUM_TYPE_NODE:
>> -		gc_node_segment(sbi, sum->entries, segno, gc_type);
>> -		break;
>> -	case SUM_TYPE_DATA:
>> -		gc_data_segment(sbi, sum->entries, ilist, segno, gc_type);
>> -		break;
>> -	}
>> -	blk_finish_plug(&plug);
>> +		sum = page_address(sum_page);
>>
>> -	stat_inc_seg_count(sbi, GET_SUM_TYPE((&sum->footer)));
>> -	stat_inc_call_count(sbi->stat_info);
>> +		switch (GET_SUM_TYPE((&sum->footer))) {
>> +		case SUM_TYPE_NODE:
>> +			gc_node_segment(sbi, sum->entries,
>> +				segno, gc_type);
>> +			break;
>> +		case SUM_TYPE_DATA:
>> +			gc_data_segment(sbi, sum->entries, ilist,
>> +				segno, gc_type);
>> +			break;
>> +		default:
>> +			BUG();
>> +		}
>> +		blk_finish_plug(&plug);
>> +
>> +		stat_inc_seg_count(sbi, GET_SUM_TYPE((&sum->footer)));
>> +		stat_inc_call_count(sbi->stat_info);
>>
>> -	f2fs_put_page(sum_page, 1);
>> +		f2fs_put_page(sum_page, 1);
>> +	}
>>  }
>>
>>  int f2fs_gc(struct f2fs_sb_info *sbi)
>>  {
>>  	struct list_head ilist;
>> -	unsigned int segno, i;
>> +	unsigned int segno;
>>  	int gc_type = BG_GC;
>>  	int nfree = 0;
>>  	int ret = -1;
>> @@ -688,8 +696,7 @@ gc_more:
>>  		goto stop;
>>  	ret = 0;
>>
>> -	for (i = 0; i < sbi->segs_per_sec; i++)
>> -		do_garbage_collect(sbi, segno + i, &ilist, gc_type);
>> +	do_garbage_collect(sbi, segno, &ilist, gc_type);
>>
>>  	if (gc_type == FG_GC) {
>>  		sbi->cur_victim_sec = NULL_SEGNO;
> 



------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel