Re: [PATCH V2 08/23] metadump: Introduce metadump v1 operations

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jun 06, 2023 at 02:57:51PM +0530, Chandan Babu R wrote:
> This commit moves functionality associated with writing metadump to disk into
> a new function. It also renames metadump initialization, write and release
> functions to reflect the fact that they work with v1 metadump files.
> 
> The metadump initialization, write and release functions are now invoked via
> metadump_ops->init(), metadump_ops->write() and metadump_ops->release()
> respectively.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  db/metadump.c | 123 +++++++++++++++++++++++++-------------------------
>  1 file changed, 61 insertions(+), 62 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 266d3413..287e8f91 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -151,59 +151,6 @@ print_progress(const char *fmt, ...)
>  	metadump.progress_since_warning = true;
>  }
>  
> -/*
> - * A complete dump file will have a "zero" entry in the last index block,
> - * even if the dump is exactly aligned, the last index will be full of
> - * zeros. If the last index entry is non-zero, the dump is incomplete.
> - * Correspondingly, the last chunk will have a count < num_indices.
> - *
> - * Return 0 for success, -1 for failure.
> - */
> -
> -static int
> -write_index(void)
> -{
> -	struct xfs_metablock *metablock = metadump.metablock;
> -	/*
> -	 * write index block and following data blocks (streaming)
> -	 */
> -	metablock->mb_count = cpu_to_be16(metadump.cur_index);
> -	if (fwrite(metablock, (metadump.cur_index + 1) << BBSHIFT, 1,
> -			metadump.outf) != 1) {
> -		print_warning("error writing to target file");
> -		return -1;
> -	}
> -
> -	memset(metadump.block_index, 0, metadump.num_indices * sizeof(__be64));
> -	metadump.cur_index = 0;
> -	return 0;
> -}
> -
> -/*
> - * Return 0 for success, -errno for failure.
> - */
> -static int
> -write_buf_segment(
> -	char		*data,
> -	int64_t		off,
> -	int		len)
> -{
> -	int		i;
> -	int		ret;
> -
> -	for (i = 0; i < len; i++, off++, data += BBSIZE) {
> -		metadump.block_index[metadump.cur_index] = cpu_to_be64(off);
> -		memcpy(&metadump.block_buffer[metadump.cur_index << BBSHIFT],
> -			data, BBSIZE);
> -		if (++metadump.cur_index == metadump.num_indices) {
> -			ret = write_index();
> -			if (ret)
> -				return -EIO;
> -		}
> -	}
> -	return 0;
> -}
> -
>  /*
>   * we want to preserve the state of the metadata in the dump - whether it is
>   * intact or corrupt, so even if the buffer has a verifier attached to it we
> @@ -240,15 +187,16 @@ write_buf(
>  
>  	/* handle discontiguous buffers */
>  	if (!buf->bbmap) {
> -		ret = write_buf_segment(buf->data, buf->bb, buf->blen);
> +		ret = metadump.mdops->write(buf->typ->typnm, buf->data, buf->bb,
> +				buf->blen);
>  		if (ret)
>  			return ret;
>  	} else {
>  		int	len = 0;
>  		for (i = 0; i < buf->bbmap->nmaps; i++) {
> -			ret = write_buf_segment(buf->data + BBTOB(len),
> -						buf->bbmap->b[i].bm_bn,
> -						buf->bbmap->b[i].bm_len);
> +			ret = metadump.mdops->write(buf->typ->typnm,
> +				buf->data + BBTOB(len), buf->bbmap->b[i].bm_bn,
> +				buf->bbmap->b[i].bm_len);
>  			if (ret)
>  				return ret;
>  			len += buf->bbmap->b[i].bm_len;
> @@ -3010,7 +2958,7 @@ done:
>  }
>  
>  static int
> -init_metadump(void)
> +init_metadump_v1(void)
>  {
>  	metadump.metablock = (xfs_metablock_t *)calloc(BBSIZE + 1, BBSIZE);
>  	if (metadump.metablock == NULL) {
> @@ -3051,12 +2999,61 @@ init_metadump(void)
>  	return 0;
>  }
>  
> +static int
> +end_write_metadump_v1(void)
> +{
> +	/*
> +	 * write index block and following data blocks (streaming)
> +	 */
> +	metadump.metablock->mb_count = cpu_to_be16(metadump.cur_index);
> +	if (fwrite(metadump.metablock, (metadump.cur_index + 1) << BBSHIFT, 1,
> +			metadump.outf) != 1) {
> +		print_warning("error writing to target file");
> +		return -1;
> +	}
> +
> +	memset(metadump.block_index, 0, metadump.num_indices * sizeof(__be64));
> +	metadump.cur_index = 0;
> +	return 0;
> +}
> +
> +static int
> +write_metadump_v1(
> +	enum typnm	type,
> +	char		*data,
> +	xfs_daddr_t	off,
> +	int		len)
> +{
> +	int		i;
> +	int		ret;
> +
> +	for (i = 0; i < len; i++, off++, data += BBSIZE) {
> +		metadump.block_index[metadump.cur_index] = cpu_to_be64(off);
> +		memcpy(&metadump.block_buffer[metadump.cur_index << BBSHIFT],
> +			data, BBSIZE);
> +		if (++metadump.cur_index == metadump.num_indices) {
> +			ret = end_write_metadump_v1();
> +			if (ret)
> +				return -EIO;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void
> -release_metadump(void)
> +release_metadump_v1(void)
>  {
>  	free(metadump.metablock);
>  }
>  
> +static struct metadump_ops metadump1_ops = {
> +	.init		= init_metadump_v1,
> +	.write		= write_metadump_v1,
> +	.end_write	= end_write_metadump_v1,
> +	.release	= release_metadump_v1,
> +};
> +
>  static int
>  metadump_f(
>  	int 		argc,
> @@ -3193,7 +3190,9 @@ metadump_f(
>  		}
>  	}
>  
> -	ret = init_metadump();
> +	metadump.mdops = &metadump1_ops;
> +
> +	ret = metadump.mdops->init();
>  	if (ret)
>  		goto out;
>  
> @@ -3216,7 +3215,7 @@ metadump_f(
>  
>  	/* write the remaining index */
>  	if (!exitcode)
> -		exitcode = write_index() < 0;
> +		exitcode = metadump.mdops->end_write() < 0;

Now that I see ->end_write in usage, I think it would be better named
->finish_dump or something like that.  It's only called once, right?

--D

>  
>  	if (metadump.progress_since_warning)
>  		fputc('\n', metadump.stdout_metadump ? stderr : stdout);
> @@ -3235,7 +3234,7 @@ metadump_f(
>  	while (iocur_sp > start_iocur_sp)
>  		pop_cur();
>  
> -	release_metadump();
> +	metadump.mdops->release();
>  
>  out:
>  	return 0;
> -- 
> 2.39.1
>