[Qemu-devel] [PATCH for-2.4 00/10] various improvements for the iSCSI driver

[Qemu-devel] [PATCH for-2.4 00/10] various improvements for the iSCSI driver

[Peter Lieven]

Peter Lieven (10):
  block/iscsi: do not forget to logout from target
  block/iscsi: change all iscsilun properties from uint8_t to bool
  block/iscsi: rename iscsi_write_protected and let it return void
  block/iscsi: store DPOFUA bit from the modesense command
  block/iscsi: optimize WRITE10/16 if cache.writeback is not set
  block/iscsi: increase retry count
  block/iscsi: bump libiscsi requirement to 1.10.0
  block/iscsi: handle SCSI_STATUS_TASK_SET_FULL
  block/iscsi: bump year in copyright notice
  block/iscsi: use the allocationmap also if cache.direct=on

 block/iscsi.c | 58 +++++++++++++++++++++++++++++++++++++++-------------------
 configure     |  6 +++---
 2 files changed, 42 insertions(+), 22 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 01/10] block/iscsi: do not forget to logout from target

[Peter Lieven]

We actually were always impolitely dropping the connection and
not cleanly logging out.

CC: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index ba33290..be8af46 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1501,6 +1501,9 @@ out:
 
     if (ret) {
         if (iscsi != NULL) {
+            if (iscsi_is_logged_in(iscsi)) {
+                iscsi_logout_sync(iscsi);
+            }
             iscsi_destroy_context(iscsi);
         }
         memset(iscsilun, 0, sizeof(IscsiLun));
@@ -1514,6 +1517,9 @@ static void iscsi_close(BlockDriverState *bs)
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
     iscsi_detach_aio_context(bs);
+    if (iscsi_is_logged_in(iscsi)) {
+        iscsi_logout_sync(iscsi);
+    }
     iscsi_destroy_context(iscsi);
     g_free(iscsilun->zeroblock);
     g_free(iscsilun->allocationmap);
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 02/10] block/iscsi: change all iscsilun properties from uint8_t to bool

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index be8af46..6cf7e99 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -57,9 +57,6 @@ typedef struct IscsiLun {
     int events;
     QEMUTimer *nop_timer;
     QEMUTimer *event_timer;
-    uint8_t lbpme;
-    uint8_t lbprz;
-    uint8_t has_write_same;
     struct scsi_inquiry_logical_block_provisioning lbp;
     struct scsi_inquiry_block_limits bl;
     unsigned char *zeroblock;
@@ -67,6 +64,9 @@ typedef struct IscsiLun {
     int cluster_sectors;
     bool use_16_for_rw;
     bool write_protected;
+    bool lbpme;
+    bool lbprz;
+    bool has_write_same;
 } IscsiLun;
 
 typedef struct IscsiTask {
@@ -460,7 +460,7 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
     *pnum = nb_sectors;
 
     /* LUN does not support logical block provisioning */
-    if (iscsilun->lbpme == 0) {
+    if (!iscsilun->lbpme) {
         goto out;
     }
 
@@ -1121,8 +1121,8 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
                 } else {
                     iscsilun->block_size = rc16->block_length;
                     iscsilun->num_blocks = rc16->returned_lba + 1;
-                    iscsilun->lbpme = rc16->lbpme;
-                    iscsilun->lbprz = rc16->lbprz;
+                    iscsilun->lbpme = !!rc16->lbpme;
+                    iscsilun->lbprz = !!rc16->lbprz;
                     iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff);
                 }
             }
@@ -1655,7 +1655,7 @@ out:
 static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     IscsiLun *iscsilun = bs->opaque;
-    bdi->unallocated_blocks_are_zero = !!iscsilun->lbprz;
+    bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
     bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
     bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
     return 0;
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 03/10] block/iscsi: rename iscsi_write_protected and let it return void

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6cf7e99..221c9fc 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1253,11 +1253,11 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
                                           iscsi_timed_set_events, iscsilun);
 }
 
