So far we have two branches: either we zero BLOCK_PER_PACKET
(currently 128) block at one, or if we're close to the last block
then we zero out one block at the time. This is very suboptimal.
We know how many block are there left. Might as well just write
them all at once.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/storage/storage_backend_iscsi_direct.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
index 786663534d..574a65449d 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -645,21 +645,15 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol,
         return ret;
 
     while (lba < nb_block) {
-        if (nb_block - lba > block_size * BLOCK_PER_PACKET) {
+        const uint64_t to_write = MIN(nb_block - lba + 1, BLOCK_PER_PACKET);
 
-            if (!(task = iscsi_write16_sync(iscsi, lun, lba, data,
-                                            block_size * BLOCK_PER_PACKET,
-                                            block_size, 0, 0, 0, 0, 0)))
-                return -1;
-            scsi_free_scsi_task(task);
-            lba += BLOCK_PER_PACKET;
-        } else {
-            if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, block_size,
+        if (!(task = iscsi_write16_sync(iscsi, lun, lba, data,
+                                        to_write * block_size,
                                         block_size, 0, 0, 0, 0, 0)))
-                return -1;
-            scsi_free_scsi_task(task);
-            lba++;
-        }
+            return -1;
+        scsi_free_scsi_task(task);
+
+        lba += to_write;
     }
 
     return 0;
-- 
2.19.2

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

On 3/1/19 11:42 AM, Michal Privoznik wrote:
> So far we have two branches: either we zero BLOCK_PER_PACKET
> (currently 128) block at one, or if we're close to the last block
> then we zero out one block at the time. This is very suboptimal.
> We know how many block are there left. Might as well just write
> them all at once.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/storage/storage_backend_iscsi_direct.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 786663534d..574a65449d 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -645,21 +645,15 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol,
>           return ret;
>   
>       while (lba < nb_block) {
> -        if (nb_block - lba > block_size * BLOCK_PER_PACKET) {
> +        const uint64_t to_write = MIN(nb_block - lba + 1, BLOCK_PER_PACKET);
>   
> -            if (!(task = iscsi_write16_sync(iscsi, lun, lba, data,
> -                                            block_size * BLOCK_PER_PACKET,
> -                                            block_size, 0, 0, 0, 0, 0)))
> -                return -1;
> -            scsi_free_scsi_task(task);
> -            lba += BLOCK_PER_PACKET;
> -        } else {
> -            if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, block_size,
> +        if (!(task = iscsi_write16_sync(iscsi, lun, lba, data,
> +                                        to_write * block_size,
>                                           block_size, 0, 0, 0, 0, 0)))
> -                return -1;

Self-NACK. I've experimented a bit and realized we can use 
writesame16_sync() which allows us to allocate smaller buffer and also 
it is a bit faster. I'll post v2 for this one.

Michal

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