diff mbox series

[v8,08/21] iscsi: Switch to .bdrv_co_block_status()

Message ID 20180213202701.15858-9-eblake@redhat.com
State New
Headers show
Series add byte-based block_status driver callbacks | expand

Commit Message

Eric Blake Feb. 13, 2018, 8:26 p.m. UTC 
We are gradually moving away from sector-based interfaces, towards
byte-based.  Update the iscsi driver accordingly.  In this case,
it is handy to teach iscsi_co_block_status() to handle a NULL map
and file parameter, even though the block layer passes non-NULL
values, because we also call the function directly.  For now, there
are no optimizations done based on the want_zero flag.

We can also make the simplification of asserting that the block
layer passed in aligned values.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>

---
v8: rebase to master
v7: rebase to master
v6: no change
v5: assert rather than check for alignment
v4: rebase to interface tweaks
v3: no change
v2: rebase to mapping parameter
---
 block/iscsi.c | 67 ++++++++++++++++++++++++++++-------------------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

Comments

Kevin Wolf Feb. 14, 2018, 11:53 a.m. UTC | #1 
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Update the iscsi driver accordingly.  In this case,
> it is handy to teach iscsi_co_block_status() to handle a NULL map
> and file parameter, even though the block layer passes non-NULL
> values, because we also call the function directly.  For now, there
> are no optimizations done based on the want_zero flag.
> 
> We can also make the simplification of asserting that the block
> layer passed in aligned values.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v8: rebase to master
> v7: rebase to master
> v6: no change
> v5: assert rather than check for alignment
> v4: rebase to interface tweaks
> v3: no change
> v2: rebase to mapping parameter
> ---
>  block/iscsi.c | 67 ++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index d2b0466775c..4842519fdad 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -653,36 +653,36 @@ out_unlock:
> 
> 
> 
> -static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
> -                                                  int64_t sector_num,
> -                                                  int nb_sectors, int *pnum,
> -                                                  BlockDriverState **file)
> +static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs,
> +                                              bool want_zero, int64_t offset,
> +                                              int64_t bytes, int64_t *pnum,
> +                                              int64_t *map,
> +                                              BlockDriverState **file)
>  {
>      IscsiLun *iscsilun = bs->opaque;
>      struct scsi_get_lba_status *lbas = NULL;
>      struct scsi_lba_status_descriptor *lbasd = NULL;
>      struct IscsiTask iTask;
>      uint64_t lba;
> -    int64_t ret;
> +    int ret;
> 
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
> 
> -    if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
> -        ret = -EINVAL;
> -        goto out;
> -    }
> +    assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size));
> 
>      /* default to all sectors allocated */
> -    ret = BDRV_BLOCK_DATA;
> -    ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
> -    *pnum = nb_sectors;
> +    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> +    if (map) {
> +        *map = offset;
> +    }

Can map ever be NULL? You didn't have that check in other drivers.

> +    *pnum = bytes;
> 
>      /* LUN does not support logical block provisioning */
>      if (!iscsilun->lbpme) {
>          goto out;
>      }
> 
> -    lba = sector_qemu2lun(sector_num, iscsilun);
> +    lba = offset / iscsilun->block_size;
> 
>      qemu_mutex_lock(&iscsilun->mutex);
>  retry:
> @@ -727,12 +727,12 @@ retry:
> 
>      lbasd = &lbas->descriptors[0];
> 
> -    if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
> +    if (lba != lbasd->lba) {
>          ret = -EIO;
>          goto out_unlock;
>      }
> 
> -    *pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun);
> +    *pnum = lbasd->num_blocks * iscsilun->block_size;
> 
>      if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
>          lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
> @@ -743,15 +743,13 @@ retry:
>      }
> 
>      if (ret & BDRV_BLOCK_ZERO) {
> -        iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> -                                       *pnum * BDRV_SECTOR_SIZE);
> +        iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum);
>      } else {
> -        iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> -                                     *pnum * BDRV_SECTOR_SIZE);
> +        iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);
>      }
> 
> -    if (*pnum > nb_sectors) {
> -        *pnum = nb_sectors;
> +    if (*pnum > bytes) {
> +        *pnum = bytes;
>      }
>  out_unlock:
>      qemu_mutex_unlock(&iscsilun->mutex);
> @@ -760,7 +758,7 @@ out:
>      if (iTask.task != NULL) {
>          scsi_free_scsi_task(iTask.task);
>      }
> -    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
> +    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
>          *file = bs;
>      }

Can file ever be NULL?

>      return ret;
> @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>                                   nb_sectors * BDRV_SECTOR_SIZE) &&