-static bool iscsi_is_write_protected(IscsiLun *iscsilun)
+static void iscsi_modesense_sync(IscsiLun *iscsilun)
 {
     struct scsi_task *task;
     struct scsi_mode_sense *ms = NULL;
-    bool wrprotected = false;
+    iscsilun->write_protected = false;
 
     task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
                                  1, SCSI_MODESENSE_PC_CURRENT,
@@ -1278,13 +1278,12 @@ static bool iscsi_is_write_protected(IscsiLun *iscsilun)
                      iscsi_get_error(iscsilun->iscsi));
         goto out;
     }
-    wrprotected = ms->device_specific_parameter & 0x80;
+    iscsilun->write_protected = ms->device_specific_parameter & 0x80;
 
 out:
     if (task) {
         scsi_free_scsi_task(task);
     }
-    return wrprotected;
 }
 
 /*
@@ -1403,7 +1402,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     scsi_free_scsi_task(task);
     task = NULL;
 
-    iscsilun->write_protected = iscsi_is_write_protected(iscsilun);
+    iscsi_modesense_sync(iscsilun);
+
     /* Check the write protect flag of the LUN if we want to write */
     if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
         iscsilun->write_protected) {
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 04/10] block/iscsi: store DPOFUA bit from the modesense command

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 221c9fc..237faa1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -66,6 +66,7 @@ typedef struct IscsiLun {
     bool write_protected;
     bool lbpme;
     bool lbprz;
+    bool dpofua;
     bool has_write_same;
 } IscsiLun;
 
@@ -1258,6 +1259,7 @@ static void iscsi_modesense_sync(IscsiLun *iscsilun)
     struct scsi_task *task;
     struct scsi_mode_sense *ms = NULL;
     iscsilun->write_protected = false;
+    iscsilun->dpofua = false;
 
     task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun,
                                  1, SCSI_MODESENSE_PC_CURRENT,
@@ -1279,6 +1281,7 @@ static void iscsi_modesense_sync(IscsiLun *iscsilun)
         goto out;
     }
     iscsilun->write_protected = ms->device_specific_parameter & 0x80;
+    iscsilun->dpofua          = ms->device_specific_parameter & 0x10;
 
 out:
     if (task) {
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Peter Lieven]

SCSI allowes to tell the target to not return from a write command
if the date is not written to the disk. Use this so called FUA
bit if it is supported to optimize WRITE commands if writeback is
not allowed.

In this case qemu always issues a WRITE followed by a FLUSH. This
is 2 round trip times. If we set the FUA bit we can ignore the
following FLUSH.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 237faa1..7fb04d7 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -68,6 +68,7 @@ typedef struct IscsiLun {
     bool lbprz;
     bool dpofua;
     bool has_write_same;
+    bool force_next_flush;
 } IscsiLun;
 
 typedef struct IscsiTask {
@@ -370,6 +371,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
     struct IscsiTask iTask;
     uint64_t lba;
     uint32_t num_sectors;
+    int fua = iscsilun->dpofua && !bs->enable_write_cache;
 
     if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
         return -EINVAL;
@@ -388,12 +390,12 @@ retry:
     if (iscsilun->use_16_for_rw) {
         iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
                                         NULL, num_sectors * iscsilun->block_size,
-                                        iscsilun->block_size, 0, 0, 0, 0, 0,
+                                        iscsilun->block_size, 0, 0, fua, 0, 0,
                                         iscsi_co_generic_cb, &iTask);
     } else {
         iTask.task = iscsi_write10_task(iscsilun->iscsi, iscsilun->lun, lba,
                                         NULL, num_sectors * iscsilun->block_size,
-                                        iscsilun->block_size, 0, 0, 0, 0, 0,
+                                        iscsilun->block_size, 0, 0, fua, 0, 0,
                                         iscsi_co_generic_cb, &iTask);
     }
     if (iTask.task == NULL) {
@@ -621,6 +623,11 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
         return 0;
     }
 
+    if (iscsilun->dpofua && !bs->enable_write_cache &&
+        !iscsilun->force_next_flush) {
+        return 0;
+    }
+
     iscsi_co_init_iscsitask(iscsilun, &iTask);
 
 retry:
@@ -648,6 +655,8 @@ retry:
         return -EIO;
     }
 
+    iscsilun->force_next_flush = false;
+
     return 0;
 }
 
@@ -969,6 +978,8 @@ retry:
         iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
     }
 
+    iscsilun->force_next_flush = true;
+
     return 0;
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 06/10] block/iscsi: increase retry count

