diff mbox

[v3,4/5] blkdebug: Add ability to override unmap geometries

Message ID 20161202192223.15153-5-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake Dec. 2, 2016, 7:22 p.m. UTC
Make it easier to simulate various unusual hardware setups (for
example, recent commits 3482b9b and b8d0a98 affect the Dell
Equallogic iSCSI with its 15M preferred and maximum unmap and
write zero sizing, or b2f95fe deals with the Linux loopback
block device having a 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>

---
v3: improve legibility of bounds checking, improve docs
v2: new patch
---
 qapi/block-core.json | 27 ++++++++++++++-
 block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 120 insertions(+), 3 deletions(-)

Comments

Max Reitz Dec. 6, 2016, 10:31 p.m. UTC | #1
On 02.12.2016 20:22, Eric Blake wrote:
> Make it easier to simulate various unusual hardware setups (for
> example, recent commits 3482b9b and b8d0a98 affect the Dell
> Equallogic iSCSI with its 15M preferred and maximum unmap and
> write zero sizing, or b2f95fe deals with the Linux loopback
> block device having a 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>
> 
> ---
> v3: improve legibility of bounds checking, improve docs
> v2: new patch
> ---
>  qapi/block-core.json | 27 ++++++++++++++-
>  block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 120 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
Kevin Wolf Dec. 7, 2016, 2:35 p.m. UTC | #2
Am 02.12.2016 um 20:22 hat Eric Blake geschrieben:
> Make it easier to simulate various unusual hardware setups (for
> example, recent commits 3482b9b and b8d0a98 affect the Dell
> Equallogic iSCSI with its 15M preferred and maximum unmap and
> write zero sizing, or b2f95fe deals with the Linux loopback
> block device having a 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>
> 
> ---
> v3: improve legibility of bounds checking, improve docs
> v2: new patch
> ---
>  qapi/block-core.json | 27 ++++++++++++++-
>  block/blkdebug.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 120 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

s/and and/and/

> +#                   (but need not be a power of 2), or 0 for default
> +#                   (since 2.9)

What is the reason for the 512 bytes restriction? Not really a problem,
though, allowing more values later is easier than restricting them.

The rest looks good.

Kevin
Eric Blake Dec. 7, 2016, 3:11 p.m. UTC | #3
On 12/07/2016 08:35 AM, Kevin Wolf wrote:

>> +++ 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
> 
> s/and and/and/
> 
>> +#                   (but need not be a power of 2), or 0 for default
>> +#                   (since 2.9)
> 
> What is the reason for the 512 bytes restriction?

Probably because I wrote the patch prior to your work to make blkdebug
byte-based, and then forgot to touch it up on rebase.

> Not really a problem,
> though, allowing more values later is easier than restricting them.

I'll see if I can drop the restriction, or at least improve the text,
for v4.

> 
> The rest looks good.
> 
> Kevin
>
diff mbox

Patch

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 edc2b05..45f565d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -39,6 +39,11 @@  typedef struct BDRVBlkdebugState {
     int state;
     int new_state;
     uint64_t align;
+    uint64_t max_transfer;
+    uint64_t opt_write_zero;
+    uint64_t max_write_zero;
+    uint64_t opt_discard;
+    uint64_t max_discard;

     /* For blkdebug_refresh_filename() */
     char *config_file;
@@ -343,6 +348,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 alignment in bytes",
+        },
+        {
+            .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 alignment in bytes",
+        },
+        {
+            .name = "max-discard",
+            .type = QEMU_OPT_SIZE,
+            .help = "Maximum discard size in bytes",
+        },
         { /* end of list */ }
     },
 };
@@ -387,11 +417,57 @@  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 */
     s->align = qemu_opt_get_size(opts, "align", 0);
     if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
         error_setg(errp, "Invalid alignment, align must be integer power of 2");
-        ret = -EINVAL;
+        goto fail_unref;
+    }
+
+    s->max_transfer = qemu_opt_get_size(opts, "max-transfer", 0);
+    if (s->max_transfer &&
+        (s->max_transfer >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_transfer,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid max-transfer, must be multiple of align");
+        goto fail_unref;
+    }
+
+    s->opt_write_zero = qemu_opt_get_size(opts, "opt-write-zero", 0);
+    if (s->opt_write_zero &&
+        (s->opt_write_zero >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->opt_write_zero,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid opt-write-zero, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->max_write_zero = qemu_opt_get_size(opts, "max-write-zero", 0);
+    if (s->max_write_zero &&
+        (s->max_write_zero >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_write_zero,
+                          MAX(s->opt_write_zero,
+                              MAX(s->align, BDRV_SECTOR_SIZE))))) {
+        error_setg(errp, "Invalid max-write-zero, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->opt_discard = qemu_opt_get_size(opts, "opt-discard", 0);
+    if (s->opt_discard &&
+        (s->opt_discard >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->opt_discard,
+                          MAX(s->align, BDRV_SECTOR_SIZE)))) {
+        error_setg(errp, "Invalid opt-discard, not consistent with align");
+        goto fail_unref;
+    }
+
+    s->max_discard = qemu_opt_get_size(opts, "max-discard", 0);
+    if (s->max_discard &&
+        (s->max_discard >= INT_MAX ||
+         !QEMU_IS_ALIGNED(s->max_discard,
+                          MAX(s->opt_discard,
+                              MAX(s->align, BDRV_SECTOR_SIZE))))) {
+        error_setg(errp, "Invalid max-discard, not consistent with align");
         goto fail_unref;
     }

@@ -399,6 +475,7 @@  static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     goto out;

 fail_unref:
+    ret = -EINVAL;
     bdrv_unref_child(bs, bs->file);
 out:
     if (ret < 0) {
@@ -814,6 +891,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,