No iscsi_co_preadv() yet... :-(

>          !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
>                                       nb_sectors * BDRV_SECTOR_SIZE)) {
> -        int pnum;
> -        BlockDriverState *file;
> +        int64_t pnum;
>          /* check the block status from the beginning of the cluster
>           * containing the start sector */
> -        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
> -        int head;
> -        int64_t ret;
> +        int64_t head;
> +        int ret;
> 
> -        assert(cluster_sectors);
> -        head = sector_num % cluster_sectors;
> -        ret = iscsi_co_get_block_status(bs, sector_num - head,
> -                                        BDRV_REQUEST_MAX_SECTORS, &pnum,
> -                                        &file);
> +        assert(iscsilun->cluster_size);
> +        head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
> +        ret = iscsi_co_block_status(bs, false,
> +                                    sector_num * BDRV_SECTOR_SIZE - head,
> +                                    BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);

It doesn't make a difference with your current implementation because it
ignores want_zero, but consistent with your approach that
want_zero=false returns just that everyhting is allocated for drivers
without support for backing files, I think you want want_zero=true here.

>          if (ret < 0) {
>              return ret;
>          }
>          /* if the whole request falls into an unallocated area we can avoid
>           * reading and directly return zeroes instead */
> -        if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
> +        if (ret & BDRV_BLOCK_ZERO &&
> +            pnum >= nb_sectors * BDRV_SECTOR_SIZE + head) {
>              qemu_iovec_memset(iov, 0, 0x00, iov->size);
>              return 0;
>          }

Kevin
Eric Blake Feb. 14, 2018, 2:33 p.m. UTC | #2 
On 02/14/2018 05:53 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based.  Update the iscsi driver accordingly.  In this case,
>> it is handy to teach iscsi_co_block_status() to handle a NULL map
>> and file parameter, even though the block layer passes non-NULL
>> values, because we also call the function directly.  For now, there
>> are no optimizations done based on the want_zero flag.

[1]

>>
>> We can also make the simplification of asserting that the block
>> layer passed in aligned values.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>

>>       /* default to all sectors allocated */
>> -    ret = BDRV_BLOCK_DATA;
>> -    ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
>> -    *pnum = nb_sectors;
>> +    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>> +    if (map) {
>> +        *map = offset;
>> +    }
> 
> Can map ever be NULL? You didn't have that check in other drivers.

The documentation in block_int.h states that io.c never passes NULL for 
map.  However, see my commit message [1], and the code below [2], for 
why THIS driver has to check for NULL.


>> @@ -760,7 +758,7 @@ out:
>>       if (iTask.task != NULL) {
>>           scsi_free_scsi_task(iTask.task);
>>       }
>> -    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
>> +    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
>>           *file = bs;
>>       }
> 
> Can file ever be NULL?

Ditto.

> 
>>       return ret;
>> @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>>                                    nb_sectors * BDRV_SECTOR_SIZE) &&
> 
> No iscsi_co_preadv() yet... :-(

Yeah, but that's for another day.

> 
>>           !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
>>                                        nb_sectors * BDRV_SECTOR_SIZE)) {
>> -        int pnum;
>> -        BlockDriverState *file;
>> +        int64_t pnum;
>>           /* check the block status from the beginning of the cluster
>>            * containing the start sector */
>> -        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
>> -        int head;
>> -        int64_t ret;
>> +        int64_t head;
>> +        int ret;
>>
>> -        assert(cluster_sectors);
>> -        head = sector_num % cluster_sectors;
>> -        ret = iscsi_co_get_block_status(bs, sector_num - head,
>> -                                        BDRV_REQUEST_MAX_SECTORS, &pnum,
>> -                                        &file);
>> +        assert(iscsilun->cluster_size);
>> +        head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
>> +        ret = iscsi_co_block_status(bs, false,
>> +                                    sector_num * BDRV_SECTOR_SIZE - head,
>> +                                    BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
> 

[2] This is the reason that THIS driver has to check for NULL, even 
though the block layer never passes NULL.

> It doesn't make a difference with your current implementation because it
> ignores want_zero, but consistent with your approach that
> want_zero=false returns just that everyhting is allocated for drivers
> without support for backing files, I think you want want_zero=true here.

Makes sense.  If that's the only tweak, can you make it while taking the 
series, or will I need to respin?
diff mbox series

Patch

diff --git a/block/iscsi.c b/block/iscsi.c
index d2b0466775c..4842519fdad 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -653,36 +653,36 @@  out_unlock:



-static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
-                                                  int64_t sector_num,
-                                                  int nb_sectors, int *pnum,
-                                                  BlockDriverState **file)
+static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs,
+                                              bool want_zero, int64_t offset,
+                                              int64_t bytes, int64_t *pnum,
+                                              int64_t *map,
+                                              BlockDriverState **file)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct scsi_get_lba_status *lbas = NULL;
     struct scsi_lba_status_descriptor *lbasd = NULL;
     struct IscsiTask iTask;
     uint64_t lba;
