> -----邮件原件-----
> 发件人: Stefan Hajnoczi <stefa...@redhat.com>
> 发送时间: 2020年6月22日 20:14
> 收件人: Lin Ma <l...@suse.com>
> 抄送: qemu-devel@nongnu.org; f...@euphon.net; kw...@redhat.com;
> mre...@redhat.com; pbonz...@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 <l...@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
> >
