From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Lieven <pl@kamp.de>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, stefanha@redhat.com, ronniesahlberg@gmail.com
Subject: Re: [Qemu-devel] [PATCH] block/iscsi: bump libiscsi requirement to 1.8.0
Date: Wed, 18 Jun 2014 18:05:09 +0200 [thread overview]
Message-ID: <53A1B8B5.40507@redhat.com> (raw)
In-Reply-To: <1401885206-23144-1-git-send-email-pl@kamp.de>
Il 04/06/2014 14:33, Peter Lieven ha scritto:
> This patch lifts the minimum supported libiscsi version from 1.4.0 to
> 1.8.0 in preparation to an upcoming patch to dynamically switch between
> 10 Byte and 16 Byte CDBs.
>
> On one this allows us to remove nearly all #ifdefs from the code which
> makes the code easier to maintain and read. On the other hand
> I would not recommend libiscsi prior to 1.8.0 for production use
> because the following important libiscsi fixes for deadlocks and
> protocol errors are missing prior to 1.8.0:
>
> dbe9a1e SOCKET queue cmd PDUs directly in waitpdu queue
> 30df192 DATA-OUT set pdu->cmdsn appropriately
> 548bd22 ISCSI fix broken send logic in iscsi_scsi_async_command
> 14bee10 RECONNECT do not increase CmdSN for immediate PDUs
> 1f4a66a PDU queue out PDUs in order of itt.
> 562dd46 PDU avoid incrementing itt to 0xffffffff
> cd09c0f PDU use serial32 arithmetic for cmdsn, maxcmdsn and expcmdsn.
> 89e918e SOCKET validate data_size in in_pdu header
> 91267f5 Limit immediate and unsolicited data to FirstBurstLength
>
> Note that libiscsi 1.8.0 was already released Jan 5th, 2013.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> block/iscsi.c | 66 ---------------------------------------------------------
> configure | 39 +++-------------------------------
> 2 files changed, 3 insertions(+), 102 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1bf9ccd..7394fa6 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -366,17 +366,6 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
>
> lba = sector_qemu2lun(sector_num, iscsilun);
> num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
> -#if !defined(LIBISCSI_FEATURE_IOVECTOR)
> - /* if the iovec only contains one buffer we can pass it directly */
> - if (iov->niov == 1) {
> - data = iov->iov[0].iov_base;
> - } else {
> - 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
> iscsi_co_init_iscsitask(iscsilun, &iTask);
> retry:
> iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
> @@ -387,10 +376,8 @@ retry:
> g_free(buf);
> return -ENOMEM;
> }
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
> iov->niov);
> -#endif
> while (!iTask.complete) {
> iscsi_set_events(iscsilun);
> qemu_coroutine_yield();
> @@ -417,7 +404,6 @@ retry:
> return 0;
> }
>
> -
> static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun,
> int64_t sector_num, int nb_sectors)
> {
> @@ -430,9 +416,6 @@ static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun,
> sector_num / iscsilun->cluster_sectors) == size);
> }
>
> -
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> -
> static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
> int64_t sector_num,
> int nb_sectors, int *pnum)
> @@ -530,9 +513,6 @@ out:
> return ret;
> }
>
> -#endif /* LIBISCSI_FEATURE_IOVECTOR */
> -
> -
> static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
> int64_t sector_num, int nb_sectors,
> QEMUIOVector *iov)
> @@ -541,15 +521,11 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
> struct IscsiTask iTask;
> uint64_t lba;
> uint32_t num_sectors;
> -#if !defined(LIBISCSI_FEATURE_IOVECTOR)
> - int i;
> -#endif
>
> if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
> return -EINVAL;
> }
>
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> if (iscsilun->lbprz && nb_sectors >= ISCSI_CHECKALLOC_THRES &&
> !iscsi_allocationmap_is_allocated(iscsilun, sector_num, nb_sectors)) {
> int64_t ret;
> @@ -563,7 +539,6 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
> return 0;
> }
> }
> -#endif
>
> lba = sector_qemu2lun(sector_num, iscsilun);
> num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
> @@ -581,24 +556,14 @@ retry:
> iTask.task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun, lba,
> num_sectors * iscsilun->block_size,
> iscsilun->block_size,
> -#if !defined(CONFIG_LIBISCSI_1_4) /* API change from 1.4.0 to 1.5.0 */
> 0, 0, 0, 0, 0,
> -#endif
> iscsi_co_generic_cb, &iTask);
> break;
> }
> if (iTask.task == NULL) {
> return -ENOMEM;
> }
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov);
> -#else
> - 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
>
> while (!iTask.complete) {
> iscsi_set_events(iscsilun);
> @@ -753,18 +718,9 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> data.data = acb->ioh->dxferp;
> data.size = acb->ioh->dxfer_len;
> } else {
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> scsi_task_set_iov_out(acb->task,
> (struct scsi_iovec *) acb->ioh->dxferp,
> acb->ioh->iovec_count);
> -#else
> - struct iovec *iov = (struct iovec *)acb->ioh->dxferp;
> -
> - acb->buf = g_malloc(acb->ioh->dxfer_len);
> - data.data = acb->buf;
> - data.size = iov_to_buf(iov, acb->ioh->iovec_count, 0,
> - acb->buf, acb->ioh->dxfer_len);
> -#endif
> }
> }
>
> @@ -784,20 +740,9 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> acb->ioh->dxfer_len,
> acb->ioh->dxferp);
> } else {
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> scsi_task_set_iov_in(acb->task,
> (struct scsi_iovec *) acb->ioh->dxferp,
> acb->ioh->iovec_count);
> -#else
> - int i;
> - for (i = 0; i < acb->ioh->iovec_count; i++) {
> - struct iovec *iov = (struct iovec *)acb->ioh->dxferp;
> -
> - scsi_task_add_data_in_buffer(acb->task,
> - iov[i].iov_len,
> - iov[i].iov_base);
> - }
> -#endif
> }
> }
>
> @@ -806,7 +751,6 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> return &acb->common;
> }
>
> -
> static void ioctl_cb(void *opaque, int status)
> {
> int *p_status = opaque;
> @@ -912,7 +856,6 @@ retry:
> }
>
> #if defined(SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED)
> -
> static int
> coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, BdrvRequestFlags flags)
> @@ -989,7 +932,6 @@ retry:
>
> return 0;
> }
> -
> #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
>
> static void parse_chap(struct iscsi_context *iscsi, const char *target,
> @@ -1101,7 +1043,6 @@ static char *parse_initiator_name(const char *target)
> return iscsi_name;
> }
>
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> static void iscsi_nop_timed_event(void *opaque)
> {
> IscsiLun *iscsilun = opaque;
> @@ -1119,7 +1060,6 @@ static void iscsi_nop_timed_event(void *opaque)
> timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> iscsi_set_events(iscsilun);
> }
> -#endif
>
> static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
> {
> @@ -1258,14 +1198,12 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
> iscsilun->aio_context = new_context;
> iscsi_set_events(iscsilun);
>
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> /* Set up a timer for sending out iSCSI NOPs */
> iscsilun->nop_timer = aio_timer_new(iscsilun->aio_context,
> QEMU_CLOCK_REALTIME, SCALE_MS,
> iscsi_nop_timed_event, iscsilun);
> timer_mod(iscsilun->nop_timer,
> qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> -#endif
> }
>
> /*
> @@ -1457,13 +1395,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
> iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
> iscsilun->block_size) >> BDRV_SECTOR_BITS;
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
> iscsilun->allocationmap =
> bitmap_new(DIV_ROUND_UP(bs->total_sectors,
> iscsilun->cluster_sectors));
> }
> -#endif
> }
>
> out:
> @@ -1652,9 +1588,7 @@ static BlockDriver bdrv_iscsi = {
> .bdrv_truncate = iscsi_truncate,
> .bdrv_refresh_limits = iscsi_refresh_limits,
>
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> .bdrv_co_get_block_status = iscsi_co_get_block_status,
> -#endif
> .bdrv_co_discard = iscsi_co_discard,
> #if defined(SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED)
> .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
> diff --git a/configure b/configure
> index 69b9f56..1ef998e 100755
> --- a/configure
> +++ b/configure
> @@ -3346,46 +3346,20 @@ if compile_prog "" "" ; then
> fi
>
> ##########################################
> -# Do we have libiscsi
> -# We check for iscsi_write16_sync() to make sure we have a
> -# at least version 1.4.0 of libiscsi.
> +# Do we have libiscsi >= 1.8.0
> if test "$libiscsi" != "no" ; then
> - cat > $TMPC << EOF
> -#include <stdio.h>
> -#include <iscsi/iscsi.h>
> -int main(void) { iscsi_write16_sync(NULL,0,0,NULL,0,0,0,0,0,0,0); return 0; }
> -EOF
> - if $pkg_config --atleast-version=1.7.0 libiscsi; then
> + if $pkg_config --atleast-version=1.8.0 libiscsi; then
> libiscsi="yes"
> libiscsi_cflags=$($pkg_config --cflags libiscsi)
> libiscsi_libs=$($pkg_config --libs libiscsi)
> - elif compile_prog "" "-liscsi" ; then
> - libiscsi="yes"
> - libiscsi_libs="-liscsi"
> else
> if test "$libiscsi" = "yes" ; then
> - feature_not_found "libiscsi" "Install libiscsi devel"
> + feature_not_found "libiscsi" "Install libiscsi >= 1.8.0"
> fi
> libiscsi="no"
> fi
> fi
>
> -# We also need to know the API version because there was an
> -# API change from 1.4.0 to 1.5.0.
> -if test "$libiscsi" = "yes"; then
> - cat >$TMPC <<EOF
> -#include <iscsi/iscsi.h>
> -int main(void)
> -{
> - iscsi_read10_task(0, 0, 0, 0, 0, 0, 0);
> - return 0;
> -}
> -EOF
> - if compile_prog "" "-liscsi"; then
> - libiscsi_version="1.4.0"
> - fi
> -fi
> -
> ##########################################
> # Do we need libm
> cat > $TMPC << EOF
> @@ -4160,11 +4134,7 @@ echo "nss used $smartcard_nss"
> echo "libusb $libusb"
> echo "usb net redir $usb_redir"
> echo "GLX support $glx"
> -if test "$libiscsi_version" = "1.4.0"; then
> -echo "libiscsi support $libiscsi (1.4.0)"
> -else
> echo "libiscsi support $libiscsi"
> -fi
> echo "libnfs support $libnfs"
> echo "build guest agent $guest_agent"
> echo "QGA VSS support $guest_agent_with_vss"
> @@ -4519,9 +4489,6 @@ fi
>
> if test "$libiscsi" = "yes" ; then
> echo "CONFIG_LIBISCSI=m" >> $config_host_mak
> - if test "$libiscsi_version" = "1.4.0"; then
> - echo "CONFIG_LIBISCSI_1_4=y" >> $config_host_mak
> - fi
> echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
> echo "LIBISCSI_LIBS=$libiscsi_libs" >> $config_host_mak
> fi
>
Applied now, with further simplifications to bump to 1.9.0.
Paolo
prev parent reply other threads:[~2014-06-18 16:05 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-04 12:33 [Qemu-devel] [PATCH] block/iscsi: bump libiscsi requirement to 1.8.0 Peter Lieven
2014-06-18 16:05 ` Paolo Bonzini [this message]
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=53A1B8B5.40507@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).