-    int64_t ret;
+    int ret;

     iscsi_co_init_iscsitask(iscsilun, &iTask);

-    if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
-        ret = -EINVAL;
-        goto out;
-    }
+    assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size));

     /* default to all sectors allocated */
-    ret = BDRV_BLOCK_DATA;
-    ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
-    *pnum = nb_sectors;
+    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+    if (map) {
+        *map = offset;
+    }
+    *pnum = bytes;

     /* LUN does not support logical block provisioning */
     if (!iscsilun->lbpme) {
         goto out;
     }

-    lba = sector_qemu2lun(sector_num, iscsilun);
+    lba = offset / iscsilun->block_size;

     qemu_mutex_lock(&iscsilun->mutex);
 retry:
@@ -727,12 +727,12 @@  retry:

     lbasd = &lbas->descriptors[0];

-    if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
+    if (lba != lbasd->lba) {
         ret = -EIO;
         goto out_unlock;
     }

-    *pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun);
+    *pnum = lbasd->num_blocks * iscsilun->block_size;

     if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
         lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
@@ -743,15 +743,13 @@  retry:
     }

     if (ret & BDRV_BLOCK_ZERO) {
-        iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
-                                       *pnum * BDRV_SECTOR_SIZE);
+        iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum);
     } else {
-        iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
-                                     *pnum * BDRV_SECTOR_SIZE);
+        iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);
     }

-    if (*pnum > nb_sectors) {
-        *pnum = nb_sectors;
+    if (*pnum > bytes) {
+        *pnum = bytes;
     }
 out_unlock:
     qemu_mutex_unlock(&iscsilun->mutex);
@@ -760,7 +758,7 @@  out:
     if (iTask.task != NULL) {
         scsi_free_scsi_task(iTask.task);
     }
-    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
         *file = bs;
     }
     return ret;
@@ -800,25 +798,24 @@  static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
                                  nb_sectors * BDRV_SECTOR_SIZE) &&
         !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
                                      nb_sectors * BDRV_SECTOR_SIZE)) {
-        int pnum;
-        BlockDriverState *file;
+        int64_t pnum;
         /* check the block status from the beginning of the cluster
          * containing the start sector */
-        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
-        int head;
-        int64_t ret;
+        int64_t head;
+        int ret;

-        assert(cluster_sectors);
-        head = sector_num % cluster_sectors;
-        ret = iscsi_co_get_block_status(bs, sector_num - head,
-                                        BDRV_REQUEST_MAX_SECTORS, &pnum,
-                                        &file);
+        assert(iscsilun->cluster_size);
+        head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
+        ret = iscsi_co_block_status(bs, false,
+                                    sector_num * BDRV_SECTOR_SIZE - head,
+                                    BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
         if (ret < 0) {
             return ret;
         }
         /* if the whole request falls into an unallocated area we can avoid
          * reading and directly return zeroes instead */
-        if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
+        if (ret & BDRV_BLOCK_ZERO &&
+            pnum >= nb_sectors * BDRV_SECTOR_SIZE + head) {
             qemu_iovec_memset(iov, 0, 0x00, iov->size);
             return 0;
         }
@@ -2218,7 +2215,7 @@  static BlockDriver bdrv_iscsi = {
     .bdrv_truncate   = iscsi_truncate,
     .bdrv_refresh_limits = iscsi_refresh_limits,

-    .bdrv_co_get_block_status = iscsi_co_get_block_status,
+    .bdrv_co_block_status  = iscsi_co_block_status,
     .bdrv_co_pdiscard      = iscsi_co_pdiscard,
     .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
     .bdrv_co_readv         = iscsi_co_readv,
@@ -2253,7 +2250,7 @@  static BlockDriver bdrv_iser = {
     .bdrv_truncate   = iscsi_truncate,
     .bdrv_refresh_limits = iscsi_refresh_limits,

-    .bdrv_co_get_block_status = iscsi_co_get_block_status,
+    .bdrv_co_block_status  = iscsi_co_block_status,
     .bdrv_co_pdiscard      = iscsi_co_pdiscard,
     .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
     .bdrv_co_readv         = iscsi_co_readv,