Re: [PATCH 09/24] metadump: Introduce metadump v1 operations

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

On Tue, May 23, 2023 at 02:30:35PM +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(), metadump_ops->write_metadump() and
> metadump_ops->release_metadump() respectively.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  db/metadump.c | 124 +++++++++++++++++++++++++-------------------------
>  1 file changed, 61 insertions(+), 63 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 56d8c3bdf..7265f73ec 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -135,59 +135,6 @@ print_progress(const char *fmt, ...)
>  	metadump.progress_since_warning = 1;
>  }
>  
> -/*
> - * 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
> @@ -224,15 +171,16 @@ write_buf(
>  
>  	/* handle discontiguous buffers */
>  	if (!buf->bbmap) {
> -		ret = write_buf_segment(buf->data, buf->bb, buf->blen);
> +		ret = metadump.mdops->write_metadump(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_metadump(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;
> @@ -2994,7 +2942,7 @@ done:
>  }
>  
>  static int
> -init_metadump(void)
> +init_metadump_v1(void)
>  {
>  	metadump.metablock = (xfs_metablock_t *)calloc(BBSIZE + 1, BBSIZE);
>  	if (metadump.metablock == NULL) {
> @@ -3035,12 +2983,60 @@ 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,
> +	int64_t		off,

This really ought to be an xfs_daddr_t, right?

> +	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);

Wondering if this ought to be called ->record_segment or something, since
it's not really writing anything to disk, merely copying it to the index
buffer.

> +		if (++metadump.cur_index == metadump.num_indices) {
> +			ret = end_write_metadump_v1();
> +			if (ret)
> +				return -EIO;

This is "generic" code for "Have we filled up the index table?  If so,
then write the index block the indexed data".  Shouldn't it go in
write_buf?  And then write_buf does something like:

	while (len > 0) {
		segment_len = min(len, metadump.num_indices - metadump.cur_index);

		metadump.ops->record_segment(type, buf, daddr, segment_len);

		metadump.cur_index += segment_len;
		if (metadump.cur_index == metadump.num_indices) {
			metadump.ops->write_index(...);
			metadump.cur_index = 0;
		}

		len -= segment_len;
		daddr += segment_len;
		buf += (segment_len << 9);
	}

	if (metadump.cur_index)
		metadump.ops->write_index(...);
	metadump.cur_index = 0;

--D

> +		}
> +	}
> +
> +        return 0;
> +}
> +
>  static void
> -release_metadump(void)
> +release_metadump_v1(void)
>  {
>  	free(metadump.metablock);
>  }
>  
> +static struct metadump_ops metadump1_ops = {
> +	.init_metadump = init_metadump_v1,
> +	.write_metadump = write_metadump_v1,
> +	.end_write_metadump = end_write_metadump_v1,
> +	.release_metadump = release_metadump_v1,
> +};
> +
>  static int
>  metadump_f(
>  	int 		argc,
> @@ -3182,9 +3178,11 @@ metadump_f(
>  		}
>  	}
>  
> -	ret = init_metadump();
> +	metadump.mdops = &metadump1_ops;
> +
> +	ret = metadump.mdops->init_metadump();
>  	if (ret)
> -		return 0;
> +		goto out;
>  
>  	exitcode = 0;
>  
> @@ -3206,7 +3204,7 @@ metadump_f(
>  
>  	/* write the remaining index */
>  	if (!exitcode)
> -		exitcode = write_index() < 0;
> +		exitcode = metadump.mdops->end_write_metadump() < 0;
>  
>  	if (metadump.progress_since_warning)
>  		fputc('\n', metadump.stdout_metadump ? stderr : stdout);
> @@ -3225,7 +3223,7 @@ metadump_f(
>  	while (iocur_sp > start_iocur_sp)
>  		pop_cur();
>  
> -	release_metadump();
> +	metadump.mdops->release_metadump();
>  
>  out:
>  	return 0;
> -- 
> 2.39.1
>