Re: [Qemu-block] [PATCH v4 05/20] iscsi: Switch cluster_sectors to byte-based

[Paolo Bonzini]

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 <address@hidden>
> 
> ---
> 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 <address@hidden>