[Peter Lieven]

The idea is that a command is retried in a BUSY condition
up a time of approx. 60 seconds before it is failed. This should
be far higher than any command timeout in the guest.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 7fb04d7..a4902ea 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -102,7 +102,7 @@ typedef struct IscsiAIOCB {
 #define NOP_INTERVAL 5000
 #define MAX_NOP_FAILURES 3
 #define ISCSI_CMD_RETRIES ARRAY_SIZE(iscsi_retry_times)
-static const unsigned iscsi_retry_times[] = {8, 32, 128, 512, 2048};
+static const unsigned iscsi_retry_times[] = {8, 32, 128, 512, 2048, 8192, 32768};
 
 /* this threshold is a trade-off knob to choose between
  * the potential additional overhead of an extra GET_LBA_STATUS request
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Peter Lieven]

We need this to support SCSI_STATUS_TASK_SET_FULL.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 configure | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 6969f6f..f73b8d0 100755
--- a/configure
+++ b/configure
@@ -3630,15 +3630,15 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
-# Do we have libiscsi >= 1.9.0
+# Do we have libiscsi >= 1.10.0
 if test "$libiscsi" != "no" ; then
-  if $pkg_config --atleast-version=1.9.0 libiscsi; then
+  if $pkg_config --atleast-version=1.10.0 libiscsi; then
     libiscsi="yes"
     libiscsi_cflags=$($pkg_config --cflags libiscsi)
     libiscsi_libs=$($pkg_config --libs libiscsi)
   else
     if test "$libiscsi" = "yes" ; then
-      feature_not_found "libiscsi" "Install libiscsi >= 1.9.0"
+      feature_not_found "libiscsi" "Install libiscsi >= 1.10.0"
     fi
     libiscsi="no"
   fi
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 08/10] block/iscsi: handle SCSI_STATUS_TASK_SET_FULL

[Peter Lieven]

a target may issue a SCSI_STATUS_TASK_SET_FULL status
if there is more than one "BUSY" command queued already.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index a4902ea..3d0ffeb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -185,10 +185,10 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 iTask->do_retry = 1;
                 goto out;
             }
-            if (status == SCSI_STATUS_BUSY) {
+            if (status == SCSI_STATUS_BUSY || status == SCSI_STATUS_TASK_SET_FULL) {
                 unsigned retry_time =
                     exp_random(iscsi_retry_times[iTask->retries - 1]);
-                error_report("iSCSI Busy (retry #%u in %u ms): %s",
+                error_report("iSCSI Busy/TaskSetFull (retry #%u in %u ms): %s",
                              iTask->retries, retry_time,
                              iscsi_get_error(iscsi));
                 aio_timer_init(iTask->iscsilun->aio_context,
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 09/10] block/iscsi: bump year in copyright notice

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 3d0ffeb..04c1309 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2,7 +2,7 @@
  * QEMU Block driver for iSCSI images
  *
  * Copyright (c) 2010-2011 Ronnie Sahlberg <ronniesahlberg@gmail.com>
- * Copyright (c) 2012-2014 Peter Lieven <pl@kamp.de>
+ * Copyright (c) 2012-2015 Peter Lieven <pl@kamp.de>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
-- 
1.9.1

[Qemu-devel] [PATCH for-2.4 10/10] block/iscsi: use the allocationmap also if cache.direct=on

[Peter Lieven]

the allocationmap has only a hint character. The driver always
double checks that blocks marked unallocated in the cache are
still unallocated before taking the fast path and return zeroes.
So using the allocationmap is migration safe and can
also be enabled with cache.direct=on.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 04c1309..0737354 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1495,7 +1495,7 @@ 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 (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
+        if (iscsilun->lbprz) {
             iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
             if (iscsilun->allocationmap == NULL) {
                 ret = -ENOMEM;
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Paolo Bonzini]

On 16/04/2015 14:18, Peter Lieven wrote:
> We need this to support SCSI_STATUS_TASK_SET_FULL.

Any reason apart from the missing constant?

Paolo

> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  configure | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 6969f6f..f73b8d0 100755
> --- a/configure
> +++ b/configure
> @@ -3630,15 +3630,15 @@ if compile_prog "" "" ; then
>  fi
>  
>  ##########################################
> -# Do we have libiscsi >= 1.9.0
> +# Do we have libiscsi >= 1.10.0
>  if test "$libiscsi" != "no" ; then
> -  if $pkg_config --atleast-version=1.9.0 libiscsi; then
> +  if $pkg_config --atleast-version=1.10.0 libiscsi; then
>      libiscsi="yes"
>      libiscsi_cflags=$($pkg_config --cflags libiscsi)
>      libiscsi_libs=$($pkg_config --libs libiscsi)
>    else
>      if test "$libiscsi" = "yes" ; then
> -      feature_not_found "libiscsi" "Install libiscsi >= 1.9.0"
> +      feature_not_found "libiscsi" "Install libiscsi >= 1.10.0"
>      fi
>      libiscsi="no"
>    fi
> 

Re: [Qemu-devel] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Paolo Bonzini]

On 16/04/2015 14:18, Peter Lieven wrote:
> SCSI allowes to tell the target to not return from a write command
> if the date is not written to the disk. Use this so called FUA
> bit if it is supported to optimize WRITE commands if writeback is
> not allowed.
> 
> In this case qemu always issues a WRITE followed by a FLUSH. This
> is 2 round trip times. If we set the FUA bit we can ignore the
> following FLUSH.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 237faa1..7fb04d7 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -68,6 +68,7 @@ typedef struct IscsiLun {
>      bool lbprz;
>      bool dpofua;
>      bool has_write_same;
> +    bool force_next_flush;
>  } IscsiLun;
>  
>  typedef struct IscsiTask {
> @@ -370,6 +371,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
>      struct IscsiTask iTask;
>      uint64_t lba;
>      uint32_t num_sectors;
> +    int fua = iscsilun->dpofua && !bs->enable_write_cache;
>  
>      if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
>          return -EINVAL;
> @@ -388,12 +390,12 @@ retry:
>      if (iscsilun->use_16_for_rw) {
>          iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                          NULL, num_sectors * iscsilun->block_size,
> -                                        iscsilun->block_size, 0, 0, 0, 0, 0,
> +                                        iscsilun->block_size, 0, 0, fua, 0, 0,
>                                          iscsi_co_generic_cb, &iTask);
>      } else {
>          iTask.task = iscsi_write10_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                          NULL, num_sectors * iscsilun->block_size,
> -                                        iscsilun->block_size, 0, 0, 0, 0, 0,
> +                                        iscsilun->block_size, 0, 0, fua, 0, 0,
>                                          iscsi_co_generic_cb, &iTask);
>      }
>      if (iTask.task == NULL) {
> @@ -621,6 +623,11 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
>          return 0;
>      }
>  
> +    if (iscsilun->dpofua && !bs->enable_write_cache &&
> +        !iscsilun->force_next_flush) {
> +        return 0;
> +    }
> +
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  
>  retry:
> @@ -648,6 +655,8 @@ retry:
>          return -EIO;
>      }
>  
> +    iscsilun->force_next_flush = false;

You still need a flush if you do WRITE SAME, WRITE+FUA, WRITE+FUA.
Also, since bs->enable_write_cache can be toggled arbitrarily, I would
prefer to set force_next_flush on all non-FUA writes, including those
done with bs->enable_write_cache.

>      return 0;
>  }
>  
> @@ -969,6 +978,8 @@ retry:
>          iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
>      }
>  
> +    iscsilun->force_next_flush = true;

Also, I think it is iscsi_co_generic_cb that should set
force_next_flush, so that it is only set on failure.  Not really for the
optimization value, but because it's clearer.

I think that if you do these changes, iscsi_co_flush can just check "if
(!iscsilun->force_next_flush)".

But still---nice approach. :)

>      return 0;
>  }
>  
> 

Re: [Qemu-devel] [PATCH for-2.4 00/10] various improvements for the iSCSI driver

[Paolo Bonzini]

On 16/04/2015 14:18, Peter Lieven wrote:
> Peter Lieven (10):
>   block/iscsi: do not forget to logout from target
>   block/iscsi: change all iscsilun properties from uint8_t to bool
>   block/iscsi: rename iscsi_write_protected and let it return void
>   block/iscsi: store DPOFUA bit from the modesense command
>   block/iscsi: optimize WRITE10/16 if cache.writeback is not set
>   block/iscsi: increase retry count
>   block/iscsi: bump libiscsi requirement to 1.10.0
>   block/iscsi: handle SCSI_STATUS_TASK_SET_FULL
>   block/iscsi: bump year in copyright notice
>   block/iscsi: use the allocationmap also if cache.direct=on
> 
>  block/iscsi.c | 58 +++++++++++++++++++++++++++++++++++++++-------------------
>  configure     |  6 +++---
>  2 files changed, 42 insertions(+), 22 deletions(-)
> 

Applied 1, 2, 3, 6, 9, 10.

Thanks!

Paolo

Re: [Qemu-devel] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Peter Lieven]

Am 16.04.2015 um 14:33 schrieb Paolo Bonzini:
>
> On 16/04/2015 14:18, Peter Lieven wrote:
>> We need this to support SCSI_STATUS_TASK_SET_FULL.
> Any reason apart from the missing constant?

No, but I wanted to avoid starting checking for constants that were added shortly after this.
You can't check with #ifdef for a constant in an enum.
Libiscsi 1.10 was released in September 2013.

Peter


>
> Paolo
>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   configure | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 6969f6f..f73b8d0 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3630,15 +3630,15 @@ if compile_prog "" "" ; then
>>   fi
>>   
>>   ##########################################
>> -# Do we have libiscsi >= 1.9.0
>> +# Do we have libiscsi >= 1.10.0
>>   if test "$libiscsi" != "no" ; then
>> -  if $pkg_config --atleast-version=1.9.0 libiscsi; then
>> +  if $pkg_config --atleast-version=1.10.0 libiscsi; then
>>       libiscsi="yes"
>>       libiscsi_cflags=$($pkg_config --cflags libiscsi)
>>       libiscsi_libs=$($pkg_config --libs libiscsi)
>>     else
>>       if test "$libiscsi" = "yes" ; then
>> -      feature_not_found "libiscsi" "Install libiscsi >= 1.9.0"
>> +      feature_not_found "libiscsi" "Install libiscsi >= 1.10.0"
>>       fi
>>       libiscsi="no"
>>     fi
>>


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................

Re: [Qemu-devel] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Peter Lieven]

Am 16.04.2015 um 14:42 schrieb Paolo Bonzini:
>
> On 16/04/2015 14:18, Peter Lieven wrote:
>> SCSI allowes to tell the target to not return from a write command
>> if the date is not written to the disk. Use this so called FUA
>> bit if it is supported to optimize WRITE commands if writeback is
>> not allowed.
>>
>> In this case qemu always issues a WRITE followed by a FLUSH. This
>> is 2 round trip times. If we set the FUA bit we can ignore the
>> following FLUSH.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/iscsi.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 237faa1..7fb04d7 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -68,6 +68,7 @@ typedef struct IscsiLun {
>>       bool lbprz;
>>       bool dpofua;
>>       bool has_write_same;
>> +    bool force_next_flush;
>>   } IscsiLun;
>>   
>>   typedef struct IscsiTask {
>> @@ -370,6 +371,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
>>       struct IscsiTask iTask;
>>       uint64_t lba;
>>       uint32_t num_sectors;
>> +    int fua = iscsilun->dpofua && !bs->enable_write_cache;
>>   
>>       if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
>>           return -EINVAL;
>> @@ -388,12 +390,12 @@ retry:
>>       if (iscsilun->use_16_for_rw) {
>>           iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
>>                                           NULL, num_sectors * iscsilun->block_size,
>> -                                        iscsilun->block_size, 0, 0, 0, 0, 0,
>> +                                        iscsilun->block_size, 0, 0, fua, 0, 0,
>>                                           iscsi_co_generic_cb, &iTask);
>>       } else {
>>           iTask.task = iscsi_write10_task(iscsilun->iscsi, iscsilun->lun, lba,
>>                                           NULL, num_sectors * iscsilun->block_size,
>> -                                        iscsilun->block_size, 0, 0, 0, 0, 0,
>> +                                        iscsilun->block_size, 0, 0, fua, 0, 0,
>>                                           iscsi_co_generic_cb, &iTask);
>>       }
>>       if (iTask.task == NULL) {
>> @@ -621,6 +623,11 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)
>>           return 0;
>>       }
>>   
>> +    if (iscsilun->dpofua && !bs->enable_write_cache &&
>> +        !iscsilun->force_next_flush) {
>> +        return 0;
>> +    }
>> +
>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>   
>>   retry:
>> @@ -648,6 +655,8 @@ retry:
>>           return -EIO;
>>       }
>>   
>> +    iscsilun->force_next_flush = false;
> You still need a flush if you do WRITE SAME, WRITE+FUA, WRITE+FUA.
> Also, since bs->enable_write_cache can be toggled arbitrarily, I would
> prefer to set force_next_flush on all non-FUA writes, including those
> done with bs->enable_write_cache.

Good idea. So we can avoid flushes if there was never a write before.

>
>>       return 0;
>>   }
>>   
>> @@ -969,6 +978,8 @@ retry:
>>           iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
>>       }
>>   
>> +    iscsilun->force_next_flush = true;
> Also, I think it is iscsi_co_generic_cb that should set
> force_next_flush, so that it is only set on failure.  Not really for the
> optimization value, but because it's clearer.

I don't get what you mean with it should only "set on failure".
My approach would be to add a flag to the iTask that the command
requires to set force_flush after successful completion. Is it that
what you mean?

>
> I think that if you do these changes, iscsi_co_flush can just check "if
> (!iscsilun->force_next_flush)".
>
> But still---nice approach. :)

Thanks :-)

Peter

Re: [Qemu-devel] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Paolo Bonzini]

On 16/04/2015 15:02, Peter Lieven wrote:
>>>
>> Also, I think it is iscsi_co_generic_cb that should set
>> force_next_flush, so that it is only set on failure.  Not really for the
>> optimization value, but because it's clearer.
> 
> I don't get what you mean with it should only "set on failure".
> My approach would be to add a flag to the iTask that the command
> requires to set force_flush after successful completion. Is it that
> what you mean?

Set on success.  Lack of sleep.

Paolo

Re: [Qemu-devel] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Paolo Bonzini]

On 16/04/2015 14:58, Peter Lieven wrote:
>>
>> On 16/04/2015 14:18, Peter Lieven wrote:
>>> We need this to support SCSI_STATUS_TASK_SET_FULL.
>> Any reason apart from the missing constant?
> 
> No, but I wanted to avoid starting checking for constants that were
> added shortly after this.
> You can't check with #ifdef for a constant in an enum.

But you can #define it if libiscsi version is <1.10.

> Libiscsi 1.10 was released in September 2013.

But RHEL7 only has libiscsi 1.9 for example (it froze in August 2013),
and it won't upgrade ever due to ABI breakage in libiscsi.

Paolo

Re: [Qemu-devel] [PATCH for-2.4 05/10] block/iscsi: optimize WRITE10/16 if cache.writeback is not set

[Peter Lieven]

Am 16.04.2015 um 15:17 schrieb Paolo Bonzini:
>
> On 16/04/2015 15:02, Peter Lieven wrote:
>>> Also, I think it is iscsi_co_generic_cb that should set
>>> force_next_flush, so that it is only set on failure.  Not really for the
>>> optimization value, but because it's clearer.
>> I don't get what you mean with it should only "set on failure".
>> My approach would be to add a flag to the iTask that the command
>> requires to set force_flush after successful completion. Is it that
>> what you mean?
> Set on success.  Lack of sleep.

I have send a v2 following your suggestions.

Peter

Re: [Qemu-devel] [PATCH for-2.4 07/10] block/iscsi: bump libiscsi requirement to 1.10.0

[Peter Lieven]

Am 16.04.2015 um 15:20 schrieb Paolo Bonzini:
>
> On 16/04/2015 14:58, Peter Lieven wrote:
>>> On 16/04/2015 14:18, Peter Lieven wrote:
>>>> We need this to support SCSI_STATUS_TASK_SET_FULL.
>>> Any reason apart from the missing constant?
>> No, but I wanted to avoid starting checking for constants that were
>> added shortly after this.
>> You can't check with #ifdef for a constant in an enum.
> But you can #define it if libiscsi version is <1.10.

There is no macro to check for that. I took the easy way
hardcoding the value. Its an official standard so there is
no chance it will change.

Peter