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

[Joel Becker]

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;

Otherwise, you're repeating the same thing for (left || right), whereas
the old code was ((possibly left) && right)
	In fact, if the new code does assert(left_path || right_path),
can't we just change the prototype to
ocfs2_sync_path_to_disk(fs, path, subtree_index) and have all the
callers fill in the correct path?   Perhaps we don't want that - the
callers might want to blindly call it without checking their left and
right path pointers.  But shouldn't they know which is the valid one?

> +	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!

> +	ctxt.rec.e_flags &= ~OCFS2_EXT_UNWRITTEN;

	Wouldn't it be nice to have accessors so that we don't do naked
bitops?

Joel

-- 

"Not everything that can be counted counts, and not everything
 that counts can be counted."
        - Albert Einstein 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127