[PATCH v3 0/3] testing block device blocksizes

[PATCH v3 0/3] testing block device blocksizes

[Kit Westneat]

These patches add a few parameters to blkdebug to allow modification of
the block device block sizes, both logical and physical. It also adds a
test that uses the parameter to verify correct UNMAP behavior in devices
with 4k blocks.

v2: fixes style issues
v3: adds parameters to qapi

Kit Westneat (3):
  block/blkdebug: add blocksize parameter
  tests/qtest/virtio-scsi-test: add unmap large LBA with 4k blocks test
  block/blkdebug: add log-blocksize and phys-blocksize parameters

 block/blkdebug.c               | 56 ++++++++++++++++++++++++++++++++++
 qapi/block-core.json           | 11 +++++++
 tests/qtest/virtio-scsi-test.c | 50 ++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

-- 
2.26.3

[PATCH v3 1/3] block/blkdebug: add blocksize parameter

[Kit Westneat]

Allow users to specify the block size of the qdev for testing purposes.

Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
---
 block/blkdebug.c     | 29 +++++++++++++++++++++++++++++
 qapi/block-core.json |  4 ++++
 2 files changed, 33 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2c0b9b0ee8..d5f589920c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -47,6 +47,7 @@ typedef struct BDRVBlkdebugState {
     uint64_t max_write_zero;
     uint64_t opt_discard;
     uint64_t max_discard;
+    uint64_t blocksize;
 
     uint64_t take_child_perms;
     uint64_t unshare_child_perms;
@@ -455,6 +456,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_SIZE,
             .help = "Maximum discard size in bytes",
         },
+        {
+            .name = "blocksize",
+            .type = QEMU_OPT_SIZE,
+            .help = "Blocksize of device",
+        },
         { /* end of list */ }
     },
 };
@@ -562,6 +568,14 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
+    s->blocksize = qemu_opt_get_size(opts, "blocksize", 512);
+    if (s->blocksize && (s->blocksize >= INT_MAX ||
+        !is_power_of_2(s->blocksize))) {
+        error_setg(errp, "Cannot meet constraints with blocksize %" PRIu64,
+                   s->blocksize);
+        goto out;
+    }
+
     bdrv_debug_event(bs, BLKDBG_NONE);
 
     ret = 0;
