various improvements for the iSCSI driver

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 <[email protected]>

---
 block/iscsi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

block/iscsi.c +9 -2

index 237faa1..7fb04d7 100644
@@ -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:

block/iscsi.c +0 -0

     return 0;
 }
 

Quoted 65 lines

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 <[email protected]>

---
  block/iscsi.c | 15 +++++++++++++--
  1 file changed, 13 insertions(+), 2 deletions(-)

block/iscsi.c +9 -2

index 237faa1..7fb04d7 100644
@@ -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;
      }
 

block/iscsi.c retry: · line 658

Quoted 1 line

block/iscsi.c +1 -0

+    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.

Quoted 4 lines

block/iscsi.c +0 -0

      return 0;
  }
 

block/iscsi.c retry: · line 981

Quoted 5 lines

block/iscsi.c +1 -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. :)

Quoted 4 lines

Quoted 1 line

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?

Quoted 7 lines

Quoted 3 lines

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.

We need this to support SCSI_STATUS_TASK_SET_FULL.

Signed-off-by Peter Lieven <[email protected]>

---
 configure | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

configure +3 -3

index 6969f6f..f73b8d0 100755
@@ -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

Quoted 1 line

We need this to support SCSI_STATUS_TASK_SET_FULL.

Any reason apart from the missing constant?

Paolo

Quoted 29 lines

Signed-off-by Peter Lieven <[email protected]>

---
  configure | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

configure +3 -3

index 6969f6f..f73b8d0 100755
@@ -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

Quoted 4 lines · Peter Lieven

Quoted 1 line

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.

Quoted 6 lines

Quoted 3 lines · Peter Lieven

Quoted 1 line

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.

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(-)