[Ocfs2-devel] [PATCH 1/1] Ocfs2: Optimize punching-hole codes v2.

[tristan]

Hi Joel,

Thanks for your comments.

Joel Becker wrote:
> On Thu, Feb 25, 2010 at 05:49:08PM +0800, Tristan Ye wrote:
>> ===========================================================================
>>  * Former punching-hole mechanism:
>> ===========================================================================
>>
>>    I waited 1 hour for its completion, unfortunately it's still ongoing.
>>
>> ===========================================================================
>>  * Patched punching-hode mechanism:
>> ===========================================================================
>>
>>    real	0m2.518s
>>    user	0m0.000s
>>    sys	0m2.445s
>>
>> That means we've gained up to 1000 times improvement on performance in this
>> case, whee! It's fairly cool. and it looks like that performance gain will
>> be raising when extent records grow.
>
> 	Love the numbers, obviously.

Maye such extent number didn't exist in a real world, it however was 
doing something positive;)

>
>> The patch was based on my former 2 patches, which were about truncating
>> codes optimization and fixup to handle CoW on punching hole.
>
> 	I've already reviewed these.  I'm waiting on Mark's ack for them
> to go to ocfs2.git.
>
>> -	cpos = trunc_start;
>> -	while (trunc_len) {
>> -		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos,
>> -					 &alloc_size, &flags);
>> -		if (ret) {
>> -			mlog_errno(ret);
>> -			goto out;
>> -		}
>> +	path = ocfs2_new_path_from_et(&et);
>> +	if (!path) {
>> +		ret = -ENOMEM;
>> +		mlog_errno(ret);
>> +		goto out;
>> +	}
>> +
>> +start:
>> +	if (trunc_end == 0) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>
> 	NO!  Don't do loops via goto.  Just Don't.  There are some
> convoluted functions that end up being cleaner with gotos, but they are
> *convoluted*.  This is a simple loop.  Keep it that way.
> 	In fact, this all really wants to be a helper function:
>
> 	while (trunc_end > 0) {
> 		do_one_hunk();
> 		ocfs2_reinit_path(path, 1);
> 	}
>
> 	Actually, looking at the rest of the code, I see a couple
> helpers.  If you wrap trunc_start, trunc_len, etc in a structure, you
> can pass it through.
>
>>  
>> -		if (alloc_size > trunc_len)
>> -			alloc_size = trunc_len;
>> +	/*
>> +	 * Unlike truncating codes, here we want to find a path which contains
>> +	 * (trunc_end - 1) cpos, and trunc_end will be decreased after each
>> +	 * removal of a record range.
>> +	 *
>> +	 * Why didn't use trunc_end to search the path?
>> +	 * The reason is simple, think about the situation when we cross the
>> +	 * extent block, we need to find the adjacent block by decreasing one
>> +	 * cluster, otherwise, it will run into loop.
>> +	 */
>> +	ret = ocfs2_find_path(INODE_CACHE(inode), path, cluster_within_list);
>> +	if (ret) {
>> +		mlog_errno(ret);
>> +		goto out;
>> +	}
>>  
>> -		/* Only do work for non-holes */
>> -		if (phys_cpos != 0) {
>> -			ret = ocfs2_remove_btree_range(inode, &et, cpos,
>> -						       phys_cpos, alloc_size,
>> -						       &dealloc, refcount_loc,
>> -						       flags);
>> -			if (ret) {
>> -				mlog_errno(ret);
>> -				goto out;
>> -			}
>> +	el = path_leaf_el(path);
>> +
>> +	for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) {
>> +		rec = &el->l_recs[i];
>> +		/*
>> +		 * Find the rightmost record which contains 'trunc_end' cpos,
>> +		 * and we just simply jump to previous record if the trunc_end
>> +		 * is the start of a record.
>> +		 */
>> +		if (le32_to_cpu(rec->e_cpos) < trunc_end) {
>> +			/*
>> +			 * Skip a hole.
>> +			 */
>> +			if ((le32_to_cpu(rec->e_cpos) +
>> +			     ocfs2_rec_clusters(el, rec)) < trunc_end)
>> +				trunc_end = le32_to_cpu(rec->e_cpos) +
>> +					ocfs2_rec_clusters(el, rec);
>> +			break;
>>  		}
>>  
>> -		cpos += alloc_size;
>> -		trunc_len -= alloc_size;
>> +		if (le32_to_cpu(rec->e_cpos) == trunc_end) {
>> +			i--;
>> +			break;
>> +		}
>> +	}
>> +
>> +	rec = &el->l_recs[i];
>
> 	This is the first helper.  It finds the rec.
>
>> +	flags = rec->e_flags;
>> +	range = le32_to_cpu(rec->e_cpos) + ocfs2_rec_clusters(el, rec);
>> +
>> +	/*
>> +	 * Similar with the truncating codes, we also handle the
>> +	 * following three cases in order:
>> +	 *
>> +	 * - remove the entire record
>> +	 * - remove a partial record
>> +	 * - no record needs to be removed
>> +	 */
>> +	if (le32_to_cpu(rec->e_cpos) >= trunc_start) {
>> +		trunc_cpos = le32_to_cpu(rec->e_cpos);
>> +		trunc_len = trunc_end - le32_to_cpu(rec->e_cpos);
>> +		blkno = le64_to_cpu(rec->e_blkno);
>> +		trunc_end = le32_to_cpu(rec->e_cpos);
>> +	} else if (range > trunc_start) {
>> +		trunc_cpos = trunc_start;
>> +		trunc_len = range - trunc_start;
>> +		coff = trunc_start - le32_to_cpu(rec->e_cpos);
>> +		blkno = le64_to_cpu(rec->e_blkno) +
>> +				ocfs2_clusters_to_blocks(inode->i_sb, coff);
>> +		trunc_end = trunc_start;
>> +	} else {
>> +		/*
>> +		 * It may have two following possibilities:
>> +		 *
>> +		 * - last record has been removed
>> +		 * - trunc_start was within a hole
>> +		 *
>> +		 * both two cases mean the completion of hole punching.
>> +		 */
>> +		ret = 0;
>> +		goto out;
>>  	}
>>  
>> +	phys_cpos = ocfs2_blocks_to_clusters(inode->i_sb, blkno);
>
> 	This is the second helper.  It computes the actual results from
> the found record.
>
>> +	ret = ocfs2_remove_btree_range(inode, &et, trunc_cpos,
>> +				       phys_cpos, trunc_len, &dealloc,
>> +				       refcount_loc, flags);
>> +	if (ret < 0) {
>> +		mlog_errno(ret);
>> +		goto out;
>> +	}
>> +
>> +	if (trunc_end > 0)
>> +		cluster_within_list = trunc_end - 1;
>
> 	This is the third helper.  It does the actual punch.

Your words make sense:)


I may wrap these loose codes up for better readability.


Tristan.

>
> Joel
>