| Message ID | 1380728469-29435-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
Il 02/10/2013 17:41, Peter Lieven ha scritto: > this converts read, write and flush functions from aio to coroutines. I'm not sure it's already the time for this... Cancellation sucks in QEMU, and this is going to make things even worse. Paolo > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/iscsi.c | 405 +++++++++++++++------------------------------------------ > 1 file changed, 107 insertions(+), 298 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 5c7baee..f37e3c5 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -237,44 +237,6 @@ iscsi_process_write(void *arg) > iscsi_set_events(iscsilun); > } > > -static int > -iscsi_aio_writev_acb(IscsiAIOCB *acb); > - > -static void > -iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, > - void *command_data, void *opaque) > -{ > - IscsiAIOCB *acb = opaque; > - > - trace_iscsi_aio_write16_cb(iscsi, status, acb, acb->canceled); > - > - g_free(acb->buf); > - acb->buf = NULL; > - > - if (acb->canceled != 0) { > - return; > - } > - > - acb->status = 0; > - if (status != 0) { > - if (status == SCSI_STATUS_CHECK_CONDITION > - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION > - && acb->retries-- > 0) { > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > - if (iscsi_aio_writev_acb(acb) == 0) { > - iscsi_set_events(acb->iscsilun); > - return; > - } > - } > - error_report("Failed to write16 data to iSCSI lun. %s", > - iscsi_get_error(iscsi)); > - acb->status = -EIO; > - } > - > - iscsi_schedule_bh(acb); > -} > - > static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) > { > return sector * iscsilun->block_size / BDRV_SECTOR_SIZE; > @@ -299,324 +261,172 @@ static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors, > return 1; > } > > -static int > -iscsi_aio_writev_acb(IscsiAIOCB *acb) > +static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, > + int64_t sector_num, int nb_sectors, > + QEMUIOVector *iov) > { > - struct iscsi_context *iscsi = acb->iscsilun->iscsi; > - size_t size; > - uint32_t num_sectors; > + IscsiLun *iscsilun = bs->opaque; > + struct IscsiTask iTask; > uint64_t lba; > -#if !defined(LIBISCSI_FEATURE_IOVECTOR) > - struct iscsi_data data; > -#endif > - int ret; > - > - acb->canceled = 0; > - acb->bh = NULL; > - acb->status = -EINPROGRESS; > - acb->buf = NULL; > + uint32_t num_sectors; > + uint8_t *data = NULL; > + uint8_t *buf = NULL; > > - /* this will allow us to get rid of 'buf' completely */ > - size = acb->nb_sectors * BDRV_SECTOR_SIZE; > + if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > + return -EINVAL; > + } > > + lba = sector_qemu2lun(sector_num, iscsilun); > + num_sectors = sector_qemu2lun(nb_sectors, iscsilun); > #if !defined(LIBISCSI_FEATURE_IOVECTOR) > - data.size = MIN(size, acb->qiov->size); > - > /* if the iovec only contains one buffer we can pass it directly */ > - if (acb->qiov->niov == 1) { > - data.data = acb->qiov->iov[0].iov_base; > + if (iov->niov == 1) { > + data = iov->iov[0].iov_base; > } else { > - acb->buf = g_malloc(data.size); > - qemu_iovec_to_buf(acb->qiov, 0, acb->buf, data.size); > - data.data = acb->buf; > + size_t size = MIN(nb_sectors * BDRV_SECTOR_SIZE, iov->size); > + buf = g_malloc(size); > + qemu_iovec_to_buf(iov, 0, buf, size); > + data = buf; > } > #endif > - > - acb->task = malloc(sizeof(struct scsi_task)); > - if (acb->task == NULL) { > - error_report("iSCSI: Failed to allocate task for scsi WRITE16 " > - "command. %s", iscsi_get_error(iscsi)); > - return -1; > + iscsi_co_init_iscsitask(iscsilun, &iTask); > +retry: > + iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba, > + data, num_sectors * iscsilun->block_size, > + iscsilun->block_size, 0, 0, 0, 0, 0, > + iscsi_co_generic_cb, &iTask); > + if (iTask.task == NULL) { > + g_free(buf); > + return -EIO; > } > - memset(acb->task, 0, sizeof(struct scsi_task)); > - > - acb->task->xfer_dir = SCSI_XFER_WRITE; > - acb->task->cdb_size = 16; > - acb->task->cdb[0] = 0x8a; > - lba = sector_qemu2lun(acb->sector_num, acb->iscsilun); > - *(uint32_t *)&acb->task->cdb[2] = htonl(lba >> 32); > - *(uint32_t *)&acb->task->cdb[6] = htonl(lba & 0xffffffff); > - num_sectors = sector_qemu2lun(acb->nb_sectors, acb->iscsilun); > - *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors); > - acb->task->expxferlen = size; > - > #if defined(LIBISCSI_FEATURE_IOVECTOR) > - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, > - iscsi_aio_write16_cb, > - NULL, > - acb); > -#else > - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, > - iscsi_aio_write16_cb, > - &data, > - acb); > + scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov, > + iov->niov); > #endif > - if (ret != 0) { > - scsi_free_scsi_task(acb->task); > - g_free(acb->buf); > - return -1; > + while (!iTask.complete) { > + iscsi_set_events(iscsilun); > + qemu_coroutine_yield(); > } > > -#if defined(LIBISCSI_FEATURE_IOVECTOR) > - scsi_task_set_iov_out(acb->task, (struct scsi_iovec*) acb->qiov->iov, acb->qiov->niov); > -#endif > - > - return 0; > -} > - > -static BlockDriverAIOCB * > -iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num, > - QEMUIOVector *qiov, int nb_sectors, > - BlockDriverCompletionFunc *cb, > - void *opaque) > -{ > - IscsiLun *iscsilun = bs->opaque; > - IscsiAIOCB *acb; > - > - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > - return NULL; > + if (iTask.task != NULL) { > + scsi_free_scsi_task(iTask.task); > + iTask.task = NULL; > } > > - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); > - trace_iscsi_aio_writev(iscsilun->iscsi, sector_num, nb_sectors, opaque, acb); > - > - acb->iscsilun = iscsilun; > - acb->qiov = qiov; > - acb->nb_sectors = nb_sectors; > - acb->sector_num = sector_num; > - acb->retries = ISCSI_CMD_RETRIES; > - > - if (iscsi_aio_writev_acb(acb) != 0) { > - qemu_aio_release(acb); > - return NULL; > + if (iTask.do_retry) { > + goto retry; > } > > - iscsi_set_events(iscsilun); > - return &acb->common; > -} > - > -static int > -iscsi_aio_readv_acb(IscsiAIOCB *acb); > - > -static void > -iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, > - void *command_data, void *opaque) > -{ > - IscsiAIOCB *acb = opaque; > - > - trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); > - > - if (acb->canceled != 0) { > - return; > - } > + g_free(buf); > > - acb->status = 0; > - if (status != 0) { > - if (status == SCSI_STATUS_CHECK_CONDITION > - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION > - && acb->retries-- > 0) { > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > - if (iscsi_aio_readv_acb(acb) == 0) { > - iscsi_set_events(acb->iscsilun); > - return; > - } > - } > - error_report("Failed to read16 data from iSCSI lun. %s", > - iscsi_get_error(iscsi)); > - acb->status = -EIO; > + if (iTask.status != SCSI_STATUS_GOOD) { > + return -EIO; > } > > - iscsi_schedule_bh(acb); > + return 0; > } > > -static int > -iscsi_aio_readv_acb(IscsiAIOCB *acb) > +static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, > + int64_t sector_num, int nb_sectors, > + QEMUIOVector *iov) > { > - struct iscsi_context *iscsi = acb->iscsilun->iscsi; > - size_t size; > + IscsiLun *iscsilun = bs->opaque; > + struct IscsiTask iTask; > uint64_t lba; > uint32_t num_sectors; > - int ret; > #if !defined(LIBISCSI_FEATURE_IOVECTOR) > int i; > #endif > > - acb->canceled = 0; > - acb->bh = NULL; > - acb->status = -EINPROGRESS; > - acb->buf = NULL; > - > - size = acb->nb_sectors * BDRV_SECTOR_SIZE; > - > - acb->task = malloc(sizeof(struct scsi_task)); > - if (acb->task == NULL) { > - error_report("iSCSI: Failed to allocate task for scsi READ16 " > - "command. %s", iscsi_get_error(iscsi)); > - return -1; > + if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > + return -EINVAL; > } > - memset(acb->task, 0, sizeof(struct scsi_task)); > > - acb->task->xfer_dir = SCSI_XFER_READ; > - acb->task->expxferlen = size; > - lba = sector_qemu2lun(acb->sector_num, acb->iscsilun); > - num_sectors = sector_qemu2lun(acb->nb_sectors, acb->iscsilun); > + lba = sector_qemu2lun(sector_num, iscsilun); > + num_sectors = sector_qemu2lun(nb_sectors, iscsilun); > > - switch (acb->iscsilun->type) { > + iscsi_co_init_iscsitask(iscsilun, &iTask); > +retry: > + switch (iscsilun->type) { > case TYPE_DISK: > - acb->task->cdb_size = 16; > - acb->task->cdb[0] = 0x88; > - *(uint32_t *)&acb->task->cdb[2] = htonl(lba >> 32); > - *(uint32_t *)&acb->task->cdb[6] = htonl(lba & 0xffffffff); > - *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors); > + iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba, > + num_sectors * iscsilun->block_size, > + iscsilun->block_size, 0, 0, 0, 0, 0, > + iscsi_co_generic_cb, &iTask); > break; > default: > - acb->task->cdb_size = 10; > - acb->task->cdb[0] = 0x28; > - *(uint32_t *)&acb->task->cdb[2] = htonl(lba); > - *(uint16_t *)&acb->task->cdb[7] = htons(num_sectors); > + iTask.task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun, lba, > + num_sectors * iscsilun->block_size, > + iscsilun->block_size, 0, 0, 0, 0, 0, > + iscsi_co_generic_cb, &iTask); > break; > } > - > - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, > - iscsi_aio_read16_cb, > - NULL, > - acb); > - if (ret != 0) { > - scsi_free_scsi_task(acb->task); > - return -1; > + if (iTask.task == NULL) { > + return -EIO; > } > - > #if defined(LIBISCSI_FEATURE_IOVECTOR) > - scsi_task_set_iov_in(acb->task, (struct scsi_iovec*) acb->qiov->iov, acb->qiov->niov); > + scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov); > #else > - for (i = 0; i < acb->qiov->niov; i++) { > - scsi_task_add_data_in_buffer(acb->task, > - acb->qiov->iov[i].iov_len, > - acb->qiov->iov[i].iov_base); > + for (i = 0; i < iov->niov; i++) { > + scsi_task_add_data_in_buffer(iTask.task, > + iov->iov[i].iov_len, > + iov->iov[i].iov_base); > } > #endif > - return 0; > -} > - > -static BlockDriverAIOCB * > -iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num, > - QEMUIOVector *qiov, int nb_sectors, > - BlockDriverCompletionFunc *cb, > - void *opaque) > -{ > - IscsiLun *iscsilun = bs->opaque; > - IscsiAIOCB *acb; > - > - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > - return NULL; > - } > > - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); > - trace_iscsi_aio_readv(iscsilun->iscsi, sector_num, nb_sectors, opaque, acb); > - > - acb->nb_sectors = nb_sectors; > - acb->sector_num = sector_num; > - acb->iscsilun = iscsilun; > - acb->qiov = qiov; > - acb->retries = ISCSI_CMD_RETRIES; > - > - if (iscsi_aio_readv_acb(acb) != 0) { > - qemu_aio_release(acb); > - return NULL; > + while (!iTask.complete) { > + iscsi_set_events(iscsilun); > + qemu_coroutine_yield(); > } > > - iscsi_set_events(iscsilun); > - return &acb->common; > -} > - > -static int > -iscsi_aio_flush_acb(IscsiAIOCB *acb); > - > -static void > -iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, > - void *command_data, void *opaque) > -{ > - IscsiAIOCB *acb = opaque; > - > - if (acb->canceled != 0) { > - return; > + if (iTask.task != NULL) { > + scsi_free_scsi_task(iTask.task); > + iTask.task = NULL; > } > > - acb->status = 0; > - if (status != 0) { > - if (status == SCSI_STATUS_CHECK_CONDITION > - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION > - && acb->retries-- > 0) { > - scsi_free_scsi_task(acb->task); > - acb->task = NULL; > - if (iscsi_aio_flush_acb(acb) == 0) { > - iscsi_set_events(acb->iscsilun); > - return; > - } > - } > - error_report("Failed to sync10 data on iSCSI lun. %s", > - iscsi_get_error(iscsi)); > - acb->status = -EIO; > + if (iTask.do_retry) { > + goto retry; > } > > - iscsi_schedule_bh(acb); > -} > - > -static int > -iscsi_aio_flush_acb(IscsiAIOCB *acb) > -{ > - struct iscsi_context *iscsi = acb->iscsilun->iscsi; > - > - acb->canceled = 0; > - acb->bh = NULL; > - acb->status = -EINPROGRESS; > - acb->buf = NULL; > - > - acb->task = iscsi_synchronizecache10_task(iscsi, acb->iscsilun->lun, > - 0, 0, 0, 0, > - iscsi_synccache10_cb, > - acb); > - if (acb->task == NULL) { > - error_report("iSCSI: Failed to send synchronizecache10 command. %s", > - iscsi_get_error(iscsi)); > - return -1; > + if (iTask.status != SCSI_STATUS_GOOD) { > + return -EIO; > } > > return 0; > } > > -static BlockDriverAIOCB * > -iscsi_aio_flush(BlockDriverState *bs, > - BlockDriverCompletionFunc *cb, void *opaque) > +static int coroutine_fn iscsi_co_flush(BlockDriverState *bs) > { > IscsiLun *iscsilun = bs->opaque; > + struct IscsiTask iTask; > > - IscsiAIOCB *acb; > + iscsi_co_init_iscsitask(iscsilun, &iTask); > > - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); > +retry: > + if (iscsi_synchronizecache10_task(iscsilun->iscsi, iscsilun->lun, 0, 0, 0, > + 0, iscsi_co_generic_cb, &iTask) == NULL) { > + return -EIO; > + } > > - acb->iscsilun = iscsilun; > - acb->retries = ISCSI_CMD_RETRIES; > + while (!iTask.complete) { > + iscsi_set_events(iscsilun); > + qemu_coroutine_yield(); > + } > > - if (iscsi_aio_flush_acb(acb) != 0) { > - qemu_aio_release(acb); > - return NULL; > + if (iTask.task != NULL) { > + scsi_free_scsi_task(iTask.task); > + iTask.task = NULL; > } > > - iscsi_set_events(iscsilun); > + if (iTask.do_retry) { > + goto retry; > + } > > - return &acb->common; > + if (iTask.status != SCSI_STATUS_GOOD) { > + return -EIO; > + } > + > + return 0; > } > > #ifdef __linux__ > @@ -1599,12 +1409,11 @@ static BlockDriver bdrv_iscsi = { > #if defined(LIBISCSI_FEATURE_IOVECTOR) > .bdrv_co_get_block_status = iscsi_co_get_block_status, > #endif > - .bdrv_co_discard = iscsi_co_discard, > - .bdrv_co_write_zeroes = iscsi_co_write_zeroes, > - > - .bdrv_aio_readv = iscsi_aio_readv, > - .bdrv_aio_writev = iscsi_aio_writev, > - .bdrv_aio_flush = iscsi_aio_flush, > + .bdrv_co_discard = iscsi_co_discard, > + .bdrv_co_write_zeroes = iscsi_co_write_zeroes, > + .bdrv_co_readv = iscsi_co_readv, > + .bdrv_co_writev = iscsi_co_writev, > + .bdrv_co_flush_to_disk = iscsi_co_flush, > > .bdrv_has_zero_init = iscsi_has_zero_init, > .bdrv_has_discard_zeroes = iscsi_has_discard_zeroes, >
Am 02.10.2013 17:46, schrieb Paolo Bonzini: > Il 02/10/2013 17:41, Peter Lieven ha scritto: >> this converts read, write and flush functions from aio to coroutines. > I'm not sure it's already the time for this... Cancellation sucks in > QEMU, and this is going to make things even worse. Ok, I was not aware. I talked with Kevin about this some months ago and promised (list CCed) to prepare this for 1.7.0. Nobody objected ;-) But, anyway the patch is there as a basis as soon as the time has come. Peter
Il 02/10/2013 17:54, Peter Lieven ha scritto: >> > I'm not sure it's already the time for this... Cancellation sucks in >> > QEMU, and this is going to make things even worse. > Ok, I was not aware. I talked with Kevin about this some months ago > and promised (list CCed) to prepare this for 1.7.0. Nobody objected ;-) > > But, anyway the patch is there as a basis as soon as the time has come. Yes, I'll stash it in my all-things-SCSI branch. :) Paolo
Am 02.10.2013 17:55, schrieb Paolo Bonzini: > Il 02/10/2013 17:54, Peter Lieven ha scritto: >>>> I'm not sure it's already the time for this... Cancellation sucks in >>>> QEMU, and this is going to make things even worse. >> Ok, I was not aware. I talked with Kevin about this some months ago >> and promised (list CCed) to prepare this for 1.7.0. Nobody objected ;-) >> >> But, anyway the patch is there as a basis as soon as the time has come. > Yes, I'll stash it in my all-things-SCSI branch. :) It was nice to see that the code collapsed to 1/3, btw. Peter
Am 02.10.2013 um 17:46 hat Paolo Bonzini geschrieben: > Il 02/10/2013 17:41, Peter Lieven ha scritto: > > this converts read, write and flush functions from aio to coroutines. > > I'm not sure it's already the time for this... Cancellation sucks in > QEMU, and this is going to make things even worse. Not sure what you're referring to. If you mean iscsi_aio_cancel(), isn't it dead code anyway since we changed block.c to use coroutines for everything? bdrv_co_io_em() even throws the acb away, so even if you wanted, there's no way to cancel the request even today. And the lines deleted/inserted are much in favour of this patch. Kevin
Il 08/10/2013 14:33, Kevin Wolf ha scritto: >>> > > this converts read, write and flush functions from aio to coroutines. >> > >> > I'm not sure it's already the time for this... Cancellation sucks in >> > QEMU, and this is going to make things even worse. > Not sure what you're referring to. If you mean iscsi_aio_cancel(), isn't > it dead code anyway since we changed block.c to use coroutines for > everything? bdrv_co_io_em() even throws the acb away, so even if you > wanted, there's no way to cancel the request even today. SCSI tries to use cancellation, and this results in VCPU threads starving all other threads. So I would like to introduce cancellation points for coroutines. Paolo
Am 08.10.2013 um 14:35 hat Paolo Bonzini geschrieben: > Il 08/10/2013 14:33, Kevin Wolf ha scritto: > >>> > > this converts read, write and flush functions from aio to coroutines. > >> > > >> > I'm not sure it's already the time for this... Cancellation sucks in > >> > QEMU, and this is going to make things even worse. > > Not sure what you're referring to. If you mean iscsi_aio_cancel(), isn't > > it dead code anyway since we changed block.c to use coroutines for > > everything? bdrv_co_io_em() even throws the acb away, so even if you > > wanted, there's no way to cancel the request even today. > > SCSI tries to use cancellation, and this results in VCPU threads > starving all other threads. So I would like to introduce cancellation > points for coroutines. Sounds like a nice thing to have, but it's unrelated to this patch. Cancellation means waiting for request completion before and after the patch. Kevin
On 08.10.2013 14:39, Kevin Wolf wrote: > Am 08.10.2013 um 14:35 hat Paolo Bonzini geschrieben: >> Il 08/10/2013 14:33, Kevin Wolf ha scritto: >>>>>>> this converts read, write and flush functions from aio to coroutines. >>>>> I'm not sure it's already the time for this... Cancellation sucks in >>>>> QEMU, and this is going to make things even worse. >>> Not sure what you're referring to. If you mean iscsi_aio_cancel(), isn't >>> it dead code anyway since we changed block.c to use coroutines for >>> everything? bdrv_co_io_em() even throws the acb away, so even if you >>> wanted, there's no way to cancel the request even today. >> SCSI tries to use cancellation, and this results in VCPU threads >> starving all other threads. So I would like to introduce cancellation >> points for coroutines. > Sounds like a nice thing to have, but it's unrelated to this patch. > Cancellation means waiting for request completion before and after the > patch. Is there anything I can contribute or is this all outside the iscsi driver?
On 08.10.2013 14:39, Kevin Wolf wrote: > Am 08.10.2013 um 14:35 hat Paolo Bonzini geschrieben: >> Il 08/10/2013 14:33, Kevin Wolf ha scritto: >>>>>>> this converts read, write and flush functions from aio to coroutines. >>>>> I'm not sure it's already the time for this... Cancellation sucks in >>>>> QEMU, and this is going to make things even worse. >>> Not sure what you're referring to. If you mean iscsi_aio_cancel(), isn't >>> it dead code anyway since we changed block.c to use coroutines for >>> everything? bdrv_co_io_em() even throws the acb away, so even if you >>> wanted, there's no way to cancel the request even today. >> SCSI tries to use cancellation, and this results in VCPU threads >> starving all other threads. So I would like to introduce cancellation >> points for coroutines. > Sounds like a nice thing to have, but it's unrelated to this patch. > Cancellation means waiting for request completion before and after the > patch. > > Kevin Can we proceed with the above patch for 1.8? Peter
Il 25/11/2013 11:47, Peter Lieven ha scritto: > On 08.10.2013 14:39, Kevin Wolf wrote: >> Am 08.10.2013 um 14:35 hat Paolo Bonzini geschrieben: >>> Il 08/10/2013 14:33, Kevin Wolf ha scritto: >>>>>>>> this converts read, write and flush functions from aio to >>>>>>>> coroutines. >>>>>> I'm not sure it's already the time for this... Cancellation sucks in >>>>>> QEMU, and this is going to make things even worse. >>>> Not sure what you're referring to. If you mean iscsi_aio_cancel(), >>>> isn't >>>> it dead code anyway since we changed block.c to use coroutines for >>>> everything? bdrv_co_io_em() even throws the acb away, so even if you >>>> wanted, there's no way to cancel the request even today. >>> SCSI tries to use cancellation, and this results in VCPU threads >>> starving all other threads. So I would like to introduce cancellation >>> points for coroutines. >> Sounds like a nice thing to have, but it's unrelated to this patch. >> Cancellation means waiting for request completion before and after the >> patch. > > Can we proceed with the above patch for 1.8? Yes, thanks. Paolo
On 25.11.2013 11:54, Paolo Bonzini wrote: > Il 25/11/2013 11:47, Peter Lieven ha scritto: >> On 08.10.2013 14:39, Kevin Wolf wrote: >>> Am 08.10.2013 um 14:35 hat Paolo Bonzini geschrieben: >>>> Il 08/10/2013 14:33, Kevin Wolf ha scritto: >>>>>>>>> this converts read, write and flush functions from aio to >>>>>>>>> coroutines. >>>>>>> I'm not sure it's already the time for this... Cancellation sucks in >>>>>>> QEMU, and this is going to make things even worse. >>>>> Not sure what you're referring to. If you mean iscsi_aio_cancel(), >>>>> isn't >>>>> it dead code anyway since we changed block.c to use coroutines for >>>>> everything? bdrv_co_io_em() even throws the acb away, so even if you >>>>> wanted, there's no way to cancel the request even today. >>>> SCSI tries to use cancellation, and this results in VCPU threads >>>> starving all other threads. So I would like to introduce cancellation >>>> points for coroutines. >>> Sounds like a nice thing to have, but it's unrelated to this patch. >>> Cancellation means waiting for request completion before and after the >>> patch. >> Can we proceed with the above patch for 1.8? > Yes, thanks. Will you pull it or shall I rebase and sent it again after 1.7.0 release? Peter
Il 25/11/2013 11:56, Peter Lieven ha scritto: >>>> >>> Can we proceed with the above patch for 1.8? >> Yes, thanks. > Will you pull it or shall I rebase and sent it again after 1.7.0 release? Please do the latter. Paolo
diff --git a/block/iscsi.c b/block/iscsi.c index 5c7baee..f37e3c5 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -237,44 +237,6 @@ iscsi_process_write(void *arg) iscsi_set_events(iscsilun); } -static int -iscsi_aio_writev_acb(IscsiAIOCB *acb); - -static void -iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, - void *command_data, void *opaque) -{ - IscsiAIOCB *acb = opaque; - - trace_iscsi_aio_write16_cb(iscsi, status, acb, acb->canceled); - - g_free(acb->buf); - acb->buf = NULL; - - if (acb->canceled != 0) { - return; - } - - acb->status = 0; - if (status != 0) { - if (status == SCSI_STATUS_CHECK_CONDITION - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION - && acb->retries-- > 0) { - scsi_free_scsi_task(acb->task); - acb->task = NULL; - if (iscsi_aio_writev_acb(acb) == 0) { - iscsi_set_events(acb->iscsilun); - return; - } - } - error_report("Failed to write16 data to iSCSI lun. %s", - iscsi_get_error(iscsi)); - acb->status = -EIO; - } - - iscsi_schedule_bh(acb); -} - static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) { return sector * iscsilun->block_size / BDRV_SECTOR_SIZE; @@ -299,324 +261,172 @@ static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors, return 1; } -static int -iscsi_aio_writev_acb(IscsiAIOCB *acb) +static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, + int64_t sector_num, int nb_sectors, + QEMUIOVector *iov) { - struct iscsi_context *iscsi = acb->iscsilun->iscsi; - size_t size; - uint32_t num_sectors; + IscsiLun *iscsilun = bs->opaque; + struct IscsiTask iTask; uint64_t lba; -#if !defined(LIBISCSI_FEATURE_IOVECTOR) - struct iscsi_data data; -#endif - int ret; - - acb->canceled = 0; - acb->bh = NULL; - acb->status = -EINPROGRESS; - acb->buf = NULL; + uint32_t num_sectors; + uint8_t *data = NULL; + uint8_t *buf = NULL; - /* this will allow us to get rid of 'buf' completely */ - size = acb->nb_sectors * BDRV_SECTOR_SIZE; + if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + return -EINVAL; + } + lba = sector_qemu2lun(sector_num, iscsilun); + num_sectors = sector_qemu2lun(nb_sectors, iscsilun); #if !defined(LIBISCSI_FEATURE_IOVECTOR) - data.size = MIN(size, acb->qiov->size); - /* if the iovec only contains one buffer we can pass it directly */ - if (acb->qiov->niov == 1) { - data.data = acb->qiov->iov[0].iov_base; + if (iov->niov == 1) { + data = iov->iov[0].iov_base; } else { - acb->buf = g_malloc(data.size); - qemu_iovec_to_buf(acb->qiov, 0, acb->buf, data.size); - data.data = acb->buf; + size_t size = MIN(nb_sectors * BDRV_SECTOR_SIZE, iov->size); + buf = g_malloc(size); + qemu_iovec_to_buf(iov, 0, buf, size); + data = buf; } #endif - - acb->task = malloc(sizeof(struct scsi_task)); - if (acb->task == NULL) { - error_report("iSCSI: Failed to allocate task for scsi WRITE16 " - "command. %s", iscsi_get_error(iscsi)); - return -1; + iscsi_co_init_iscsitask(iscsilun, &iTask); +retry: + iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba, + data, num_sectors * iscsilun->block_size, + iscsilun->block_size, 0, 0, 0, 0, 0, + iscsi_co_generic_cb, &iTask); + if (iTask.task == NULL) { + g_free(buf); + return -EIO; } - memset(acb->task, 0, sizeof(struct scsi_task)); - - acb->task->xfer_dir = SCSI_XFER_WRITE; - acb->task->cdb_size = 16; - acb->task->cdb[0] = 0x8a; - lba = sector_qemu2lun(acb->sector_num, acb->iscsilun); - *(uint32_t *)&acb->task->cdb[2] = htonl(lba >> 32); - *(uint32_t *)&acb->task->cdb[6] = htonl(lba & 0xffffffff); - num_sectors = sector_qemu2lun(acb->nb_sectors, acb->iscsilun); - *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors); - acb->task->expxferlen = size; - #if defined(LIBISCSI_FEATURE_IOVECTOR) - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, - iscsi_aio_write16_cb, - NULL, - acb); -#else - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, - iscsi_aio_write16_cb, - &data, - acb); + scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov, + iov->niov); #endif - if (ret != 0) { - scsi_free_scsi_task(acb->task); - g_free(acb->buf); - return -1; + while (!iTask.complete) { + iscsi_set_events(iscsilun); + qemu_coroutine_yield(); } -#if defined(LIBISCSI_FEATURE_IOVECTOR) - scsi_task_set_iov_out(acb->task, (struct scsi_iovec*) acb->qiov->iov, acb->qiov->niov); -#endif - - return 0; -} - -static BlockDriverAIOCB * -iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num, - QEMUIOVector *qiov, int nb_sectors, - BlockDriverCompletionFunc *cb, - void *opaque) -{ - IscsiLun *iscsilun = bs->opaque; - IscsiAIOCB *acb; - - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { - return NULL; + if (iTask.task != NULL) { + scsi_free_scsi_task(iTask.task); + iTask.task = NULL; } - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); - trace_iscsi_aio_writev(iscsilun->iscsi, sector_num, nb_sectors, opaque, acb); - - acb->iscsilun = iscsilun; - acb->qiov = qiov; - acb->nb_sectors = nb_sectors; - acb->sector_num = sector_num; - acb->retries = ISCSI_CMD_RETRIES; - - if (iscsi_aio_writev_acb(acb) != 0) { - qemu_aio_release(acb); - return NULL; + if (iTask.do_retry) { + goto retry; } - iscsi_set_events(iscsilun); - return &acb->common; -} - -static int -iscsi_aio_readv_acb(IscsiAIOCB *acb); - -static void -iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, - void *command_data, void *opaque) -{ - IscsiAIOCB *acb = opaque; - - trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); - - if (acb->canceled != 0) { - return; - } + g_free(buf); - acb->status = 0; - if (status != 0) { - if (status == SCSI_STATUS_CHECK_CONDITION - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION - && acb->retries-- > 0) { - scsi_free_scsi_task(acb->task); - acb->task = NULL; - if (iscsi_aio_readv_acb(acb) == 0) { - iscsi_set_events(acb->iscsilun); - return; - } - } - error_report("Failed to read16 data from iSCSI lun. %s", - iscsi_get_error(iscsi)); - acb->status = -EIO; + if (iTask.status != SCSI_STATUS_GOOD) { + return -EIO; } - iscsi_schedule_bh(acb); + return 0; } -static int -iscsi_aio_readv_acb(IscsiAIOCB *acb) +static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, + int64_t sector_num, int nb_sectors, + QEMUIOVector *iov) { - struct iscsi_context *iscsi = acb->iscsilun->iscsi; - size_t size; + IscsiLun *iscsilun = bs->opaque; + struct IscsiTask iTask; uint64_t lba; uint32_t num_sectors; - int ret; #if !defined(LIBISCSI_FEATURE_IOVECTOR) int i; #endif - acb->canceled = 0; - acb->bh = NULL; - acb->status = -EINPROGRESS; - acb->buf = NULL; - - size = acb->nb_sectors * BDRV_SECTOR_SIZE; - - acb->task = malloc(sizeof(struct scsi_task)); - if (acb->task == NULL) { - error_report("iSCSI: Failed to allocate task for scsi READ16 " - "command. %s", iscsi_get_error(iscsi)); - return -1; + if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + return -EINVAL; } - memset(acb->task, 0, sizeof(struct scsi_task)); - acb->task->xfer_dir = SCSI_XFER_READ; - acb->task->expxferlen = size; - lba = sector_qemu2lun(acb->sector_num, acb->iscsilun); - num_sectors = sector_qemu2lun(acb->nb_sectors, acb->iscsilun); + lba = sector_qemu2lun(sector_num, iscsilun); + num_sectors = sector_qemu2lun(nb_sectors, iscsilun); - switch (acb->iscsilun->type) { + iscsi_co_init_iscsitask(iscsilun, &iTask); +retry: + switch (iscsilun->type) { case TYPE_DISK: - acb->task->cdb_size = 16; - acb->task->cdb[0] = 0x88; - *(uint32_t *)&acb->task->cdb[2] = htonl(lba >> 32); - *(uint32_t *)&acb->task->cdb[6] = htonl(lba & 0xffffffff); - *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors); + iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba, + num_sectors * iscsilun->block_size, + iscsilun->block_size, 0, 0, 0, 0, 0, + iscsi_co_generic_cb, &iTask); break; default: - acb->task->cdb_size = 10; - acb->task->cdb[0] = 0x28; - *(uint32_t *)&acb->task->cdb[2] = htonl(lba); - *(uint16_t *)&acb->task->cdb[7] = htons(num_sectors); + iTask.task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun, lba, + num_sectors * iscsilun->block_size, + iscsilun->block_size, 0, 0, 0, 0, 0, + iscsi_co_generic_cb, &iTask); break; } - - ret = iscsi_scsi_command_async(iscsi, acb->iscsilun->lun, acb->task, - iscsi_aio_read16_cb, - NULL, - acb); - if (ret != 0) { - scsi_free_scsi_task(acb->task); - return -1; + if (iTask.task == NULL) { + return -EIO; } - #if defined(LIBISCSI_FEATURE_IOVECTOR) - scsi_task_set_iov_in(acb->task, (struct scsi_iovec*) acb->qiov->iov, acb->qiov->niov); + scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov); #else - for (i = 0; i < acb->qiov->niov; i++) { - scsi_task_add_data_in_buffer(acb->task, - acb->qiov->iov[i].iov_len, - acb->qiov->iov[i].iov_base); + for (i = 0; i < iov->niov; i++) { + scsi_task_add_data_in_buffer(iTask.task, + iov->iov[i].iov_len, + iov->iov[i].iov_base); } #endif - return 0; -} - -static BlockDriverAIOCB * -iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num, - QEMUIOVector *qiov, int nb_sectors, - BlockDriverCompletionFunc *cb, - void *opaque) -{ - IscsiLun *iscsilun = bs->opaque; - IscsiAIOCB *acb; - - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { - return NULL; - } - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); - trace_iscsi_aio_readv(iscsilun->iscsi, sector_num, nb_sectors, opaque, acb); - - acb->nb_sectors = nb_sectors; - acb->sector_num = sector_num; - acb->iscsilun = iscsilun; - acb->qiov = qiov; - acb->retries = ISCSI_CMD_RETRIES; - - if (iscsi_aio_readv_acb(acb) != 0) { - qemu_aio_release(acb); - return NULL; + while (!iTask.complete) { + iscsi_set_events(iscsilun); + qemu_coroutine_yield(); } - iscsi_set_events(iscsilun); - return &acb->common; -} - -static int -iscsi_aio_flush_acb(IscsiAIOCB *acb); - -static void -iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, - void *command_data, void *opaque) -{ - IscsiAIOCB *acb = opaque; - - if (acb->canceled != 0) { - return; + if (iTask.task != NULL) { + scsi_free_scsi_task(iTask.task); + iTask.task = NULL; } - acb->status = 0; - if (status != 0) { - if (status == SCSI_STATUS_CHECK_CONDITION - && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION - && acb->retries-- > 0) { - scsi_free_scsi_task(acb->task); - acb->task = NULL; - if (iscsi_aio_flush_acb(acb) == 0) { - iscsi_set_events(acb->iscsilun); - return; - } - } - error_report("Failed to sync10 data on iSCSI lun. %s", - iscsi_get_error(iscsi)); - acb->status = -EIO; + if (iTask.do_retry) { + goto retry; } - iscsi_schedule_bh(acb); -} - -static int -iscsi_aio_flush_acb(IscsiAIOCB *acb) -{ - struct iscsi_context *iscsi = acb->iscsilun->iscsi; - - acb->canceled = 0; - acb->bh = NULL; - acb->status = -EINPROGRESS; - acb->buf = NULL; - - acb->task = iscsi_synchronizecache10_task(iscsi, acb->iscsilun->lun, - 0, 0, 0, 0, - iscsi_synccache10_cb, - acb); - if (acb->task == NULL) { - error_report("iSCSI: Failed to send synchronizecache10 command. %s", - iscsi_get_error(iscsi)); - return -1; + if (iTask.status != SCSI_STATUS_GOOD) { + return -EIO; } return 0; } -static BlockDriverAIOCB * -iscsi_aio_flush(BlockDriverState *bs, - BlockDriverCompletionFunc *cb, void *opaque) +static int coroutine_fn iscsi_co_flush(BlockDriverState *bs) { IscsiLun *iscsilun = bs->opaque; + struct IscsiTask iTask; - IscsiAIOCB *acb; + iscsi_co_init_iscsitask(iscsilun, &iTask); - acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque); +retry: + if (iscsi_synchronizecache10_task(iscsilun->iscsi, iscsilun->lun, 0, 0, 0, + 0, iscsi_co_generic_cb, &iTask) == NULL) { + return -EIO; + } - acb->iscsilun = iscsilun; - acb->retries = ISCSI_CMD_RETRIES; + while (!iTask.complete) { + iscsi_set_events(iscsilun); + qemu_coroutine_yield(); + } - if (iscsi_aio_flush_acb(acb) != 0) { - qemu_aio_release(acb); - return NULL; + if (iTask.task != NULL) { + scsi_free_scsi_task(iTask.task); + iTask.task = NULL; } - iscsi_set_events(iscsilun); + if (iTask.do_retry) { + goto retry; + } - return &acb->common; + if (iTask.status != SCSI_STATUS_GOOD) { + return -EIO; + } + + return 0; } #ifdef __linux__ @@ -1599,12 +1409,11 @@ static BlockDriver bdrv_iscsi = { #if defined(LIBISCSI_FEATURE_IOVECTOR) .bdrv_co_get_block_status = iscsi_co_get_block_status, #endif - .bdrv_co_discard = iscsi_co_discard, - .bdrv_co_write_zeroes = iscsi_co_write_zeroes, - - .bdrv_aio_readv = iscsi_aio_readv, - .bdrv_aio_writev = iscsi_aio_writev, - .bdrv_aio_flush = iscsi_aio_flush, + .bdrv_co_discard = iscsi_co_discard, + .bdrv_co_write_zeroes = iscsi_co_write_zeroes, + .bdrv_co_readv = iscsi_co_readv, + .bdrv_co_writev = iscsi_co_writev, + .bdrv_co_flush_to_disk = iscsi_co_flush, .bdrv_has_zero_init = iscsi_has_zero_init, .bdrv_has_discard_zeroes = iscsi_has_discard_zeroes,
this converts read, write and flush functions from aio to coroutines. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/iscsi.c | 405 +++++++++++++++------------------------------------------ 1 file changed, 107 insertions(+), 298 deletions(-)