[v4,05/20] iscsi: Switch cluster_sectors to byte-based

Message ID 20171012185916.22776-6-eblake@redhat.com
State New
Headers show
Series
  • add byte-based block_status driver callbacks
Related show

Commit Message

Eric Blake Oct. 12, 2017, 6:59 p.m.
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based.  Convert all uses of
the cluster size in sectors, along with adding assertions that we
are not dividing by zero.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: no change
---
 block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

Comments

Paolo Bonzini Oct. 18, 2017, 9:20 a.m. | #1
On 12/10/2017 20:59, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert all uses of
> the cluster size in sectors, along with adding assertions that we
> are not dividing by zero.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: no change
> ---
>  block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 4683f3b244..8f903d8370 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -84,7 +84,7 @@ typedef struct IscsiLun {
>      unsigned long *allocmap;
>      unsigned long *allocmap_valid;
>      long allocmap_size;
> -    int cluster_sectors;
> +    int cluster_size;
>      bool use_16_for_rw;
>      bool write_protected;
>      bool lbpme;
> @@ -427,9 +427,10 @@ static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
>  {
>      iscsi_allocmap_free(iscsilun);
> 
> +    assert(iscsilun->cluster_size);
>      iscsilun->allocmap_size =
> -        DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
> -                     iscsilun->cluster_sectors);
> +        DIV_ROUND_UP(iscsilun->num_blocks * iscsilun->block_size,
> +                     iscsilun->cluster_size);
> 
>      iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
>      if (!iscsilun->allocmap) {
> @@ -437,7 +438,7 @@ static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
>      }
> 
>      if (open_flags & BDRV_O_NOCACHE) {
> -        /* in case that cache.direct = on all allocmap entries are
> +        /* when cache.direct = on all allocmap entries are
>           * treated as invalid to force a relookup of the block
>           * status on every read request */
>          return 0;
> @@ -458,17 +459,19 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
>                        int nb_sectors, bool allocated, bool valid)
>  {
>      int64_t cl_num_expanded, nb_cls_expanded, cl_num_shrunk, nb_cls_shrunk;
> +    int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
> 
>      if (iscsilun->allocmap == NULL) {
>          return;
>      }
>      /* expand to entirely contain all affected clusters */
> -    cl_num_expanded = sector_num / iscsilun->cluster_sectors;
> +    assert(cluster_sectors);
> +    cl_num_expanded = sector_num / cluster_sectors;
>      nb_cls_expanded = DIV_ROUND_UP(sector_num + nb_sectors,
> -                                   iscsilun->cluster_sectors) - cl_num_expanded;
> +                                   cluster_sectors) - cl_num_expanded;
>      /* shrink to touch only completely contained clusters */
> -    cl_num_shrunk = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
> -    nb_cls_shrunk = (sector_num + nb_sectors) / iscsilun->cluster_sectors
> +    cl_num_shrunk = DIV_ROUND_UP(sector_num, cluster_sectors);
> +    nb_cls_shrunk = (sector_num + nb_sectors) / cluster_sectors
>                        - cl_num_shrunk;
>      if (allocated) {
>          bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
> @@ -532,9 +535,12 @@ iscsi_allocmap_is_allocated(IscsiLun *iscsilun, int64_t sector_num,
>      if (iscsilun->allocmap == NULL) {
>          return true;
>      }
> -    size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
> +    assert(iscsilun->cluster_size);
> +    size = DIV_ROUND_UP(sector_num + nb_sectors,
> +                        iscsilun->cluster_size >> BDRV_SECTOR_BITS);
>      return !(find_next_bit(iscsilun->allocmap, size,
> -                           sector_num / iscsilun->cluster_sectors) == size);
> +                           sector_num * BDRV_SECTOR_SIZE /
> +                           iscsilun->cluster_size) == size);
>  }
> 
>  static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
> @@ -544,9 +550,12 @@ static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
>      if (iscsilun->allocmap_valid == NULL) {
>          return false;
>      }
> -    size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
> +    assert(iscsilun->cluster_size);
> +    size = DIV_ROUND_UP(sector_num + nb_sectors,
> +                        iscsilun->cluster_size >> BDRV_SECTOR_BITS);
>      return (find_next_zero_bit(iscsilun->allocmap_valid, size,
> -                               sector_num / iscsilun->cluster_sectors) == size);
> +                               sector_num * BDRV_SECTOR_SIZE /
> +                               iscsilun->cluster_size) == size);
>  }
> 
>  static int coroutine_fn
> @@ -781,16 +790,21 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>          BlockDriverState *file;
>          /* check the block status from the beginning of the cluster
>           * containing the start sector */
> -        int64_t ret = iscsi_co_get_block_status(bs,
> -                          sector_num - sector_num % iscsilun->cluster_sectors,
> -                          BDRV_REQUEST_MAX_SECTORS, &pnum, &file);
> +        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
> +        int head;
> +        int64_t 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);
>          if (ret < 0) {
>              return ret;
>          }
>          /* if the whole request falls into an unallocated area we can avoid
> -         * to read and directly return zeroes instead */
> -        if (ret & BDRV_BLOCK_ZERO &&
> -            pnum >= nb_sectors + sector_num % iscsilun->cluster_sectors) {
> +         * reading and directly return zeroes instead */
> +        if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
>              qemu_iovec_memset(iov, 0, 0x00, iov->size);
>              return 0;
>          }
> @@ -1930,8 +1944,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>       * reasonable size */
>      if (iscsilun->bl.opt_unmap_gran * iscsilun->block_size >= 4 * 1024 &&
>          iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
> -        iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
> -                                     iscsilun->block_size) >> BDRV_SECTOR_BITS;
> +        iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
> +            iscsilun->block_size;
>          if (iscsilun->lbprz) {
>              ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>          }
> @@ -2137,7 +2151,7 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>      IscsiLun *iscsilun = bs->opaque;
>      bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
>      bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
> -    bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
> +    bdi->cluster_size = iscsilun->cluster_size;
>      return 0;
>  }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Patch

