回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

> -----邮件原件-----
> 发件人: 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
> >