* [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
@ 2020-06-17 10:30 Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Lin Ma @ 2020-06-17 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, mreitz, stefanha, Lin Ma, pbonzini
v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.
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 (3):
block: Add bdrv_co_get_lba_status
block: Add GET LBA STATUS support
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 | 90 ++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/block/block_int.h | 5 ++
include/scsi/constants.h | 1 +
include/sysemu/block-backend.h | 2 +
7 files changed, 180 insertions(+)
--
2.26.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-22 10:25 ` Stefan Hajnoczi
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Lin Ma @ 2020-06-17 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, mreitz, stefanha, Lin Ma, pbonzini
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 +++++++++++++++++++++++++++++++++++++++
include/block/block_int.h | 5 +++++
2 files changed, 48 insertions(+)
diff --git a/block/io.c b/block/io.c
index df8f2a98d4..2064016b19 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2208,6 +2208,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 = 0;
+ 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.
*/
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c..43f90591b9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1296,6 +1296,11 @@ int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
int64_t *pnum,
int64_t *map,
BlockDriverState **file);
+int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
+ int64_t offset,
+ int64_t bytes,
+ uint32_t *num_blocks,
+ uint32_t *is_deallocated);
const char *bdrv_get_parent_name(const BlockDriverState *bs);
void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
bool blk_dev_has_removable_media(BlockBackend *blk);
--
2.26.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] block: Add GET LBA STATUS support
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 0 replies; 16+ messages in thread
From: Lin Ma @ 2020-06-17 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, mreitz, stefanha, Lin Ma, pbonzini
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..6d08dd5e0d 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 = (uint32_t *)data;
+ uint32_t *is_deallocated = (uint32_t *)(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.26.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
@ 2020-06-17 10:30 ` Lin Ma
2020-06-22 12:14 ` Stefan Hajnoczi
2020-06-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
3 siblings, 1 reply; 16+ messages in thread
From: Lin Ma @ 2020-06-17 10:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, mreitz, stefanha, Lin Ma, pbonzini
Signed-off-by: Lin Ma <lma@suse.com>
---
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/scsi/constants.h | 1 +
3 files changed, 92 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ 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);
+}
+
+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;
+}
+
static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,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/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,
};
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 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.26.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
` (2 preceding siblings ...)
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-17 11:14 ` no-reply
3 siblings, 0 replies; 16+ messages in thread
From: no-reply @ 2020-06-17 11:14 UTC (permalink / raw)
To: lma; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, lma, pbonzini
Patchew URL: https://patchew.org/QEMU/20200617103018.18026-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 ===
GEN docs/interop/qemu-qmp-ref.txt
GEN docs/interop/qemu-qmp-ref.7
CC qga/commands.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
CC qga/guest-agent-command-state.o
CC qga/main.o
CC qga/commands-posix.o
---
GEN docs/interop/qemu-ga-ref.html
GEN docs/interop/qemu-ga-ref.txt
GEN docs/interop/qemu-ga-ref.7
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/multiboot.o
AS pc-bios/optionrom/linuxboot.o
CC pc-bios/optionrom/linuxboot_dma.o
LINK qemu-keymap
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/kvmvapic.o
LINK ivshmem-client
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/pvh.o
CC pc-bios/optionrom/pvh_main.o
BUILD pc-bios/optionrom/multiboot.img
---
BUILD pc-bios/optionrom/kvmvapic.img
BUILD pc-bios/optionrom/multiboot.raw
LINK qemu-storage-daemon
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
BUILD pc-bios/optionrom/linuxboot.raw
BUILD pc-bios/optionrom/linuxboot_dma.raw
BUILD pc-bios/optionrom/kvmvapic.raw
BUILD pc-bios/optionrom/pvh.img
SIGN pc-bios/optionrom/multiboot.bin
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
SIGN pc-bios/optionrom/linuxboot.bin
SIGN pc-bios/optionrom/linuxboot_dma.bin
SIGN pc-bios/optionrom/kvmvapic.bin
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
LINK qemu-io
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-edid
LINK fsdev/virtfs-proxy-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK scsi/qemu-pr-helper
LINK qemu-bridge-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK virtiofsd
LINK vhost-user-input
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-ga
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-img
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN x86_64-softmmu/hmp-commands-info.h
GEN x86_64-softmmu/hmp-commands.h
GEN x86_64-softmmu/config-devices.h
---
CC x86_64-softmmu/accel/tcg/tcg-runtime-gvec.o
CC x86_64-softmmu/accel/tcg/cpu-exec.o
CC x86_64-softmmu/accel/tcg/cpu-exec-common.o
/tmp/qemu-test/src/fpu/softfloat.c:3365:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3423:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~ ( ( (uint64_t) ( absZ1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3483:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~(((uint64_t)(absZ1<<1) == 0) & roundNearestEven);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3606:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3760:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x200 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3987:21: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4003:22: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig0 &= ~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4273:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig1 &= ~ ( ( zSig2 + zSig2 == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
8 errors generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: fpu/softfloat.o] Error 1
make[1]: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/migration/ram.c:919:45: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
xbzrle_counters.encoding_rate = UINT64_MAX;
~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make: *** [Makefile:527: x86_64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3b000e4674de454ca1785efd680d2175', '-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-2shrqpbq/src/docker-src.2020-06-17-07.09.45.12034:/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=3b000e4674de454ca1785efd680d2175
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2shrqpbq/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 4m33.543s
user 0m7.633s
The full log is available at
http://patchew.org/logs/20200617103018.18026-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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
@ 2020-06-22 10:25 ` Stefan Hajnoczi
0 siblings, 0 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2020-06-22 10:25 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, pbonzini
[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]
On Wed, Jun 17, 2020 at 06:30:16PM +0800, 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 +++++++++++++++++++++++++++++++++++++++
> include/block/block_int.h | 5 +++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/block/io.c b/block/io.c
> index df8f2a98d4..2064016b19 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2208,6 +2208,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)
Missing doc comments.
> +{
> + BlockDriverState *bs = child->bs;
Why does this function take a BdrvChild argument instead of
BlockDriverState? Most I/O functions take BlockDriverState.
> + int ret = 0;
> + 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;
This is a boolean. Please use bool instead of uint32_t.
Please initialize is_deallocated to false at the beginning of the
function to avoid accidental uninitialized variable accesses in the
caller.
> + } else {
> + wanted_bit1 = 0;
> + }
> + first = false;
> + }
> + if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
> + (*num_blocks)++;
If there is a long span of allocated/deallocated blocks then this
function only increments num_blocks once without counting the number of
blocks. I expected something like num_blocks += pnum / block_size. What
is the relationship between bytes, count, and blocks in this function?
> + } else {
> + break;
> + }
> + }
> +out:
> + return ret;
> +}
> +
> /*
> * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
> */
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 791de6a59c..43f90591b9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1296,6 +1296,11 @@ int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs,
> int64_t *pnum,
> int64_t *map,
> BlockDriverState **file);
> +int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
> + int64_t offset,
> + int64_t bytes,
> + uint32_t *num_blocks,
> + uint32_t *is_deallocated);
Should this function be in include/block/block.h (the public API) so
that any part of QEMU can call it? It's not an internal API.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-22 12:14 ` Stefan Hajnoczi
2020-06-29 9:18 ` 回复: " Lin Ma
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Hajnoczi @ 2020-06-22 12:14 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, pbonzini, qemu-devel, mreitz
[-- Attachment #1: Type: text/plain, Size: 6163 bytes --]
On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
> hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
> include/block/accounting.h | 1 +
> include/scsi/constants.h | 1 +
> 3 files changed, 92 insertions(+)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..9e3002ddaf 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1866,6 +1866,89 @@ 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);
> +}
> +
> +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);
scsi_get_lba_status_complete_noio() looks asynchronous. If the
BlockDriver yields in .bdrv_co_block_status() then the operation has not
completed yet when scsi_get_lba_status_complete_noio() returns. It is
not safe to access the GetLbaStatusCBData data until the async operation
is complete.
Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
use-after-free when *num_blocks and *is_deallocated are accessed.
These issues can be solved by making this code asynchronous (similar to
read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
the completion function before g_free(data) is called.
> +
> + /*
> + * 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;
SCSI defines 3 values and QEMU can represent all of them:
0 - mapped or unknown
1 - deallocated
2 - anchored
See the BDRV_BLOCK_* constants in include/block/block.h. The
is_deallocated boolean is not enough to represent this state, but the
bdrv_block_status() return value can be used instead.
> +}
> +
> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> {
> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> @@ -2076,6 +2159,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;
Is there something tricky going on here with iov_len that prevents us
from using break here and sharing the functions normal return code path?
> }
> trace_scsi_disk_emulate_command_SAI_unsupported();
> goto illegal_request;
> 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,
> };
>
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..b18377b214 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.26.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-22 12:14 ` Stefan Hajnoczi
@ 2020-06-29 9:18 ` Lin Ma
2020-07-08 12:29 ` Stefan Hajnoczi
0 siblings, 1 reply; 16+ messages in thread
From: Lin Ma @ 2020-06-29 9:18 UTC (permalink / raw)
To: 'Stefan Hajnoczi'
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'pbonzini@redhat.com', 'qemu-devel@nongnu.org',
'mreitz@redhat.com'
> -----邮件原件-----
> 发件人: Stefan Hajnoczi <stefanha@redhat.com>
> 发送时间: 2020年6月22日 20:14
> 收件人: Lin Ma <LMa@suse.com>
> 抄送: qemu-devel@nongnu.org; fam@euphon.net; kwolf@redhat.com;
> mreitz@redhat.com; pbonzini@redhat.com
> 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> command
>
> On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> > Signed-off-by: Lin Ma <lma@suse.com>
> > ---
> > hw/scsi/scsi-disk.c | 90
> ++++++++++++++++++++++++++++++++++++++
> > include/block/accounting.h | 1 +
> > include/scsi/constants.h | 1 +
> > 3 files changed, 92 insertions(+)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index
> > 387503e11b..9e3002ddaf 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -1866,6 +1866,89 @@ 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); }
> > +
> > +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);
>
> scsi_get_lba_status_complete_noio() looks asynchronous. If the BlockDriver
> yields in .bdrv_co_block_status() then the operation has not completed yet
> when scsi_get_lba_status_complete_noio() returns. It is not safe to access the
> GetLbaStatusCBData data until the async operation is complete.
>
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
Got it, I'll fill the outbuf[] in the completion function in V3.
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in the completion
> function before g_free(data) is called.
I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' and the
'bdrv_co_flush()', They look similiar, They called corresponding bs->drv->bdrv_co_*()
or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or bs->drv->bdrv_aio_pdiscard()
The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or bs->drv->bdrv_aio_flush().
The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains the
coroutine version of block_status, no aio version of block_status.
About "making this code asynchronous", Well, In fact I havn't realized yet where the issue is.
If what you mean is that make the 'bdrv_co_get_lba_status()' asynchronous, How about
directly calling coroutine-based 'bdrv_co_block_status()' instead of 'bdrv_block_status()' in it?
Or could you please suggest more detailed information?
BTW, IMO the existing BlockDriver->bdrv_co_block_status() is enough, It's not necessary to
implement a drv->bdrv_aio_get_block_status() in BlockDrivers(say qcow2 or raw), Am I right?
I'm not familiar with qemu block layer and coroutine, Sorry for any inconvenience.
> > +
> > + /*
> > + * 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;
>
> SCSI defines 3 values and QEMU can represent all of them:
>
> 0 - mapped or unknown
> 1 - deallocated
> 2 - anchored
>
> See the BDRV_BLOCK_* constants in include/block/block.h. The is_deallocated
> boolean is not enough to represent this state, but the
> bdrv_block_status() return value can be used instead.
>
> > +}
> > +
> > static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t
> > *buf) {
> > SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req); @@ -2076,6
> > +2159,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;
>
> Is there something tricky going on here with iov_len that prevents us from using
> break here and sharing the functions normal return code path?
>
> > }
> > trace_scsi_disk_emulate_command_SAI_unsupported();
> > goto illegal_request;
> > 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,
> > };
> >
> > diff --git a/include/scsi/constants.h b/include/scsi/constants.h index
> > 874176019e..b18377b214 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.26.0
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 9:18 ` 回复: " Lin Ma
@ 2020-07-08 12:29 ` Stefan Hajnoczi
2020-07-08 12:38 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Hajnoczi @ 2020-07-08 12:29 UTC (permalink / raw)
To: Lin Ma
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'pbonzini@redhat.com', 'qemu-devel@nongnu.org',
'mreitz@redhat.com'
[-- Attachment #1: Type: text/plain, Size: 7345 bytes --]
On Mon, Jun 29, 2020 at 09:18:59AM +0000, Lin Ma wrote:
>
>
> > -----邮件原件-----
> > 发件人: Stefan Hajnoczi <stefanha@redhat.com>
> > 发送时间: 2020年6月22日 20:14
> > 收件人: Lin Ma <LMa@suse.com>
> > 抄送: qemu-devel@nongnu.org; fam@euphon.net; kwolf@redhat.com;
> > mreitz@redhat.com; pbonzini@redhat.com
> > 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> > command
> >
> > On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> > > Signed-off-by: Lin Ma <lma@suse.com>
> > > ---
> > > hw/scsi/scsi-disk.c | 90
> > ++++++++++++++++++++++++++++++++++++++
> > > include/block/accounting.h | 1 +
> > > include/scsi/constants.h | 1 +
> > > 3 files changed, 92 insertions(+)
> > >
> > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index
> > > 387503e11b..9e3002ddaf 100644
> > > --- a/hw/scsi/scsi-disk.c
> > > +++ b/hw/scsi/scsi-disk.c
> > > @@ -1866,6 +1866,89 @@ 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); }
> > > +
> > > +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);
> >
> > scsi_get_lba_status_complete_noio() looks asynchronous. If the BlockDriver
> > yields in .bdrv_co_block_status() then the operation has not completed yet
> > when scsi_get_lba_status_complete_noio() returns. It is not safe to access the
> > GetLbaStatusCBData data until the async operation is complete.
> >
> > Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> > use-after-free when *num_blocks and *is_deallocated are accessed.
>
> Got it, I'll fill the outbuf[] in the completion function in V3.
>
> > These issues can be solved by making this code asynchronous (similar to
> > read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in the completion
> > function before g_free(data) is called.
>
> I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' and the
> 'bdrv_co_flush()', They look similiar, They called corresponding bs->drv->bdrv_co_*()
> or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
> The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or bs->drv->bdrv_aio_pdiscard()
> The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or bs->drv->bdrv_aio_flush().
> The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains the
> coroutine version of block_status, no aio version of block_status.
>
> About "making this code asynchronous", Well, In fact I havn't realized yet where the issue is.
> If what you mean is that make the 'bdrv_co_get_lba_status()' asynchronous, How about
> directly calling coroutine-based 'bdrv_co_block_status()' instead of 'bdrv_block_status()' in it?
> Or could you please suggest more detailed information?
> BTW, IMO the existing BlockDriver->bdrv_co_block_status() is enough, It's not necessary to
> implement a drv->bdrv_aio_get_block_status() in BlockDrivers(say qcow2 or raw), Am I right?
scsi_disk_emulate_get_lba_status() is called outside coroutine context.
It is expected to return without blocking so that other activity can
continue (e.g. the vCPU can continue execution).
scsi_disk_emulate_get_lba_status() cannot fill in outbuf because we may
not have fetched the LBA status yet when it needs to return.
Luckily there are other SCSI commands in scsi_disk_emulate_command()
that are asynchronous. You can follow their model:
case SYNCHRONIZE_CACHE:
/* The request is used as the AIO opaque value, so add a ref. */
scsi_req_ref(&r->req);
block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0,
BLOCK_ACCT_FLUSH);
r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r);
return 0;
The request is not completed by scsi_disk_emulate_command(). Instead
blk_aio_flush() launches a flush operation and the SCSI request is
passed along as the argument to the scsi_aio_complete() completion
function.
Something similar is needed for GET_LBA_STATUS. Since there is no
bdrv_aio_block_status() you can create a coroutine instead of an aiocb:
static void coroutine_fn scsi_co_block_status(void *opaque)
{
int ret;
aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
ret = bdrv_co_block_status(...);
...fill in outbuf...
scsi_req_complete(&r->req, GOOD);
aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
scsi_req_unref(&r->req);
}
...in scsi_disk_emulate_command()...
scsi_req_ref(&r->req);
co = qemu_coroutine_create(scsi_co_block_status, r);
aio_co_schedule(blk_get_aio_context(s->qdev.conf.blk), co);
return 0;
This is just a sketch, I haven't checked the details. The trickiest
issue is probably how to deal with r->req.aiocb, which is normally set
for async requests. It will be necessary to study the code to figure out
a solution because there is no BlockAIOCB in this case (we're using a
coroutine instead).
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: 回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-07-08 12:29 ` Stefan Hajnoczi
@ 2020-07-08 12:38 ` Paolo Bonzini
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2020-07-08 12:38 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma
Cc: 'fam@euphon.net', 'kwolf@redhat.com',
'qemu-devel@nongnu.org', 'mreitz@redhat.com'
[-- Attachment #1.1: Type: text/plain, Size: 1304 bytes --]
On 08/07/20 14:29, Stefan Hajnoczi wrote:
> Something similar is needed for GET_LBA_STATUS. Since there is no
> bdrv_aio_block_status() you can create a coroutine instead of an aiocb:
>
> static void coroutine_fn scsi_co_block_status(void *opaque)
> {
> int ret;
>
> aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>
> ret = bdrv_co_block_status(...);
>
> ...fill in outbuf...
>
> scsi_req_complete(&r->req, GOOD);
>
> aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>
> scsi_req_unref(&r->req);
> }
>
> ...in scsi_disk_emulate_command()...
> scsi_req_ref(&r->req);
> co = qemu_coroutine_create(scsi_co_block_status, r);
> aio_co_schedule(blk_get_aio_context(s->qdev.conf.blk), co);
> return 0;
>
> This is just a sketch, I haven't checked the details. The trickiest
> issue is probably how to deal with r->req.aiocb, which is normally set
> for async requests. It will be necessary to study the code to figure out
> a solution because there is no BlockAIOCB in this case (we're using a
> coroutine instead).
It's probably simplest to put the code above in block/block-backend.c,
in the form of blk_aio_block_status which would follow what is done in
blk_aio_prwv.
Paolo
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 16:02 ` Eric Blake
@ 2020-06-30 15:40 ` Paolo Bonzini
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2020-06-30 15:40 UTC (permalink / raw)
To: Eric Blake, Stefan Hajnoczi, Lin Ma
Cc: fam, kwolf, qemu-devel, Stefan Hajnoczi, mreitz
On 29/06/20 18:02, Eric Blake wrote:
> - allocation implies that data comes from this layer of a backing chain, rather than deferring to a backing image
>
> - allocation implies that storage is reserved (that is, not sparse)
>
> It sounds like we are trying to represent the second question for scsi
> (namely, the same question that gets answered by lseek(SEEK_HOLE) for
> POSIX files), and not the first (namely, the question answered for qcow2
> images).
>
Yes, SCSI does not know about layers.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
@ 2020-06-29 16:02 ` Eric Blake
2020-06-30 15:40 ` Paolo Bonzini
1 sibling, 1 reply; 16+ messages in thread
From: Eric Blake @ 2020-06-29 16:02 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma
Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi, pbonzini
On 6/29/20 5:39 AM, Stefan Hajnoczi wrote:
>>> SCSI defines 3 values and QEMU can represent all of them:
>>>
>>> 0 - mapped or unknown
>>> 1 - deallocated
>>> 2 - anchored
>>>
>>> See the BDRV_BLOCK_* constants in include/block/block.h. The
>>> is_deallocated boolean is not enough to represent this state, but the
>>> bdrv_block_status() return value can be used instead.
>>
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
>
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
>
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
I haven't read the scsi code, but as written, I would assume that any
portion of the block status with this result has the following properties:
- reading sees guest-visible data; we cannot guarantee whether it is all
zero, and the data reserves actual space
> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
- reading sees all zeros, but the disk has reserved actual space such
that future writes will not run out of space
> deallocated: !BDRV_BLOCK_DATA
- reading does not see data; future writes will have to reserve space.
I would expect this option might also have '| BDRV_BLOCK_ZERO' depending
on whether this particular scsi device guarantees that reads of an
unallocated portion see all zeros instead of unspecified garbage.
>
> I have CCed Eric Blake, who is familiar with block status.
Adding Vladimir in CC as well, as he has pending patches that try to
clear up the use of BDRV_BLOCK_* constants with regards to two different
queries:
- allocation implies that data comes from this layer of a backing chain,
rather than deferring to a backing image
- allocation implies that storage is reserved (that is, not sparse)
It sounds like we are trying to represent the second question for scsi
(namely, the same question that gets answered by lseek(SEEK_HOLE) for
POSIX files), and not the first (namely, the question answered for qcow2
images).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-29 10:39 ` Stefan Hajnoczi
@ 2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
1 sibling, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2020-06-29 13:01 UTC (permalink / raw)
To: Stefan Hajnoczi, Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi
On 29/06/20 12:39, Stefan Hajnoczi wrote:
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
>
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> deallocated: !BDRV_BLOCK_DATA
I agree except that I wouldn't test BDRV_BLOCK_OFFSET_VALID. For
example a compressed cluster would still be reported as allocated even
if BDRV_BLOCK_OFFSET_VALID is cleared.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-25 13:31 [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
@ 2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
0 siblings, 2 replies; 16+ messages in thread
From: Stefan Hajnoczi @ 2020-06-29 10:39 UTC (permalink / raw)
To: Lin Ma; +Cc: fam, kwolf, qemu-devel, mreitz, Stefan Hajnoczi, pbonzini
[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]
On Thu, Jun 25, 2020 at 01:31:56PM +0000, Lin Ma wrote:
> On 2020-06-25 21:25, Lin Ma wrote:
> >> + /*
> >> + * 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;
> >
> > SCSI defines 3 values and QEMU can represent all of them:
> >
> > 0 - mapped or unknown
> > 1 - deallocated
> > 2 - anchored
> >
> > See the BDRV_BLOCK_* constants in include/block/block.h. The
> > is_deallocated boolean is not enough to represent this state, but the
> > bdrv_block_status() return value can be used instead.
>
> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>
> I'd like to use these combinations to analyze the bdrv_block_status() return value:
> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
> 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
> Am I right?
My understanding is that the SCSI status are mapped to QEMU block status
as follows:
allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
deallocated: !BDRV_BLOCK_DATA
I have CCed Eric Blake, who is familiar with block status.
> >> +}
> >> +
> >> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >> {
> >> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> >> @@ -2076,6 +2159,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;
> >
> > Is there something tricky going on here with iov_len that prevents us
> > from using break here and sharing the functions normal return code path?
>
> Using 'break' here and following the normal return code path cause the assertion
> 'assert(!r->req.aiocb)'.
>
> In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
> scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
> I borrowed this idea to avoid the assertion.
The assertion shows that this patch isn't asynchronous. I think the
assertion will pass once you convert GET_LBA_STATUS to async and then
a break statement will work.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
@ 2020-06-25 13:31 Lin Ma
2020-06-29 10:39 ` Stefan Hajnoczi
0 siblings, 1 reply; 16+ messages in thread
From: Lin Ma @ 2020-06-25 13:31 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: fam, kwolf, pbonzini, qemu-devel, mreitz
On 2020-06-25 21:25, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 8:14 PM
> ...
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 387503e11b..9e3002ddaf 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1866,6 +1866,89 @@ 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);
>> +}
>> +
>> +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);
>
> scsi_get_lba_status_complete_noio() looks asynchronous. If the
> BlockDriver yields in .bdrv_co_block_status() then the operation has not
> completed yet when scsi_get_lba_status_complete_noio() returns. It is
> not safe to access the GetLbaStatusCBData data until the async operation
> is complete.
>
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
>
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
> the completion function before g_free(data) is called.
Emm, I need to dig into the code for better understanding your suggestion.
I used to think that I already completely make 'scsi_get_lba_status_complete_noio'
asynchronous.
>> +
>> + /*
>> + * 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;
>
> SCSI defines 3 values and QEMU can represent all of them:
>
> 0 - mapped or unknown
> 1 - deallocated
> 2 - anchored
>
> See the BDRV_BLOCK_* constants in include/block/block.h. The
> is_deallocated boolean is not enough to represent this state, but the
> bdrv_block_status() return value can be used instead.
I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
I'd like to use these combinations to analyze the bdrv_block_status() return value:
'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
Am I right?
>> +}
>> +
>> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>> {
>> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> @@ -2076,6 +2159,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;
>
> Is there something tricky going on here with iov_len that prevents us
> from using break here and sharing the functions normal return code path?
Using 'break' here and following the normal return code path cause the assertion
'assert(!r->req.aiocb)'.
In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
I borrowed this idea to avoid the assertion.
>> }
>> trace_scsi_disk_emulate_command_SAI_unsupported();
>> goto illegal_request;
>> 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,
>> };
>>
>> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
>> index 874176019e..b18377b214 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.26.0
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
@ 2020-06-17 10:20 ` Lin Ma
0 siblings, 0 replies; 16+ messages in thread
From: Lin Ma @ 2020-06-17 10:20 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz, stefanha, Lin Ma, pbonzini
Signed-off-by: Lin Ma <lma@suse.com>
---
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++++++++++++++
include/block/accounting.h | 1 +
include/scsi/constants.h | 1 +
3 files changed, 92 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ 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);
+}
+
+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;
+}
+
static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,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/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,
};
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 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.26.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-07-08 22:18 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 10:30 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:30 ` [PATCH v2 1/3] block: Add bdrv_co_get_lba_status Lin Ma
2020-06-22 10:25 ` Stefan Hajnoczi
2020-06-17 10:30 ` [PATCH v2 2/3] block: Add GET LBA STATUS support Lin Ma
2020-06-17 10:30 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-22 12:14 ` Stefan Hajnoczi
2020-06-29 9:18 ` 回复: " Lin Ma
2020-07-08 12:29 ` Stefan Hajnoczi
2020-07-08 12:38 ` Paolo Bonzini
2020-06-17 11:14 ` [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation no-reply
-- strict thread matches above, loose matches on Subject: below --
2020-06-25 13:31 [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
2020-06-29 10:39 ` Stefan Hajnoczi
2020-06-29 13:01 ` Paolo Bonzini
2020-06-29 16:02 ` Eric Blake
2020-06-30 15:40 ` Paolo Bonzini
2020-06-17 10:20 [PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation Lin Ma
2020-06-17 10:20 ` [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command Lin Ma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.