Re: [Qemu-devel] [PATCH 1.8 3/6] qemu-img: add option to specify alternate iobuffer size

[Paolo Bonzini]

Il 25/11/2013 14:57, Peter Lieven ha scritto:
> since the convert process is basically a sync operation it might
> be benificial in some case to change the hardcoded I/O buffer
> size to an alternate (greater) value.

Do you really need the extra knob?  You can just add to BlockLimits the
optimal transfer length, and use it unconditionally.

(It would be interesting to add a double-buffer to qemu-img convert,
using asynchronous I/O...).

Paolo

> Signed-off-by: Peter Lieven <address@hidden>
> ---
>  qemu-img-cmds.hx |    4 ++--
>  qemu-img.c       |   25 ++++++++++++++++++++-----
>  qemu-img.texi    |    4 +++-
>  3 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index da1d965..e0b8ab4 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -34,9 +34,9 @@ STEXI
>  ETEXI
>  
>  DEF("convert", img_convert,
> -    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o 
> options] [-s snapshot_name] [-S sparse_size] filename [filename2 [...]] 
> output_filename")
> +    "convert [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-O output_fmt] [-o 
> options] [-s snapshot_name] [-S sparse_size] [-m iobuf_size] filename 
> [filename2 [...]] output_filename")
>  STEXI
> address@hidden convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] 
> [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S 
> @var{sparse_size}] @var{filename} address@hidden [...]] @var{output_filename}
> address@hidden convert [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] 
> [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S 
> @var{sparse_size}] [-m @var{iobuf_size}] @var{filename} address@hidden [...]] 
> @var{output_filename}
>  ETEXI
>  
>  DEF("info", img_info,
> diff --git a/qemu-img.c b/qemu-img.c
> index e2d1a0a..0ce5d14 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -105,6 +105,7 @@ static void help(void)
>             "       conversion. If the number of bytes is 0, the source will 
> not be scanned for\n"
>             "       unallocated or zero sectors, and the destination image 
> will always be\n"
>             "       fully allocated\n"
> +           "  '-m' specify I/O buffer size in bytes (default 2M)\n"
>             "  '--output' takes the format in which the output must be done 
> (human or json)\n"
>             "  '-n' skips the target volume creation (useful if the volume is 
> created\n"
>             "       prior to running qemu-img)\n"
> @@ -1135,6 +1136,7 @@ static int img_convert(int argc, char **argv)
>              sector_num_next_status = 0;
>      uint64_t bs_sectors;
>      uint8_t * buf = NULL;
> +    size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
>      BlockDriverInfo bdi;
>      QEMUOptionParameter *param = NULL, *create_options = NULL;
> @@ -1152,7 +1154,7 @@ static int img_convert(int argc, char **argv)
>      compress = 0;
>      skip_create = 0;
>      for(;;) {
> -        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:t:qn");
> +        c = getopt(argc, argv, "f:O:B:s:hce6o:pS:m:t:qn");
>          if (c == -1) {
>              break;
>          }
> @@ -1200,6 +1202,19 @@ static int img_convert(int argc, char **argv)
>              min_sparse = sval / BDRV_SECTOR_SIZE;
>              break;
>          }
> +        case 'm':
> +        {
> +            int64_t sval;
> +            char *end;
> +            sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
> +            if (sval < BDRV_SECTOR_SIZE || *end) {
> +                error_report("Invalid I/O buffer size specified");
> +                return 1;
> +            }
> +
> +            bufsectors = sval / BDRV_SECTOR_SIZE;
> +            break;
> +        }
>          case 'p':
>              progress = 1;
>              break;
> @@ -1371,7 +1386,7 @@ static int img_convert(int argc, char **argv)
>      bs_i = 0;
>      bs_offset = 0;
>      bdrv_get_geometry(bs[0], &bs_sectors);
> -    buf = qemu_blockalign(out_bs, IO_BUF_SIZE);
> +    buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
>  
>      if (skip_create) {
>          int64_t output_length = bdrv_getlength(out_bs);
> @@ -1394,7 +1409,7 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>          cluster_size = bdi.cluster_size;
> -        if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) {
> +        if (cluster_size <= 0 || cluster_size > bufsectors * 
> BDRV_SECTOR_SIZE) {
>              error_report("invalid cluster size");
>              ret = -1;
>              goto out;
> @@ -1531,8 +1546,8 @@ static int img_convert(int argc, char **argv)
>                  sector_num_next_status = sector_num + n1;
>              }
>  
> -            if (nb_sectors >= (IO_BUF_SIZE / 512)) {
> -                n = (IO_BUF_SIZE / 512);
> +            if (nb_sectors >= bufsectors) {
> +                n = bufsectors;
>              } else {
>                  n = nb_sectors;
>              }
> diff --git a/qemu-img.texi b/qemu-img.texi
> index da36975..87f9d0f 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -179,7 +179,7 @@ Error on reading data
>  
>  @end table
>  
> address@hidden convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O 
> @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S 
> @var{sparse_size}] @var{filename} address@hidden [...]] @var{output_filename}
> address@hidden convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-O 
> @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] [-S 
> @var{sparse_size}] [-m @var{iobuf_size}] @var{filename} address@hidden [...]] 
> @var{output_filename}
>  
>  Convert the disk image @var{filename} or a snapshot @var{snapshot_name} to 
> disk image @var{output_filename}
>  using format @var{output_fmt}. It can be optionally compressed (@code{-c}
> @@ -199,6 +199,8 @@ conversion. If @var{sparse_size} is 0, the source will 
> not be scanned for
>  unallocated or zero sectors, and the destination image will always be
>  fully allocated.
>  
> address@hidden indicates the size of the I/O buffer (defaults to 2M).
> +
>  You can use the @var{backing_file} option to force the output image to be
>  created as a copy on write image of the specified base image; the
>  @var{backing_file} should have the same content as the input's base image,
>