[PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0000000000000400  blocks: 32  deallocated

Lin Ma (4):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  block: Add block accounting code for GET LBA STATUS
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c          | 38 ++++++++++++++
 block/io.c                     | 43 ++++++++++++++++
 hw/scsi/scsi-disk.c            | 92 ++++++++++++++++++++++++++++++++++
 include/block/accounting.h     |  1 +
 include/scsi/constants.h       |  1 +
 include/sysemu/block-backend.h |  2 +
 6 files changed, 177 insertions(+)

-- 
2.24.0

[PATCH 1/4] block: Add bdrv_co_get_lba_status

[Lin Ma]

The get lba status wrapper based on the bdrv_block_status. The following
patches will add GET LBA STATUS 16 support for scsi emulation layer.

Signed-off-by: Lin Ma <lma@suse.com>
---
 block/io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/block/io.c b/block/io.c
index 121ce17a49..dacc3c2471 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2186,6 +2186,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
                            BDRV_REQ_ZERO_WRITE | flags);
 }
 
+int coroutine_fn
+bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
+                       uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+    BlockDriverState *bs = child->bs;
+    int ret;
+    int64_t target_size, count = 0;
+    bool first = true;
+    uint8_t wanted_bit1 = 0;
+
+    target_size = bdrv_getlength(bs);
+    if (target_size < 0) {
+        return -EIO;
+    }
+
+    if (offset < 0 || bytes < 0) {
+        return -EIO;
+    }
+
+    for ( ; offset <= target_size - bytes; offset += count) {
+        ret = bdrv_block_status(bs, offset, bytes, &count, NULL, NULL);
+        if (ret < 0) {
+            goto out;
+        }
+        if (first) {
+            if (ret & BDRV_BLOCK_ZERO) {
+                wanted_bit1 = BDRV_BLOCK_ZERO >> 1;;
+                *is_deallocated = 1;
+            } else {
+                wanted_bit1 = 0;
+            }
+            first = false;
+        }
+        if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
+            (*num_blocks)++;
+        } else {
+            break;
+        }
+    }
+out:
+    return ret;
+}
+
 /*
  * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
  */
-- 
2.24.0

[PATCH 2/4] block: Add GET LBA STATUS support

[Lin Ma]

Signed-off-by: Lin Ma <lma@suse.com>
---
 block/block-backend.c          | 38 ++++++++++++++++++++++++++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..feb1f38b98 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1650,6 +1650,44 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
     return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
 }
 
+static int coroutine_fn
+blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+                      uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+    int ret;
+
+    blk_wait_while_drained(blk);
+
+    ret = blk_check_byte_request(blk, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
+                                  is_deallocated);
+}
+
+static void blk_aio_get_lba_status_entry(void *opaque)
+{
+    BlkAioEmAIOCB *acb = opaque;
+    BlkRwCo *rwco = &acb->rwco;
+
+    void *data = acb->common.opaque;
+    uint32_t *num_blocks = &data[0];
+    uint32_t *is_deallocated = &data[sizeof(uint32_t)];
+
+    rwco->ret = blk_do_get_lba_status(rwco->blk, rwco->offset, acb->bytes,
+                                      num_blocks, is_deallocated);
+    blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+                                   BlockCompletionFunc *cb, void *opaque)
+{
+    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_get_lba_status_entry,
+                        0, cb, opaque);
+}
+
 /* To be called between exactly one pair of blk_inc/dec_in_flight() */
 static int coroutine_fn blk_do_flush(BlockBackend *blk)
 {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..cd527ec0c9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,6 +171,8 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
                           BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
                              BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+                                   BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
-- 
2.24.0

[PATCH 3/4] block: Add block accounting code for GET LBA STATUS

[Lin Ma]

Signed-off-by: Lin Ma <lma@suse.com>
---
 include/block/accounting.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
     BLOCK_ACCT_WRITE,
     BLOCK_ACCT_FLUSH,
     BLOCK_ACCT_UNMAP,
+    BLOCK_ACCT_GET_LBA_STATUS,
     BLOCK_MAX_IOTYPE,
 };
 
