From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsDAc-0006gV-IS for qemu-devel@nongnu.org; Wed, 04 Jun 2014 11:32:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WsDAO-0004af-Nq for qemu-devel@nongnu.org; Wed, 04 Jun 2014 11:32:06 -0400 Received: from mail-qg0-x22a.google.com ([2607:f8b0:400d:c04::22a]:37840) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsDAO-0004a7-Bq for qemu-devel@nongnu.org; Wed, 04 Jun 2014 11:31:52 -0400 Received: by mail-qg0-f42.google.com with SMTP id q107so16177012qgd.29 for ; Wed, 04 Jun 2014 08:31:51 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <538F3BE3.4020200@redhat.com> Date: Wed, 04 Jun 2014 17:31:47 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1401889659-24035-1-git-send-email-pl@kamp.de> In-Reply-To: <1401889659-24035-1-git-send-email-pl@kamp.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com, ronniesahlberg@gmail.com Il 04/06/2014 15:47, Peter Lieven ha scritto: > this patch changes the driver to uses 16 Byte CDBs for > READ/WRITE only if the target requires 64bit lba addressing. > > On one hand this saves 6 bytes in each PDU on the other > hand it seems that 10 Byte CDBs seems to be much better > supported and tested as a recent issue I had with a > major storage supplier lined out. > > For WRITESAME the logic is a bit more tricky as WRITESAME10 > with UNMAP was added really late. Thus a fallback to WRITESAME16 > is possible if it supports UNMAP and WRITESAME10 not. > > Signed-off-by: Peter Lieven > --- > block/iscsi.c | 58 +++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 40 insertions(+), 18 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index d241e83..019b324 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -65,6 +65,7 @@ typedef struct IscsiLun { > unsigned char *zeroblock; > unsigned long *allocationmap; > int cluster_sectors; > + bool use_16_for_rw; > } IscsiLun; > > typedef struct IscsiTask { > @@ -368,10 +369,17 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, > num_sectors = sector_qemu2lun(nb_sectors, iscsilun); > 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 (iscsilun->use_16_for_rw) { > + 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); > + } else { > + iTask.task = iscsi_write10_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 -ENOMEM; > @@ -545,20 +553,17 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, > > iscsi_co_init_iscsitask(iscsilun, &iTask); > retry: > - switch (iscsilun->type) { > - case TYPE_DISK: > + if (iscsilun->use_16_for_rw) { > 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: > + } else { > 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; > } > if (iTask.task == NULL) { > return -ENOMEM; > @@ -864,19 +869,27 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, > struct IscsiTask iTask; > uint64_t lba; > uint32_t nb_blocks; > + bool use_16_for_ws = iscsilun->use_16_for_rw; > > if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > return -EINVAL; > } > > - if ((flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->lbp.lbpws) { > - /* WRITE SAME with UNMAP is not supported by the target, > - * fall back and try WRITE SAME without UNMAP */ > - flags &= ~BDRV_REQ_MAY_UNMAP; > + if (flags & BDRV_REQ_MAY_UNMAP) { > + if (!use_16_for_ws && !iscsilun->lbp.lbpws10) { > + /* WRITESAME10 with UNMAP is unsupported try WRITESAME16 */ > + use_16_for_ws = true; > + } > + if (use_16_for_ws && !iscsilun->lbp.lbpws) { > + /* WRITESAME16 with UNMAP is not supported by the target, > + * fall back and try WRITESAME10/16 without UNMAP */ > + flags &= ~BDRV_REQ_MAY_UNMAP; > + use_16_for_ws = iscsilun->use_16_for_rw; > + } > } > > if (!(flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->has_write_same) { > - /* WRITE SAME without UNMAP is not supported by the target */ > + /* WRITESAME without UNMAP is not supported by the target */ > return -ENOTSUP; > } > > @@ -889,10 +902,18 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, > > iscsi_co_init_iscsitask(iscsilun, &iTask); > retry: > - if (iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba, > - iscsilun->zeroblock, iscsilun->block_size, > - nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP), > - 0, 0, iscsi_co_generic_cb, &iTask) == NULL) { > + if (use_16_for_ws) { > + iTask.task = iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba, > + iscsilun->zeroblock, iscsilun->block_size, > + nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP), > + 0, 0, iscsi_co_generic_cb, &iTask); > + } else { > + iTask.task = iscsi_writesame10_task(iscsilun->iscsi, iscsilun->lun, lba, > + iscsilun->zeroblock, iscsilun->block_size, > + nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP), > + 0, 0, iscsi_co_generic_cb, &iTask); > + } > + if (iTask.task == NULL) { > return -ENOMEM; > } > > @@ -1087,6 +1108,7 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp) > iscsilun->num_blocks = rc16->returned_lba + 1; > iscsilun->lbpme = rc16->lbpme; > iscsilun->lbprz = rc16->lbprz; > + iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff); > } > } > break; > Thanks, applying this to scsi-next (once Stefan pushes his patches). Paolo