[Ocfs2-tools-devel] [PATCH 1/9] dx_dirs: stop iterate dir entries for I/O error

[Tao Ma]

Hi coly,
	The patch looks good. Only small comments.

Coly Li wrote:
> Callback dx_iterator() may encounter an I/O error when calling
> ocfs2_read_dx_leaf(). The caller of dx_iterator is extent_iterate_el(),
> which does not accept error code other than OCFS2_EXTENT_ERROR and
> OCFS2_EXTENT_ABORT. The result is, dir entries iteration can not stop
> if there is an I/O error happens in dx_iterator().
> 
> This patch add 'errcode_t err' member to struct dx_iterator_data, if
> error returned from ocfs2_read_dx_leaf(), the error code will be
> stored here, then dx_iterator() returns OCFS2_EXTENT_ERROR to make
> extent_iterate_el() quit.
> 
> Thanks to Tao Ma for catching this error.
> 
> Signed-off-by: Coly Li <coly.li at suse.de>
> Cc: Mark Fasheh <mfasheh at suse.com>
> Cc: Tao Ma <tao.ma at oracle.com>
> ---
>  libocfs2/dir_iterate.c |   22 +++++++++++++++-------
>  libocfs2/extents.c     |    2 +-
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/libocfs2/dir_iterate.c b/libocfs2/dir_iterate.c
> index 8a3f5a9..d044bb8 100644
> --- a/libocfs2/dir_iterate.c
> +++ b/libocfs2/dir_iterate.c
> @@ -320,6 +320,7 @@ struct dx_iterator_data {
>  	void *dx_priv_data;
>  	char *leaf_buf;
>  	struct ocfs2_dx_root_block *dx_root;
> +	errcode_t err;
>  };
> 
>  static int dx_iterator(ocfs2_filesys *fs,
> @@ -330,7 +331,7 @@ static int dx_iterator(ocfs2_filesys *fs,
>  		       int ref_recno,
>  		       void *priv_data)
>  {
> -	int ret, i;
> +	int err, i;
I guess err should be errcode_t.
>  	struct ocfs2_dx_leaf *dx_leaf;
>  	struct dx_iterator_data *iter = priv_data;
>  	uint64_t blkno, count;
> @@ -339,9 +340,11 @@ static int dx_iterator(ocfs2_filesys *fs,
> 
>  	blkno = rec->e_blkno;
>  	for (i = 0; i < count; i++) {
> -		ret = ocfs2_read_dx_leaf(fs, blkno, iter->leaf_buf);
> -		if (ret)
> -			return ret;
> +		err = ocfs2_read_dx_leaf(fs, blkno, iter->leaf_buf);
> +		if (err) {
> +			iter->err = err;
> +			return OCFS2_EXTENT_ERROR;
> +		}
> 
>  		dx_leaf = (struct ocfs2_dx_leaf *)iter->leaf_buf;
>  		iter->dx_func(fs, &dx_leaf->dl_list, iter->dx_root, dx_leaf,
> @@ -387,8 +390,7 @@ extern errcode_t ocfs2_dx_entries_iterate(ocfs2_filesys *fs,
>  	dx_root = (struct ocfs2_dx_root_block *)buf;
> 
>  	if (dx_root->dr_flags & OCFS2_DX_FLAG_INLINE) {
> -		func(fs, &dx_root->dr_entries, dx_root, NULL, priv_data);
> -		ret = 0;
> +		ret = func(fs, &dx_root->dr_entries, dx_root, NULL, priv_data);
>  		goto out;
>  	}
> 
> @@ -404,10 +406,16 @@ extern errcode_t ocfs2_dx_entries_iterate(ocfs2_filesys *fs,
>  	data.dx_priv_data = priv_data;
>  	data.leaf_buf = leaf_buf;
>  	data.dx_root = dx_root;
> +	data.err = 0;
>  	ret = ocfs2_extent_iterate_dx_root(fs, dx_root,
>  					   OCFS2_EXTENT_FLAG_DATA_ONLY, eb_buf,
>  					   dx_iterator, &data);
> -
> +	/* dx_iterator may set the error code for non-extents-related
> +	 * errors. If the error code is set by dx_iterator, no matter
> +	 * what ocfs2_extent_iterate_dx_root() returns, we should take
> +	 * data.err as retured error code. */
> +	if (data.err)
> +		ret = data.err;
>  out:
>  	if (buf)
>  		ocfs2_free(&buf);
> diff --git a/libocfs2/extents.c b/libocfs2/extents.c
> index 8c322b1..bb233f0 100644
> --- a/libocfs2/extents.c
> +++ b/libocfs2/extents.c
> @@ -470,7 +470,7 @@ errcode_t ocfs2_extent_iterate_inode(ocfs2_filesys *fs,
>  					         uint64_t ref_blkno,
>  					         int ref_recno,
>  					         void *priv_data),
> -					         void *priv_data)
> +				     void *priv_data)
why you want to change this?

Regards,
Tao