-- 
2.24.0

[PATCH 4/4] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma <lma@suse.com>
---
 hw/scsi/scsi-disk.c      | 92 ++++++++++++++++++++++++++++++++++++++++
 include/scsi/constants.h |  1 +
 2 files changed, 93 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..2d2c6b4b82 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,91 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
     }
 }
 
+typedef struct GetLbaStatusCBData {
+    uint32_t num_blocks;
+    uint32_t is_deallocated;
+    SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
+{
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb == NULL);
+
+    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+                                          r->req.cmd.lba * s->qdev.blocksize,
+                                          s->qdev.blocksize,
+                                          scsi_get_lba_status_complete, data);
+    return;
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+    GetLbaStatusCBData *data = opaque;
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+
+    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+    if (scsi_disk_req_check_error(r, ret, true)) {
+        g_free(data);
+        goto done;
+    }
+
+    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+    scsi_req_unref(&r->req);
+    g_free(data);
+
+done:
+    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    GetLbaStatusCBData *data;
+    uint32_t *num_blocks;
+    uint32_t *is_deallocated;
+
+    data = g_new0(GetLbaStatusCBData, 1);
+    data->r = r;
+    num_blocks = &(data->num_blocks);
+    is_deallocated = &(data->is_deallocated);
+
+    scsi_req_ref(&r->req);
+    scsi_get_lba_status_complete_noio(data, 0);
+
+    /* 8 + 16 is the length in bytes of response header and
+     * one LBA status descriptor
+     */
+    memset(outbuf, 0, 8 + 16);
+    outbuf[3] = 20;
+    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+    outbuf[15] = req->cmd.lba & 0xff;
+    outbuf[16] = (*num_blocks >> 24) & 0xff;
+    outbuf[17] = (*num_blocks >> 16) & 0xff;
+    outbuf[18] = (*num_blocks >> 8) & 0xff;
+    outbuf[19] = *num_blocks & 0xff;
+    outbuf[20] = *is_deallocated ? 1 : 0;
+
+    return;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2161,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 
             /* Protection, exponent and lowest lba field left blank. */
             break;
