Re: [PATCH] qemu-img: Add --target-is-zero to convert

[David Edmondson <dme@dme.org>]

On Monday, 2020-01-20 at 19:33:43 -05, John Snow wrote:

> CC qemu-block and block maintainers
>
> On 1/17/20 5:34 AM, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>> 
>
> Is there no way to convince bdrv_has_zero_init to return what we want
> already in this case?

In the current HEAD code, bdrv_has_zero_init will never be called for
“convert -n” (skip target volume creation).

If -n is not specified the host_device block driver doesn't provide a
bdrv_has_zero_init function, so it's assumed not supported.

> I cannot recall off hand, but wonder if there's an advanced syntax
> method of specifying the target image that can set this flag already.

I couldn't see one, but I'd be happy to learn of its existence. My first
approach was to add a raw specific option and add it using
--target-image-opts, resulting in something like:

qemu-img convert -n --target-image-opts sparse.qcow2 \
	driver=raw,file.filename=/dev/sdg,assume-blank=on

(assume-blank=on is the new bit). This worked, but is only useful for
raw targets.

The discussion here led me to switch to --target-is-zero.

Mark Kanda sent me some comments suggesting that I get rid of the new
target_is_zero boolean and simply set has_zero_init, which I will do
before sending out another patch if this overall approach is considered
appropriate.

>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
>> ---
>>  qemu-img.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 95a24b9762..56ca727e8c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -70,6 +70,7 @@ enum {
>>      OPTION_PREALLOCATION = 265,
>>      OPTION_SHRINK = 266,
>>      OPTION_SALVAGE = 267,
>> +    OPTION_TARGET_IS_ZERO = 268,
>>  };
>>  
>>  typedef enum OutputFormat {
>> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
>>      bool copy_range;
>>      bool salvage;
>>      bool quiet;
>> +    bool target_is_zero;
>>      int min_sparse;
>>      int alignment;
>>      size_t cluster_sectors;
>> @@ -1984,10 +1986,11 @@ static int convert_do_copy(ImgConvertState *s)
>>      int64_t sector_num = 0;
>>  
>>      /* Check whether we have zero initialisation or can get it efficiently */
>> -    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
>> +    s->has_zero_init = s->target_is_zero;
>> +
>> +    if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
>> +        !s->target_has_backing) {
>>          s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
>> -    } else {
>> -        s->has_zero_init = false;
>>      }
>>  
>>      if (!s->has_zero_init && !s->target_has_backing &&
>> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
>>          .buf_sectors        = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>>          .wr_in_order        = true,
>>          .num_coroutines     = 8,
>> +        .target_is_zero     = false,
>>      };
>>  
>>      for(;;) {
>> @@ -2086,6 +2090,7 @@ static int img_convert(int argc, char **argv)
>>              {"force-share", no_argument, 0, 'U'},
>>              {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>>              {"salvage", no_argument, 0, OPTION_SALVAGE},
>> +            {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>>              {0, 0, 0, 0}
>>          };
>>          c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
>> @@ -2209,6 +2214,9 @@ static int img_convert(int argc, char **argv)
>>          case OPTION_TARGET_IMAGE_OPTS:
>>              tgt_image_opts = true;
>>              break;
>> +        case OPTION_TARGET_IS_ZERO:
>> +            s.target_is_zero = true;
>> +            break;
>>          }
>>      }
>>  
>> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
>>          warn_report("This will become an error in future QEMU versions.");
>>      }
>>  
>> +    if (s.target_is_zero && !skip_create) {
>> +        error_report("--target-is-zero requires use of -n flag");
>> +        goto fail_getopt;
>> +    }
>> +
>>      s.src_num = argc - optind - 1;
>>      out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>>  
>> 

dme.
-- 
Please forgive me if I act a little strange, for I know not what I do.