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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature