[libvirt] [PATCH v2] storage: add wipeVol to iscsi-direct storage backend

clem@lse.epita.fr posted 1 patch 5 years, 8 months ago
Failed in applying to current master (apply log)
src/storage/storage_backend_iscsi_direct.c | 173 ++++++++++++++++++---
1 file changed, 152 insertions(+), 21 deletions(-)
[libvirt] [PATCH v2] storage: add wipeVol to iscsi-direct storage backend
Posted by clem@lse.epita.fr 5 years, 8 months ago
From: Clementine Hayat <clem@lse.epita.fr>

Change set volume capacity to get volume capacity and put the connection
and helpers in one function to avoid code duplicates.

Signed-off-by: Clementine Hayat <clem@lse.epita.fr>
---

Set BLOCK_PER_PACKET to 128. Not sure about this value. Should be
potentially tuned.

 src/storage/storage_backend_iscsi_direct.c | 173 ++++++++++++++++++---
 1 file changed, 152 insertions(+), 21 deletions(-)

diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
index 0764356b62..958f5937f9 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -41,6 +41,8 @@
 
 #define ISCSI_DEFAULT_TARGET_PORT 3260
 #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
+#define BLOCK_PER_PACKET 128
+#define VOL_NAME_PREFIX "unit:0:0:"
 
 VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
 
@@ -225,7 +227,7 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
 
-    if (virAsprintf(&vol->name, "unit:0:0:%u", lun) < 0)
+    if (virAsprintf(&vol->name, "%s%u", VOL_NAME_PREFIX, lun) < 0)
         return -1;
     if (virAsprintf(&vol->key, "ip-%s-iscsi-%s-lun-%u", portal,
                     def->source.devices[0].path, lun) < 0)
@@ -237,13 +239,13 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
 }
 
 static int
-virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
-                                virStorageVolDefPtr vol,
-                                int lun)
+virISCSIDirectGetVolumeCapacity(struct iscsi_context *iscsi,
+                                int lun,
+                                uint32_t *block_size,
+                                uint32_t *nb_block)
 {
     struct scsi_task *task = NULL;
     struct scsi_inquiry_standard *inq = NULL;
-    long long size = 0;
     int ret = -1;
 
     if (!(task = iscsi_inquiry_sync(iscsi, lun, 0, 0, 64)) ||
@@ -282,10 +284,8 @@ virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
             goto cleanup;
         }
 
-        size  = rc10->block_size;
-        size *= rc10->lba;
-        vol->target.capacity = size;
-        vol->target.allocation = size;
+        *block_size  = rc10->block_size;
+        *nb_block = rc10->lba;
 
     }
 
@@ -303,6 +303,8 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     virStorageVolDefPtr vol = NULL;
+    uint32_t block_size;
+    uint32_t nb_block;
     int ret = -1;
 
     virStoragePoolObjClearVols(pool);
@@ -314,9 +316,12 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
 
     vol->type = VIR_STORAGE_VOL_NETWORK;
 
-    if (virISCSIDirectSetVolumeCapacity(iscsi, vol, lun) < 0)
+    if (virISCSIDirectGetVolumeCapacity(iscsi, lun,
+                                        &block_size, &nb_block) < 0)
         goto cleanup;
 
+    vol->target.capacity = block_size * nb_block;
+    vol->target.allocation = block_size * nb_block;
     def->capacity += vol->target.capacity;
     def->allocation += vol->target.allocation;
 
@@ -553,24 +558,33 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
     virStoragePoolSourceFree(source);
     return ret;
 }
+static int
+virStorageBackendISCSIDirectSetConnection(virStoragePoolObjPtr pool,
+                                          struct iscsi_context **iscsi,
+                                          char **portal)
+{
+    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+
+    if (!(*iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn)))
+        return -1;
+    if (!(*portal = virStorageBackendISCSIDirectPortal(&def->source)))
+        return -1;
+    if (virStorageBackendISCSIDirectSetAuth(*iscsi, &def->source) < 0)
+        return -1;
+    if (virISCSIDirectSetContext(*iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
+        return -1;
+    if (virISCSIDirectConnect(*iscsi, *portal) < 0)
+        return -1;
+    return 0;
+}
 
 static int
 virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
 {
-    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     struct iscsi_context *iscsi = NULL;
     char *portal = NULL;
     int ret = -1;
-
-    if (!(iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn)))
-        goto cleanup;
-    if (!(portal = virStorageBackendISCSIDirectPortal(&def->source)))
-        goto cleanup;
-    if (virStorageBackendISCSIDirectSetAuth(iscsi, &def->source) < 0)
-        goto cleanup;
-    if (virISCSIDirectSetContext(iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
-        goto cleanup;
-    if (virISCSIDirectConnect(iscsi, portal) < 0)
+    if (virStorageBackendISCSIDirectSetConnection(pool, &iscsi, &portal) <0)
         goto cleanup;
     if (virISCSIDirectReportLuns(pool, iscsi, portal) < 0)
         goto disconect;
@@ -584,12 +598,129 @@ virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
     return ret;
 }
 
+static int
+virStorageBackendISCSIDirectGetLun(virStorageVolDefPtr vol,
+                                   int *lun)
+{
+    const char *name = vol->name;
+    int ret = -1;
+
+    if (!STRPREFIX(name, VOL_NAME_PREFIX)) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Invalid volume name %s"), name);
+        goto cleanup;
+    }
+
+    name += strlen(VOL_NAME_PREFIX);
+    if (virStrToLong_i(name, NULL, 10, lun) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    return ret;
+}
+
+static int
+virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol,
+                                        struct iscsi_context *iscsi)
+{
+    uint32_t lba = 0;
+    uint32_t block_size;
+    uint32_t nb_block;
+    struct scsi_task *task = NULL;
+    int lun = 0;
+    int ret = -1;
+    unsigned char *data;
+
+    if (virStorageBackendISCSIDirectGetLun(vol, &lun) < 0)
+        return ret;
+    if (virISCSIDirectTestUnitReady(iscsi, lun) < 0)
+        return ret;
+    if (virISCSIDirectGetVolumeCapacity(iscsi, lun, &block_size, &nb_block))
+        return ret;
+    if (VIR_ALLOC_N(data, block_size * BLOCK_PER_PACKET))
+        return ret;
+
+    while (lba < nb_block) {
+        if (nb_block - lba > block_size * BLOCK_PER_PACKET) {
+
+            if (!(task = iscsi_write10_sync(iscsi, lun, lba, data,
+                                            block_size * BLOCK_PER_PACKET,
+                                            block_size, 0, 0, 0, 0, 0)))
+                goto cleanup;
+            scsi_free_scsi_task(task);
+            lba += BLOCK_PER_PACKET;
+        } else {
+            if (!(task = iscsi_write10_sync(iscsi, lun, lba, data, block_size,
+                                        block_size, 0, 0, 0, 0, 0)))
+                goto cleanup;
+            scsi_free_scsi_task(task);
+            lba++;
+        }
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(data);
+    return ret;
+}
+
+static int
+virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr pool,
+                                   virStorageVolDefPtr vol,
+                                   unsigned int algorithm,
+                                   unsigned int flags)
+{
+    struct iscsi_context *iscsi = NULL;
+    char *portal = NULL;
+    int ret = -1;
+
+    virCheckFlags(0, -1);
+
+    if (virStorageBackendISCSIDirectSetConnection(pool, &iscsi, &portal) <0)
+        goto cleanup;
+
+    switch ((virStorageVolWipeAlgorithm) algorithm) {
+    case VIR_STORAGE_VOL_WIPE_ALG_ZERO:
+        if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                                 _("failed to wipe volume %s"),
+                                 vol->name);
+            goto disconnect;
+        }
+            break;
+    case VIR_STORAGE_VOL_WIPE_ALG_TRIM:
+    case VIR_STORAGE_VOL_WIPE_ALG_NNSA:
+    case VIR_STORAGE_VOL_WIPE_ALG_DOD:
+    case VIR_STORAGE_VOL_WIPE_ALG_BSI:
+    case VIR_STORAGE_VOL_WIPE_ALG_GUTMANN:
+    case VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER:
+    case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7:
+    case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33:
+    case VIR_STORAGE_VOL_WIPE_ALG_RANDOM:
+    case VIR_STORAGE_VOL_WIPE_ALG_LAST:
+        virReportError(VIR_ERR_INVALID_ARG, _("unsupported algorithm %d"),
+                       algorithm);
+        goto disconnect;
+    }
+
+    ret = 0;
+ disconnect:
+    virISCSIDirectDisconnect(iscsi);
+ cleanup:
+    VIR_FREE(portal);
+    iscsi_destroy_context(iscsi);
+    return ret;
+}
+
+
 virStorageBackend virStorageBackendISCSIDirect = {
     .type = VIR_STORAGE_POOL_ISCSI_DIRECT,
 
     .checkPool = virStorageBackendISCSIDirectCheckPool,
     .findPoolSources = virStorageBackendISCSIDirectFindPoolSources,
     .refreshPool = virStorageBackendISCSIDirectRefreshPool,
+    .wipeVol = virStorageBackenISCSIDirectWipeVol,
 };
 
 int
-- 
2.18.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: add wipeVol to iscsi-direct storage backend
Posted by Ján Tomko 5 years, 8 months ago
On Fri, Aug 10, 2018 at 03:33:59PM +0200, clem@lse.epita.fr wrote:
>From: Clementine Hayat <clem@lse.epita.fr>
>
>Change set volume capacity to get volume capacity and put the connection
>and helpers in one function to avoid code duplicates.

