[v2,8/9] blkdebug: Add ability to override unmap geometries

Make it easier to simulate the Dell Equallogic iSCSI with its
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 <eblake@redhat.com>
---
 qapi/block-core.json | 27 ++++++++++++++-
 block/blkdebug.c     | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 121 insertions(+), 3 deletions(-)

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 <eblake@redhat.com>
> ---
>  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,
>

On 11/17/2016 05:02 PM, Max Reitz wrote:
> 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...

Not the first time I've done it - see commit b8d0a980.

Keeping it in the commit message is one thing (so I probably won't
change this one), but having it in the iotest is another (so I probably
will rework the comments in 9/9 to avoid the specific mention).

> 
>> 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.
>>

>> +        {
>> +            .name = "opt-write-zero",
>> +            .type = QEMU_OPT_SIZE,
>> +            .help = "Optimum write zero size in bytes",
> 
> s/size/alignment/?

Yes, will fix.

>> @@ -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 */
> }

I'll see what I can do.

On 11/21/2016 03:11 PM, Eric Blake wrote:

>>> @@ -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 */
>> }
> 
> I'll see what I can do.

Unfortunately, part of the problem is type casting.  qemu_opt_get_size()
returns a 64-bit number, but s->align is 'int'. You can't detect
wraparound unless you store into a temporary and check bounds prior to
assigning to the narrower type.  I guess I could always change the
struct to store 64-bit values that have been validated to fit within 32
bits.