+        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+            if (req->cmd.lba > s->qdev.max_lba) {
+                goto illegal_lba;
+            }
+            scsi_disk_emulate_get_lba_status(req, outbuf);
+            r->iov.iov_len = req->cmd.xfer;
+            return r->iov.iov_len;
         }
         trace_scsi_disk_emulate_command_SAI_unsupported();
         goto illegal_request;
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..1b6417898a 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS  0x12
 
 /*
  * READ POSITION service action codes
-- 
2.24.0

Re: [PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200602074201.10879-1-lma@suse.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/nvme.o
  CC      block/throttle-groups.o
/tmp/qemu-test/src/block/block-backend.c: In function 'blk_do_get_lba_status':
/tmp/qemu-test/src/block/block-backend.c:1666:5: error: implicit declaration of function 'bdrv_co_get_lba_status' [-Werror=implicit-function-declaration]
     return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
     ^
/tmp/qemu-test/src/block/block-backend.c:1666:5: error: nested extern declaration of 'bdrv_co_get_lba_status' [-Werror=nested-externs]
/tmp/qemu-test/src/block/block-backend.c: In function 'blk_aio_get_lba_status_entry':
/tmp/qemu-test/src/block/block-backend.c:1676:33: error: dereferencing 'void *' pointer [-Werror]
     uint32_t *num_blocks = &data[0];
                                 ^
/tmp/qemu-test/src/block/block-backend.c:1677:37: error: dereferencing 'void *' pointer [-Werror]
     uint32_t *is_deallocated = &data[sizeof(uint32_t)];
                                     ^
cc1: all warnings being treated as errors
make: *** [block/block-backend.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/io.c:2190:1: error: no previous prototype for 'bdrv_co_get_lba_status' [-Werror=missing-prototypes]
 bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
 ^
cc1: all warnings being treated as errors
make: *** [block/io.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 665, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=e0beb28f32564939b51782a3a5fc4b8e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3acrwdcl/src/docker-src.2020-06-02-03.50.11.11734:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e0beb28f32564939b51782a3a5fc4b8e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3acrwdcl/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m17.058s
user    0m8.502s


The full log is available at
http://patchew.org/logs/20200602074201.10879-1-lma@suse.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200602074201.10879-1-lma@suse.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200602074201.10879-1-lma@suse.com
Subject: [PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f21fd54 scsi-disk: Add support for the GET LBA STATUS 16 command
1cf515c block: Add block accounting code for GET LBA STATUS
fb4af56 block: Add GET LBA STATUS support
3e12b00 block: Add bdrv_co_get_lba_status

=== OUTPUT BEGIN ===
1/4 Checking commit 3e12b00c0380 (block: Add bdrv_co_get_lba_status)
ERROR: superfluous trailing semicolon
#47: FILE: block/io.c:2215:
+                wanted_bit1 = BDRV_BLOCK_ZERO >> 1;;

total: 1 errors, 0 warnings, 49 lines checked

Patch 1/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/4 Checking commit fb4af5679e29 (block: Add GET LBA STATUS support)
3/4 Checking commit 1cf515c04a31 (block: Add block accounting code for GET LBA STATUS)
4/4 Checking commit f21fd540cc03 (scsi-disk: Add support for the GET LBA STATUS 16 command)
WARNING: Block comments use a leading /* on a separate line
#81: FILE: hw/scsi/scsi-disk.c:1932:
+    /* 8 + 16 is the length in bytes of response header and

total: 0 errors, 1 warnings, 111 lines checked

Patch 4/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200602074201.10879-1-lma@suse.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200602074201.10879-1-lma@suse.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/nvme.o
  CC      block/nbd.o
  CC      block/sheepdog.o
/tmp/qemu-test/src/block/block-backend.c:1666:12: error: implicit declaration of function 'bdrv_co_get_lba_status' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
           ^
/tmp/qemu-test/src/block/block-backend.c:1666:12: note: did you mean 'blk_do_get_lba_status'?
/tmp/qemu-test/src/block/block-backend.c:1654:1: note: 'blk_do_get_lba_status' declared here
blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
^
/tmp/qemu-test/src/block/block-backend.c:1666:12: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
    return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
           ^
2 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/block-backend.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/io.c:2190:1: error: no previous prototype for function 'bdrv_co_get_lba_status' [-Werror,-Wmissing-prototypes]
bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
^
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: block/io.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 665, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=aaeff486206e49e6b5c30d0f96e9933a', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-sk0g3zzx/src/docker-src.2020-06-02-03.50.12.11895:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=aaeff486206e49e6b5c30d0f96e9933a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-sk0g3zzx/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m28.592s
user    0m8.333s


The full log is available at
http://patchew.org/logs/20200602074201.10879-1-lma@suse.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200602074201.10879-1-lma@suse.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/replication.o
  CC      block/throttle.o
/tmp/qemu-test/src/block/block-backend.c: In function 'blk_do_get_lba_status':
/tmp/qemu-test/src/block/block-backend.c:1666:12: error: implicit declaration of function 'bdrv_co_get_lba_status'; did you mean 'blk_do_get_lba_status'? [-Werror=implicit-function-declaration]
     return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
            ^~~~~~~~~~~~~~~~~~~~~~
            blk_do_get_lba_status
/tmp/qemu-test/src/block/block-backend.c:1666:12: error: nested extern declaration of 'bdrv_co_get_lba_status' [-Werror=nested-externs]
/tmp/qemu-test/src/block/block-backend.c: In function 'blk_aio_get_lba_status_entry':
/tmp/qemu-test/src/block/block-backend.c:1676:33: error: dereferencing 'void *' pointer [-Werror]
     uint32_t *num_blocks = &data[0];
                                 ^
/tmp/qemu-test/src/block/block-backend.c:1677:37: error: dereferencing 'void *' pointer [-Werror]
     uint32_t *is_deallocated = &data[sizeof(uint32_t)];
                                     ^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/block-backend.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/io.c:2190:1: error: no previous prototype for 'bdrv_co_get_lba_status' [-Werror=missing-prototypes]
 bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
 ^~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/block/io.c: In function 'bdrv_co_get_lba_status':
/tmp/qemu-test/src/block/io.c:2194:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/io.o] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 665, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=130ab752886a4d91808d25eef0fb2b19', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-g7c0p0bd/src/docker-src.2020-06-02-03.54.28.23905:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=130ab752886a4d91808d25eef0fb2b19
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-g7c0p0bd/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m5.434s
user    0m8.029s


The full log is available at
http://patchew.org/logs/20200602074201.10879-1-lma@suse.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/4] block: Add bdrv_co_get_lba_status

[Claudio Fontana]

Hi Lin,

just a couple of things, leaving the core aspects to more knowledgeable people in this area to address.

In general, you want all patches in the series to build correctly.

Applying only PATCH 1/4 breaks the build for me with:

/home/claudio/git/qemu/block/io.c:2190:1: error: no previous prototype for ‘bdrv_co_get_lba_status’ [-Werror=missing-prototypes]
 bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
 ^~~~~~~~~~~~~~~~~~~~~~
/home/claudio/git/qemu/block/io.c: In function ‘bdrv_co_get_lba_status’:
/home/claudio/git/qemu/block/io.c:2194:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^~~
cc1: all warnings being treated as errors

On 6/2/20 9:41 AM, Lin Ma wrote:
> The get lba status wrapper based on the bdrv_block_status. The following
> patches will add GET LBA STATUS 16 support for scsi emulation layer.
> 
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  block/io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 121ce17a49..dacc3c2471 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2186,6 +2186,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
>                             BDRV_REQ_ZERO_WRITE | flags);
>  }
>  
> +int coroutine_fn
> +bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
> +                       uint32_t *num_blocks, uint32_t *is_deallocated)
> +{
> +    BlockDriverState *bs = child->bs;
> +    int ret;
> +    int64_t target_size, count = 0;
> +    bool first = true;
> +    uint8_t wanted_bit1 = 0;
> +
> +    target_size = bdrv_getlength(bs);
> +    if (target_size < 0) {
> +        return -EIO;
> +    }
> +
> +    if (offset < 0 || bytes < 0) {
> +        return -EIO;
> +    }
> +
> +    for ( ; offset <= target_size - bytes; offset += count) {
> +        ret = bdrv_block_status(bs, offset, bytes, &count, NULL, NULL);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +        if (first) {
> +            if (ret & BDRV_BLOCK_ZERO) {
> +                wanted_bit1 = BDRV_BLOCK_ZERO >> 1;;

extra ; at the end of the line

> +                *is_deallocated = 1;
> +            } else {
> +                wanted_bit1 = 0;
> +            }
> +            first = false;
> +        }
> +        if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
> +            (*num_blocks)++;
> +        } else {
> +            break;
> +        }
> +    }
> +out:
> +    return ret;
> +}
> +
>  /*
>   * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
>   */
> 

