[Ocfs2-tools-devel] [PATCH 5/6] Add unwritten extent support in ocfs2-tools.

[tao.ma]

Joel Becker wrote:
> On Mon, Sep 24, 2007 at 05:20:56PM +0800, Tao Ma wrote:
>   
>> Add unwritten extent support in ocfs2-tools. The modification includes:
>> 1. Add tree left rotations in libocfs2.
>> 3. Add new libocfs2 API for the user to allocate unwritten extents.
>>
>> Signed-off-by: Tao Ma <tao.ma at oracle.com>
>>     
>
> 	A couple comments.
>
>   
>> @@ -225,20 +265,29 @@ static errcode_t ocfs2_sync_path_to_disk(ocfs2_filesys *fs,
>>  					 int subtree_index)
>>  {
>>  	errcode_t ret;
>> -	uint64_t blkno = right_path->p_node[subtree_index].blkno;
>> -	char *sub_root = right_path->p_node[subtree_index].buf;
>> -
>> -	assert(right_path);
>> +	uint64_t blkno = 0;
>> +	char *sub_root = NULL;
>>  
>> +	assert(left_path || right_path);
>>  	if (left_path) {
>>  		ret = ocfs2_write_path_eb(fs, left_path, subtree_index);
>>  		if (ret)
>>  			goto bail;
>> +
>> +		blkno = left_path->p_node[subtree_index].blkno;
>> +		sub_root = left_path->p_node[subtree_index].buf;
>>  	}
>>  
>> -	ret = ocfs2_write_path_eb(fs, right_path, subtree_index);
>> -	if (ret)
>> -		goto bail;
>> +	if (right_path) {
>> +		ret = ocfs2_write_path_eb(fs, right_path, subtree_index);
>> +		if (ret)
>> +			goto bail;
>> +
>> +		blkno = right_path->p_node[subtree_index].blkno;
>> +		sub_root = right_path->p_node[subtree_index].buf;
>> +	}
>> +
>> +	assert(sub_root);
>>  
>>  	if (subtree_index) {
>>  		/* subtree_index indicates an extent block. */
>>     
>
> 	Wouldn't this be better to go:
>
> -	uint64_t blkno = right_path->p_node[subtree_index].blkno;
> -	char *sub_root = right_path->p_node[subtree_index].buf;
> -
> -	assert(right_path);
> +	uint64_t blkno = 0;
> +	char *sub_root = NULL;
> +	struct ocfs2_path *path;
>  
> +	assert(left_path || right_path);
> -  	if (left_path) {
> -  		ret = ocfs2_write_path_eb(fs, left_path, subtree_index);
> -  		if (ret)
> -  			goto bail;
> - 	}
> +	if (left_path)
> +		path = left_path;
> +	else
> +		path = right_path;
> +	blkno = path->p_node[subtree_index].blkno;
> +	sub_root = right_path->p_node[subtree_index].buf;
> - 	ret = ocfs2_write_path_eb(fs, right_path, subtree_index);
> + 	ret = ocfs2_write_path_eb(fs, path, subtree_index);
> 	if (ret)
> 		goto bail;
>
>   
You solution is quite clear. Just one thing. Some call it with 
right=NULL, some call it with left=NULL while some call it with 
left_path and right_path both not NULL. And we depend on left_path only 
under the situation that right_path=NULL. So just change these codes:

+	if (right_path)
+		path = right_path;
+	else
+		path = left_path;

And others should be OK.
>> +	if (del_right_subtree) {
>> +		ocfs2_unlink_subtree(fs, left_path, right_path, subtree_index);
>> +		ocfs2_update_edge_lengths(left_path);
>> +
>> +		/*
>> +		 * Now the good extent block information is stored in left_path, so
>> +		 * synchronize the right path with it.
>> +		 */
>> +		for (i = 0; i <= subtree_index; i++)
>> +			memcpy(right_path->p_node[i].buf,
>> +			       left_path->p_node[i].buf,
>> +			       fs->fs_blocksize);
>>     
>
> 	I'm confused.  If I'm understanding this correctly, the
> rotations all happen *below* subtree_index.  So shouldn't left_path and
> right_path be identical below subtree index?  Thus, isn't this loop
> redundant?  Are you trying to do
> "for (i = subtree_index; i < path->tree_depth; i++)"?  I could be
> totally wrong here!
>   
Yes, the rotation happens below subtree_index, but will affect the 
extent block above it, say modify some extent record's range. See 
ocfs2_update_edge_lengths, you will get it. ;)
>   
>> +	ctxt.rec.e_flags &= ~OCFS2_EXT_UNWRITTEN;
>>     
>
> 	Wouldn't it be nice to have accessors so that we don't do naked
> bitops?
>   
en, maybe a function like clear_ext_rec_flag(struct ocfs2_extent_rec 
*rec, uint16_t flags) will do? Mark, what is your advice?

Tao