Re: [Qemu-devel] [PATCH v2 8/9] blkdebug: Add ability to override unmap geometries

[Max Reitz]

On 17.11.2016 21:14, Eric Blake wrote:
> Make it easier to simulate the Dell Equallogic iSCSI with its

Somehow I feel bad putting company and product names into commit messages...

> unusual 15M preferred and maximum unmap and write zero sizing,
> or to simulate Linux loopback block devices enforcing a small
> max_transfer of 64k, by allowing blkdebug to wrap any other
> device with further restrictions on various alignments.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  qapi/block-core.json | 27 ++++++++++++++-
>  block/blkdebug.c     | 97 
> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 121 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c29bef7..26f3e9f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2068,6 +2068,29 @@
>  # @align:           #optional required alignment for requests in bytes,
>  #                   must be power of 2, or 0 for default
>  #
> +# @max-transfer:    #optional maximum size for I/O transfers in bytes,
> +#                   must be multiple of the larger of @align and and 512
> +#                   (but need not be a power of 2), or 0 for default
> +#                   (since 2.9)
> +#
> +# @opt-write-zero:  #optional preferred alignment for write zero requests
> +#                   in bytes, must be multiple of the larger of @align
> +#                   and 512 (but need not be a power of 2), or 0 for
> +#                   default (since 2.9)
> +#
> +# @max-write-zero:  #optional maximum size for write zero requests in bytes,
> +#                   must be multiple of the larger of @align and 512 (but
> +#                   need not be a power of 2), or 0 for default (since 2.9)
> +#
> +# @opt-discard:     #optional preferred alignment for discard requests
> +#                   in bytes, must be multiple of the larger of @align
> +#                   and 512 (but need not be a power of 2), or 0 for
> +#                   default (since 2.9)
> +#
> +# @max-discard:     #optional maximum size for discard requests in bytes,
> +#                   must be multiple of the larger of @align and 512 (but
> +#                   need not be a power of 2), or 0 for default (since 2.9)
> +#
>  # @inject-error:    #optional array of error injection descriptions
>  #
>  # @set-state:       #optional array of state-change descriptions
> @@ -2077,7 +2100,9 @@
>  { 'struct': 'BlockdevOptionsBlkdebug',
>    'data': { 'image': 'BlockdevRef',
>              '*config': 'str',
> -            '*align': 'int',
> +            '*align': 'int', '*max-transfer': 'int32',
> +            '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
> +            '*opt-discard': 'int32', '*max-discard': 'int32',
>              '*inject-error': ['BlkdebugInjectErrorOptions'],
>              '*set-state': ['BlkdebugSetStateOptions'] } }
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index d45826d..3ba7a96 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -39,6 +39,11 @@ typedef struct BDRVBlkdebugState {
>      int state;
>      int new_state;
>      int align;
> +    int max_transfer;
> +    int opt_write_zero;
> +    int max_write_zero;
> +    int opt_discard;
> +    int max_discard;
> 
>      /* For blkdebug_refresh_filename() */
>      char *config_file;
> @@ -344,6 +349,31 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_SIZE,
>              .help = "Required alignment in bytes",
>          },
> +        {
> +            .name = "max-transfer",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Maximum transfer size in bytes",
> +        },
> +        {
> +            .name = "opt-write-zero",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Optimum write zero size in bytes",

s/size/alignment/?

> +        },
> +        {
> +            .name = "max-write-zero",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Maximum write zero size in bytes",
> +        },
> +        {
> +            .name = "opt-discard",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Optimum discard size in bytes",

s/size/alignment/?

> +        },
> +        {
> +            .name = "max-discard",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Maximum discard size in bytes",
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -354,7 +384,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      BDRVBlkdebugState *s = bs->opaque;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> -    uint64_t align;
> +    uint64_t align, max_transfer;
> +    uint64_t opt_write_zero, max_write_zero, opt_discard, max_discard;
>      int ret;
> 
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> @@ -389,7 +420,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
>          bs->file->bs->supported_zero_flags;
> 
> -    /* Set request alignment */
> +    /* Set alignment overrides */
>      align = qemu_opt_get_size(opts, "align", 0);
>      if (align < INT_MAX && is_power_of_2(align)) {
>          s->align = align;
> @@ -398,6 +429,53 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          ret = -EINVAL;
>          goto fail_unref;
>      }
> +    max_transfer = qemu_opt_get_size(opts, "max-transfer", 0);
> +    if (max_transfer < INT_MAX &&
> +        QEMU_IS_ALIGNED(max_transfer, MAX(align, BDRV_SECTOR_SIZE))) {
> +        s->max_transfer = max_transfer;
> +    } else if (max_transfer) {
> +        error_setg(errp, "Invalid argument");

Could you be more specific? Same in all of the error_setg() calls below.

> +        ret = -EINVAL;
> +        goto fail_unref;
> +    }

Also, the way this is formatted seems not intuitive to me. I know it's
the same as it's been done for "align", but normally I'd use the following:

s->value = qemu_opt_get_size(...);
if (s->value is set and invalid) {
    /* error out */
}

Instead of:

value = qemu_opt_get_size(...);
if (value is valid) {
    s->value = value;
} else if (value is set) {
    /* error out */
}

Same for all blocks below.

Finally, my eyes would be really grateful for some empty lines here and
there, at least one between the handling of different options.


Max

> +    opt_write_zero = qemu_opt_get_size(opts, "opt-write-zero", 0);
> +    if (opt_write_zero < INT_MAX &&
> +        QEMU_IS_ALIGNED(opt_write_zero, MAX(align, BDRV_SECTOR_SIZE))) {
> +        s->opt_write_zero = opt_write_zero;
> +    } else if (opt_write_zero) {
> +        error_setg(errp, "Invalid argument");
> +        ret = -EINVAL;
> +        goto fail_unref;
> +    }
> +    max_write_zero = qemu_opt_get_size(opts, "max-write-zero", 0);
> +    if (max_write_zero < INT_MAX &&
> +        QEMU_IS_ALIGNED(max_write_zero, MAX(opt_write_zero,
> +                                         MAX(align, BDRV_SECTOR_SIZE)))) {
> +        s->max_write_zero = max_write_zero;
> +    } else if (max_write_zero) {
> +        error_setg(errp, "Invalid argument");
> +        ret = -EINVAL;
> +        goto fail_unref;
> +    }
> +    opt_discard = qemu_opt_get_size(opts, "opt-discard", 0);
> +    if (opt_discard < INT_MAX &&
> +        QEMU_IS_ALIGNED(opt_discard, MAX(align, BDRV_SECTOR_SIZE))) {
> +        s->opt_discard = opt_discard;
> +    } else if (opt_discard) {
> +        error_setg(errp, "Invalid argument");
> +        ret = -EINVAL;
> +        goto fail_unref;
> +    }
> +    max_discard = qemu_opt_get_size(opts, "max-discard", 0);
> +    if (max_discard < INT_MAX &&
> +        QEMU_IS_ALIGNED(max_discard, MAX(opt_discard,
> +                                         MAX(align, BDRV_SECTOR_SIZE)))) {
> +        s->max_discard = max_discard;
> +    } else if (max_discard) {
> +        error_setg(errp, "Invalid argument");
> +        ret = -EINVAL;
> +        goto fail_unref;
> +    }
> 
>      ret = 0;
>      goto out;
> @@ -807,6 +885,21 @@ static void blkdebug_refresh_limits(BlockDriverState 
> *bs, Error **errp)
>      if (s->align) {
>          bs->bl.request_alignment = s->align;
>      }
> +    if (s->max_transfer) {
> +        bs->bl.max_transfer = s->max_transfer;
> +    }
> +    if (s->opt_write_zero) {
> +        bs->bl.pwrite_zeroes_alignment = s->opt_write_zero;
> +    }
> +    if (s->max_write_zero) {
> +        bs->bl.max_pwrite_zeroes = s->max_write_zero;
> +    }
> +    if (s->opt_discard) {
> +        bs->bl.pdiscard_alignment = s->opt_discard;
> +    }
> +    if (s->max_discard) {
> +        bs->bl.max_pdiscard = s->max_discard;
> +    }
>  }
> 
>  static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
>

signature.asc
Description: OpenPGP digital signature