[Qemu-devel] [PATCH for-2.0] iscsi: always query max WRITE SAME length

[Qemu-devel] [PATCH for-2.0] iscsi: always query max WRITE SAME length

[Paolo Bonzini]

Max WRITE SAME length is also used when the UNMAP bit is zero, so it
should be queried even if LBPWS=0.  Same for the optimal transfer
length.

However, the write_zeroes_alignment only matters for UNMAP=1 so we
still restrict it to LBPWS=1.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 00316af..bb88b4b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1335,18 +1335,20 @@ static int iscsi_refresh_limits(BlockDriverState *bs)
 
     /* We don't actually refresh here, but just return data queried in
      * iscsi_open(): iscsi targets don't change their limits. */
-    if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
+    if (iscsilun->lbp.lbpu) {
         if (iscsilun->bl.max_unmap < 0xffffffff) {
             bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
                                                  iscsilun);
         }
         bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
                                                    iscsilun);
+    }
 
-        if (iscsilun->bl.max_ws_len < 0xffffffff) {
-            bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
-                                                      iscsilun);
-        }
+    if (iscsilun->bl.max_ws_len < 0xffffffff) {
+        bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
+                                                  iscsilun);
+    }
+    if (iscsilun->lbp.lbpws) {
         bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
                                                         iscsilun);
     }
-- 
1.9.0

Re: [Qemu-devel] [PATCH for-2.0] iscsi: always query max WRITE SAME length

[Peter Lieven]

Am 02.04.2014 15:44, schrieb Paolo Bonzini:
> Max WRITE SAME length is also used when the UNMAP bit is zero, so it
> should be queried even if LBPWS=0.  Same for the optimal transfer
> length.
>
> However, the write_zeroes_alignment only matters for UNMAP=1 so we
> still restrict it to LBPWS=1.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/iscsi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 00316af..bb88b4b 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1335,18 +1335,20 @@ static int iscsi_refresh_limits(BlockDriverState *bs)
>  
>      /* We don't actually refresh here, but just return data queried in
>       * iscsi_open(): iscsi targets don't change their limits. */
> -    if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
> +    if (iscsilun->lbp.lbpu) {
>          if (iscsilun->bl.max_unmap < 0xffffffff) {
>              bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
>                                                   iscsilun);
>          }
>          bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>                                                     iscsilun);
> +    }
>  
> -        if (iscsilun->bl.max_ws_len < 0xffffffff) {
> -            bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
> -                                                      iscsilun);
> -        }
> +    if (iscsilun->bl.max_ws_len < 0xffffffff) {
> +        bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
> +                                                  iscsilun);
> +    }
> +    if (iscsilun->lbp.lbpws) {
>          bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>                                                          iscsilun);
>      }

Reviewed-by: Peter Lieven <pl@kamp.de>



PS: One thing that came into my mind while looking at this. If a target has LBPWS == 0 and we
send a write zeroes request with BDRV_REQ_MAY_UNMAP we fail with -ENOTSUP. Shouldn't
we fall back to WRITESAME without UNMAP if iscsilun->has_write_same == true?

Peter