@@ -984,6 +998,19 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+static int blkdebug_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+
+    if (!s->blocksize) {
+        return 0;
+    }
+
+    bsz->phys = s->blocksize;
+    bsz->log = s->blocksize;
+    return 0;
+}
+
 static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
                                    BlockReopenQueue *queue, Error **errp)
 {
@@ -1010,6 +1037,7 @@ static const char *const blkdebug_strong_runtime_opts[] = {
     "inject-error.",
     "set-state.",
     "align",
+    "blocksize",
     "max-transfer",
     "opt-write-zero",
     "max-write-zero",
@@ -1034,6 +1062,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_getlength         = blkdebug_getlength,
     .bdrv_refresh_filename  = blkdebug_refresh_filename,
     .bdrv_refresh_limits    = blkdebug_refresh_limits,
+    .bdrv_probe_blocksizes  = blkdebug_probe_blocksizes,
 
     .bdrv_co_preadv         = blkdebug_co_preadv,
     .bdrv_co_pwritev        = blkdebug_co_pwritev,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2ea294129e..1b1f2692ef 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3404,6 +3404,9 @@
 #                       on how the blkdebug node is used).  Defaults
 #                       to none.  (since 5.0)
 #
+# @blocksize: blocksize of device in bytes, must be
+#             positive power of 2 (since 6.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
@@ -3412,6 +3415,7 @@
             '*align': 'int', '*max-transfer': 'int32',
             '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
             '*opt-discard': 'int32', '*max-discard': 'int32',
+            '*blocksize': 'int',
             '*inject-error': ['BlkdebugInjectErrorOptions'],
             '*set-state': ['BlkdebugSetStateOptions'],
             '*take-child-perms': ['BlockPermission'],
-- 
2.26.3

[PATCH v3 2/3] tests/qtest/virtio-scsi-test: add unmap large LBA with 4k blocks test

[Kit Westneat]

Add test for issue #345

Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
---
 tests/qtest/virtio-scsi-test.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index 1b7ecc1c8f..e569bda7d0 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -200,6 +200,42 @@ static void test_unaligned_write_same(void *obj, void *data,
     qvirtio_scsi_pci_free(vs);
 }
 
+/* Test UNMAP with a large LBA, issue #345 */
+static void test_unmap_large_lba(void *obj, void *data,
+                                      QGuestAllocator *t_alloc)
+{
+    QVirtioSCSI *scsi = obj;
+    QVirtioSCSIQueues *vs;
+    const uint8_t unmap[VIRTIO_SCSI_CDB_SIZE] = {
+        0x42, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00
+    };
+
+    /*
+     * Default null-co device size is 2**30
+     * LBA 0x7fff is ~ 1/8 into device, with 4k blocks
+     * if check_lba_range incorrectly using 512 bytes, will trigger sense error
+     */
+    uint8_t unmap_params[0x18] = {
+        0x00, 0x16, /* unmap data length */
+        0x00, 0x10, /* unmap block descriptor data length */
+        0x00, 0x00, 0x00, 0x00, /* reserved */
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x7f, 0xff, /* LBA */
+        0x00, 0x00, 0x03, 0xff, /* sector count */
+        0x00, 0x00, 0x00, 0x00, /* reserved */
+    };
+    struct virtio_scsi_cmd_resp resp;
+
+    alloc = t_alloc;
+    vs = qvirtio_scsi_init(scsi->vdev);
+
+    virtio_scsi_do_command(vs, unmap, NULL, 0, unmap_params,
+                           sizeof(unmap_params), &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, !=, CHECK_CONDITION);
+
+    qvirtio_scsi_pci_free(vs);
+}
+
 static void test_write_to_cdrom(void *obj, void *data,
                                 QGuestAllocator *t_alloc)
 {
@@ -293,6 +329,16 @@ static void *virtio_scsi_setup(GString *cmd_line, void *arg)
     return arg;
 }
 
+static void *virtio_scsi_setup_4k(GString *cmd_line, void *arg)
+{
+    g_string_append(cmd_line,
+                    " -drive file=blkdebug::null-co://,"
+                    "file.image.read-zeroes=on,"
+                    "if=none,id=dr1,format=raw,file.blocksize=4k "
+                    "-device scsi-hd,drive=dr1,lun=0,scsi-id=1");
+    return arg;
+}
+
 static void *virtio_scsi_setup_cd(GString *cmd_line, void *arg)
 {
     g_string_append(cmd_line,
@@ -323,6 +369,10 @@ static void register_virtio_scsi_test(void)
     qos_add_test("unaligned-write-same", "virtio-scsi",
                  test_unaligned_write_same, &opts);
 
+    opts.before = virtio_scsi_setup_4k;
+    qos_add_test("large-lba-unmap", "virtio-scsi",
+                 test_unmap_large_lba, &opts);
+
     opts.before = virtio_scsi_setup_cd;
     qos_add_test("write-to-cdrom", "virtio-scsi", test_write_to_cdrom, &opts);
 
-- 
2.26.3

[PATCH v3 3/3] block/blkdebug: add log-blocksize and phys-blocksize parameters

[Kit Westneat]

Allow users to specify the logical and physical block sizes of the
qdev for testing purposes.

Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
---
 block/blkdebug.c     | 39 +++++++++++++++++++++++++++++++++------
 qapi/block-core.json |  7 +++++++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d5f589920c..85b3973427 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -48,6 +48,8 @@ typedef struct BDRVBlkdebugState {
     uint64_t opt_discard;
     uint64_t max_discard;
     uint64_t blocksize;
+    uint64_t phys_blocksize;
+    uint64_t log_blocksize;
 
     uint64_t take_child_perms;
     uint64_t unshare_child_perms;
@@ -459,7 +461,17 @@ static QemuOptsList runtime_opts = {
         {
             .name = "blocksize",
             .type = QEMU_OPT_SIZE,
-            .help = "Blocksize of device",
+            .help = "Blocksize of device (512 default)",
+        },
+        {
+            .name = "phys-blocksize",
+            .type = QEMU_OPT_SIZE,
+            .help = "Physical blocksize of device (Defaults to 'blocksize')",
+        },
+        {
+            .name = "log-blocksize",
+            .type = QEMU_OPT_SIZE,
+            .help = "Logical blocksize of device (Defaults to 'blocksize')",
         },
         { /* end of list */ }
     },
@@ -576,6 +588,22 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
+    s->phys_blocksize = qemu_opt_get_size(opts, "phys-blocksize", 0);
+    if (s->phys_blocksize && (s->phys_blocksize >= INT_MAX ||
+        !is_power_of_2(s->phys_blocksize))) {
+        error_setg(errp, "Cannot meet constraints with phys-blocksize %" PRIu64,
+                   s->phys_blocksize);
+        goto out;
+    }
+
+    s->log_blocksize = qemu_opt_get_size(opts, "log-blocksize", 0);
+    if (s->log_blocksize && (s->log_blocksize >= INT_MAX ||
+        !is_power_of_2(s->log_blocksize))) {
+        error_setg(errp, "Cannot meet constraints with log-blocksize %" PRIu64,
+                   s->log_blocksize);
+        goto out;
+    }
+
     bdrv_debug_event(bs, BLKDBG_NONE);
 
     ret = 0;
@@ -1002,12 +1030,9 @@ static int blkdebug_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 {
     BDRVBlkdebugState *s = bs->opaque;
 
-    if (!s->blocksize) {
-        return 0;
-    }
+    bsz->phys = s->phys_blocksize ? s->phys_blocksize : s->blocksize;
+    bsz->log = s->log_blocksize ? s->log_blocksize : s->blocksize;
 
-    bsz->phys = s->blocksize;
-    bsz->log = s->blocksize;
     return 0;
 }
 
@@ -1038,6 +1063,8 @@ static const char *const blkdebug_strong_runtime_opts[] = {
     "set-state.",
     "align",
     "blocksize",
+    "phys-blocksize",
+    "log-blocksize",
     "max-transfer",
     "opt-write-zero",
     "max-write-zero",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1b1f2692ef..4638026dbf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3407,6 +3407,12 @@
 # @blocksize: blocksize of device in bytes, must be
 #             positive power of 2 (since 6.1)
 #
+# @log-blocksize: logical blocksize of device in bytes, must be positive power
+#                 of 2, or 0 for default (@blocksize) (since 6.1)
+#
+# @phys-blocksize: physical blocksize of device in bytes, must be positive
+#                  power of 2, or 0 for default (@blocksize) (since 6.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
@@ -3416,6 +3422,7 @@
             '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
             '*opt-discard': 'int32', '*max-discard': 'int32',
             '*blocksize': 'int',
+            '*log-blocksize': 'int', '*phys-blocksize': 'int',
             '*inject-error': ['BlkdebugInjectErrorOptions'],
             '*set-state': ['BlkdebugSetStateOptions'],
             '*take-child-perms': ['BlockPermission'],
-- 
2.26.3

Re: [PATCH v3 2/3] tests/qtest/virtio-scsi-test: add unmap large LBA with 4k blocks test

[Paolo Bonzini]

On 25/05/21 21:46, Kit Westneat wrote:
> +static void *virtio_scsi_setup_4k(GString *cmd_line, void *arg)
> +{
> +    g_string_append(cmd_line,
> +                    " -drive file=blkdebug::null-co://,"
> +                    "file.image.read-zeroes=on,"
> +                    "if=none,id=dr1,format=raw,file.blocksize=4k "
> +                    "-device scsi-hd,drive=dr1,lun=0,scsi-id=1");

Instead of having patches 1+3, I think it's enough to use "-device 
scsi-hd,...,logical_block_size=4096".

Thanks,

Paolo