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

[Chandan Babu R <chandan.babu@oracle.com>]

On Wed, Jul 12, 2023 at 10:10:09 AM -0700, Darrick J. Wong wrote:
> 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?
>

finish_dump() is a much better name for the callback. I will make the change
before posting the next version of the patchset.

-- 
chandan