diff --git a/block/iscsi.c b/block/iscsi.c
index 4683f3b244..8f903d8370 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -84,7 +84,7 @@  typedef struct IscsiLun {
     unsigned long *allocmap;
     unsigned long *allocmap_valid;
     long allocmap_size;
-    int cluster_sectors;
+    int cluster_size;
     bool use_16_for_rw;
     bool write_protected;
     bool lbpme;
@@ -427,9 +427,10 @@  static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
 {
     iscsi_allocmap_free(iscsilun);

+    assert(iscsilun->cluster_size);
     iscsilun->allocmap_size =
-        DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
-                     iscsilun->cluster_sectors);
+        DIV_ROUND_UP(iscsilun->num_blocks * iscsilun->block_size,
+                     iscsilun->cluster_size);

     iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
     if (!iscsilun->allocmap) {
@@ -437,7 +438,7 @@  static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
     }

     if (open_flags & BDRV_O_NOCACHE) {
-        /* in case that cache.direct = on all allocmap entries are
+        /* when cache.direct = on all allocmap entries are
          * treated as invalid to force a relookup of the block
          * status on every read request */
         return 0;
@@ -458,17 +459,19 @@  iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
                       int nb_sectors, bool allocated, bool valid)
 {
     int64_t cl_num_expanded, nb_cls_expanded, cl_num_shrunk, nb_cls_shrunk;
+    int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;

     if (iscsilun->allocmap == NULL) {
         return;
     }
     /* expand to entirely contain all affected clusters */
-    cl_num_expanded = sector_num / iscsilun->cluster_sectors;
+    assert(cluster_sectors);
+    cl_num_expanded = sector_num / cluster_sectors;
     nb_cls_expanded = DIV_ROUND_UP(sector_num + nb_sectors,
-                                   iscsilun->cluster_sectors) - cl_num_expanded;
+                                   cluster_sectors) - cl_num_expanded;
     /* shrink to touch only completely contained clusters */
-    cl_num_shrunk = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
-    nb_cls_shrunk = (sector_num + nb_sectors) / iscsilun->cluster_sectors
+    cl_num_shrunk = DIV_ROUND_UP(sector_num, cluster_sectors);
+    nb_cls_shrunk = (sector_num + nb_sectors) / cluster_sectors
                       - cl_num_shrunk;
     if (allocated) {
         bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
@@ -532,9 +535,12 @@  iscsi_allocmap_is_allocated(IscsiLun *iscsilun, int64_t sector_num,
     if (iscsilun->allocmap == NULL) {
         return true;
     }
-    size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
+    assert(iscsilun->cluster_size);
+    size = DIV_ROUND_UP(sector_num + nb_sectors,
+                        iscsilun->cluster_size >> BDRV_SECTOR_BITS);
     return !(find_next_bit(iscsilun->allocmap, size,
-                           sector_num / iscsilun->cluster_sectors) == size);
+                           sector_num * BDRV_SECTOR_SIZE /
+                           iscsilun->cluster_size) == size);
 }

 static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
@@ -544,9 +550,12 @@  static inline bool iscsi_allocmap_is_valid(IscsiLun *iscsilun,
     if (iscsilun->allocmap_valid == NULL) {
         return false;
     }
-    size = DIV_ROUND_UP(sector_num + nb_sectors, iscsilun->cluster_sectors);
+    assert(iscsilun->cluster_size);
+    size = DIV_ROUND_UP(sector_num + nb_sectors,
+                        iscsilun->cluster_size >> BDRV_SECTOR_BITS);
     return (find_next_zero_bit(iscsilun->allocmap_valid, size,
-                               sector_num / iscsilun->cluster_sectors) == size);
+                               sector_num * BDRV_SECTOR_SIZE /
+                               iscsilun->cluster_size) == size);
 }

 static int coroutine_fn
@@ -781,16 +790,21 @@  static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
         BlockDriverState *file;
         /* check the block status from the beginning of the cluster
          * containing the start sector */
-        int64_t ret = iscsi_co_get_block_status(bs,
-                          sector_num - sector_num % iscsilun->cluster_sectors,
-                          BDRV_REQUEST_MAX_SECTORS, &pnum, &file);
+        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
+        int head;
+        int64_t 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);
         if (ret < 0) {
             return ret;
         }
         /* if the whole request falls into an unallocated area we can avoid
-         * to read and directly return zeroes instead */
-        if (ret & BDRV_BLOCK_ZERO &&
-            pnum >= nb_sectors + sector_num % iscsilun->cluster_sectors) {
+         * reading and directly return zeroes instead */
+        if (ret & BDRV_BLOCK_ZERO && pnum >= nb_sectors + head) {
             qemu_iovec_memset(iov, 0, 0x00, iov->size);
             return 0;
         }
@@ -1930,8 +1944,8 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
      * reasonable size */
     if (iscsilun->bl.opt_unmap_gran * iscsilun->block_size >= 4 * 1024 &&
         iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
-        iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
-                                     iscsilun->block_size) >> BDRV_SECTOR_BITS;
+        iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
+            iscsilun->block_size;
         if (iscsilun->lbprz) {
             ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
         }
@@ -2137,7 +2151,7 @@  static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     IscsiLun *iscsilun = bs->opaque;
     bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
     bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws;
-    bdi->cluster_size = iscsilun->cluster_sectors * BDRV_SECTOR_SIZE;
+    bdi->cluster_size = iscsilun->cluster_size;
     return 0;
 }