On Mon, Jun 29, 2020 at 09:18:59AM +0000, Lin Ma wrote:
>
>
> > -----邮件原件-----
> > 发件人: 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?
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