virStorageBackendISCSIDirectSetConnection addition should be in a
separate commit. And the Set->Get change in another one, leaving just
the wipeVol addition here.

For more on splitting changes into commits, see:
https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/

Jano

>
>Signed-off-by: Clementine Hayat <clem@lse.epita.fr>
>---
>
>Set BLOCK_PER_PACKET to 128. Not sure about this value. Should be
>potentially tuned.
>
> src/storage/storage_backend_iscsi_direct.c | 173 ++++++++++++++++++---
> 1 file changed, 152 insertions(+), 21 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: add wipeVol to iscsi-direct storage backend
Posted by Michal Privoznik 5 years, 8 months ago
On 08/10/2018 03:33 PM, clem@lse.epita.fr wrote:
> From: Clementine Hayat <clem@lse.epita.fr>
> 
> Change set volume capacity to get volume capacity and put the connection
> and helpers in one function to avoid code duplicates.
> 
> Signed-off-by: Clementine Hayat <clem@lse.epita.fr>
> ---
> 
> Set BLOCK_PER_PACKET to 128. Not sure about this value. Should be
> potentially tuned.
> 
>  src/storage/storage_backend_iscsi_direct.c | 173 ++++++++++++++++++---
>  1 file changed, 152 insertions(+), 21 deletions(-)

Apart from Jano's comments ....

> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 0764356b62..958f5937f9 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -41,6 +41,8 @@
>  
>  #define ISCSI_DEFAULT_TARGET_PORT 3260
>  #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
> +#define BLOCK_PER_PACKET 128
> +#define VOL_NAME_PREFIX "unit:0:0:"
>  
>  VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
>  
> @@ -225,7 +227,7 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
>  {
>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>  
> -    if (virAsprintf(&vol->name, "unit:0:0:%u", lun) < 0)
> +    if (virAsprintf(&vol->name, "%s%u", VOL_NAME_PREFIX, lun) < 0)
>          return -1;
>      if (virAsprintf(&vol->key, "ip-%s-iscsi-%s-lun-%u", portal,
>                      def->source.devices[0].path, lun) < 0)
> @@ -237,13 +239,13 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
>  }
>  
>  static int
> -virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
> -                                virStorageVolDefPtr vol,
> -                                int lun)
> +virISCSIDirectGetVolumeCapacity(struct iscsi_context *iscsi,
> +                                int lun,
> +                                uint32_t *block_size,
> +                                uint32_t *nb_block)
>  {
>      struct scsi_task *task = NULL;
>      struct scsi_inquiry_standard *inq = NULL;
> -    long long size = 0;
>      int ret = -1;
>  
>      if (!(task = iscsi_inquiry_sync(iscsi, lun, 0, 0, 64)) ||
> @@ -282,10 +284,8 @@ virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
>              goto cleanup;
>          }
>  
> -        size  = rc10->block_size;
> -        size *= rc10->lba;
> -        vol->target.capacity = size;
> -        vol->target.allocation = size;
> +        *block_size  = rc10->block_size;
> +        *nb_block = rc10->lba;
>  
>      }
>  
> @@ -303,6 +303,8 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
>  {
>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>      virStorageVolDefPtr vol = NULL;
> +    uint32_t block_size;
> +    uint32_t nb_block;
>      int ret = -1;
>  
>      virStoragePoolObjClearVols(pool);
> @@ -314,9 +316,12 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
>  
>      vol->type = VIR_STORAGE_VOL_NETWORK;
>  
> -    if (virISCSIDirectSetVolumeCapacity(iscsi, vol, lun) < 0)
> +    if (virISCSIDirectGetVolumeCapacity(iscsi, lun,
> +                                        &block_size, &nb_block) < 0)
>          goto cleanup;
>  
> +    vol->target.capacity = block_size * nb_block;
> +    vol->target.allocation = block_size * nb_block;
>      def->capacity += vol->target.capacity;
>      def->allocation += vol->target.allocation;
>  
> @@ -553,24 +558,33 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
>      virStoragePoolSourceFree(source);
>      return ret;
>  }
> +static int
> +virStorageBackendISCSIDirectSetConnection(virStoragePoolObjPtr pool,
> +                                          struct iscsi_context **iscsi,
> +                                          char **portal)
> +{
> +    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> +
> +    if (!(*iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn)))
> +        return -1;
> +    if (!(*portal = virStorageBackendISCSIDirectPortal(&def->source)))
> +        return -1;
> +    if (virStorageBackendISCSIDirectSetAuth(*iscsi, &def->source) < 0)
> +        return -1;
> +    if (virISCSIDirectSetContext(*iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
> +        return -1;
> +    if (virISCSIDirectConnect(*iscsi, *portal) < 0)
> +        return -1;
> +    return 0;

This function can return iscsi directly. And can accept @portal to be
NULL (in which case caller says "I don't care about portal").

Apart from that the patch looks good.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list