From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Lieven <pl@kamp.de>
Cc: kwolf@redhat.com, ronniesahlberg@gmail.com,
qemu-devel@nongnu.org, stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH] block/iscsi: introduce bdrv_co_{readv, writev, flush_to_disk}
Date: Wed, 02 Oct 2013 17:46:22 +0200 [thread overview]
Message-ID: <524C3FCE.4090908@redhat.com> (raw)
In-Reply-To: <1380728469-29435-1-git-send-email-pl@kamp.de>
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,
>
next prev parent reply other threads:[~2013-10-02 15:46 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-02 15:41 [Qemu-devel] [PATCH] block/iscsi: introduce bdrv_co_{readv, writev, flush_to_disk} Peter Lieven
2013-10-02 15:46 ` Paolo Bonzini [this message]
2013-10-02 15:54 ` Peter Lieven
2013-10-02 15:55 ` Paolo Bonzini
2013-10-02 15:57 ` Peter Lieven
2013-10-08 12:33 ` Kevin Wolf
2013-10-08 12:35 ` Paolo Bonzini
2013-10-08 12:39 ` Kevin Wolf
2013-10-11 7:35 ` Peter Lieven
2013-11-25 10:47 ` Peter Lieven
2013-11-25 10:54 ` Paolo Bonzini
2013-11-25 10:56 ` Peter Lieven
2013-11-25 10:57 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=524C3FCE.4090908@redhat.com \
--to=pbonzini@redhat.com \
--cc=kwolf@redhat.com \
--cc=pl@kamp.de \
--cc=qemu-devel@nongnu.org \
--cc=ronniesahlberg@gmail.com \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).