Ciao,

CLaudio

Re: [PATCH 4/4] scsi-disk: Add support for the GET LBA STATUS 16 command

[Claudio Fontana]

On 6/2/20 9:42 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  hw/scsi/scsi-disk.c      | 92 ++++++++++++++++++++++++++++++++++++++++
>  include/scsi/constants.h |  1 +
>  2 files changed, 93 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..2d2c6b4b82 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1866,6 +1866,91 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
>      }
>  }
>  
> +typedef struct GetLbaStatusCBData {
> +    uint32_t num_blocks;
> +    uint32_t is_deallocated;
> +    SCSIDiskReq *r;
> +} GetLbaStatusCBData;
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret);
> +
> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
> +{
> +    SCSIDiskReq *r = data->r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +    assert(r->req.aiocb == NULL);
> +
> +    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> +                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
> +
> +    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> +                                          r->req.cmd.lba * s->qdev.blocksize,
> +                                          s->qdev.blocksize,
> +                                          scsi_get_lba_status_complete, data);
> +    return;

this return statement does not add anything of value, I think you can remove it.

> +}
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret)
> +{
> +    GetLbaStatusCBData *data = opaque;
> +    SCSIDiskReq *r = data->r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +    assert(r->req.aiocb != NULL);
> +    r->req.aiocb = NULL;
> +
> +    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> +    if (scsi_disk_req_check_error(r, ret, true)) {
> +        g_free(data);
> +        goto done;
> +    }
> +
> +    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
> +    scsi_req_unref(&r->req);
> +    g_free(data);
> +
> +done:
> +    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> +}
> +
> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
> +{
> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +    GetLbaStatusCBData *data;
> +    uint32_t *num_blocks;
> +    uint32_t *is_deallocated;
> +
> +    data = g_new0(GetLbaStatusCBData, 1);
> +    data->r = r;
> +    num_blocks = &(data->num_blocks);
> +    is_deallocated = &(data->is_deallocated);
> +
> +    scsi_req_ref(&r->req);
> +    scsi_get_lba_status_complete_noio(data, 0);
> +
> +    /* 8 + 16 is the length in bytes of response header and
> +     * one LBA status descriptor
> +     */

this should probably look like:

/*
 * 8 + 16 ...
 * one LBA ...
 */

> +    memset(outbuf, 0, 8 + 16);
> +    outbuf[3] = 20;
> +    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> +    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> +    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> +    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> +    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> +    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> +    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> +    outbuf[15] = req->cmd.lba & 0xff;
> +    outbuf[16] = (*num_blocks >> 24) & 0xff;
> +    outbuf[17] = (*num_blocks >> 16) & 0xff;
> +    outbuf[18] = (*num_blocks >> 8) & 0xff;
> +    outbuf[19] = *num_blocks & 0xff;
> +    outbuf[20] = *is_deallocated ? 1 : 0;
> +
> +    return;

(see above)

> +}
> +
>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>  {
>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> @@ -2076,6 +2161,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>  
>              /* Protection, exponent and lowest lba field left blank. */
>              break;
> +        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> +            if (req->cmd.lba > s->qdev.max_lba) {
> +                goto illegal_lba;
> +            }
> +            scsi_disk_emulate_get_lba_status(req, outbuf);
> +            r->iov.iov_len = req->cmd.xfer;
> +            return r->iov.iov_len;
>          }
>          trace_scsi_disk_emulate_command_SAI_unsupported();
>          goto illegal_request;
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..1b6417898a 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -154,6 +154,7 @@
>   * SERVICE ACTION IN subcodes
>   */
>  #define SAI_READ_CAPACITY_16  0x10
> +#define SAI_GET_LBA_STATUS  0x12
>  
>  /*
>   * READ POSITION service action codes
> 

Re: [PATCH 3/4] block: Add block accounting code for GET LBA STATUS

[Claudio Fontana]

On 6/2/20 9:42 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  include/block/accounting.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 878b4c3581..645014fb0b 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -38,6 +38,7 @@ enum BlockAcctType {
>      BLOCK_ACCT_WRITE,
>      BLOCK_ACCT_FLUSH,
>      BLOCK_ACCT_UNMAP,
> +    BLOCK_ACCT_GET_LBA_STATUS,
>      BLOCK_MAX_IOTYPE,
>  };
>  
> 

This should probably be combined with the change that makes use of BLOCK_ACCT_GET_LBA_STATUS to constitute a single, meaningful change.

Ciao,

Claudio