Re: [Qemu-devel] [PATCH 05/13] iscsi: Convert to bdrv_co_pwrite_zeroes()

[Kevin Wolf <kwolf@redhat.com>]

Am 25.05.2016 um 00:25 hat Eric Blake geschrieben:
> Another step on our continuing quest to switch to byte-based
> interfaces.
> 
> As this is the first byte-based iscsi interface, convert
> is_request_lun_aligned() into two versions, one for sectors
> and one for bytes.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/iscsi.c | 53 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 0acc3dc..3dbfd57 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -401,18 +401,25 @@ static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
>      return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
>  }
> 
> -static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors,
> -                                      IscsiLun *iscsilun)
> +static bool is_byte_request_lun_aligned(int64_t offset, int count,
> +                                        IscsiLun *iscsilun)
>  {
> -    if ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
> -        (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) {
> -            error_report("iSCSI misaligned request: "
> -                         "iscsilun->block_size %u, sector_num %" PRIi64
> -                         ", nb_sectors %d",
> -                         iscsilun->block_size, sector_num, nb_sectors);
> -            return 0;
> +    if (offset % iscsilun->block_size || count % iscsilun->block_size) {
> +        error_report("iSCSI misaligned request: "
> +                     "iscsilun->block_size %u, offset %" PRIi64
> +                     ", count %d",
> +                     iscsilun->block_size, offset, count);
> +        return false;
>      }
> -    return 1;
> +    return true;
> +}
> +
> +static bool is_sector_request_lun_aligned(int64_t sector_num, int nb_sectors,
> +                                          IscsiLun *iscsilun)
> +{
> +    return is_byte_request_lun_aligned(sector_num << BDRV_SECTOR_BITS,
> +                                       nb_sectors << BDRV_SECTOR_BITS,
> +                                       iscsilun);
>  }

You're switching from (nb_sectors * BDRV_SECTOR_SIZE) to (nb_sectors <<
BDRV_SECTOR_BITS). The difference is that the former is a 64 bit
calculation because BDRV_SECTOR_BITS is unsigned long long, whereas the
latter is a 32 bit calculation.

Fortunately, it seems to me that all input values come directly from the
block layer which already limits requests to BDRV_REQUEST_MAX_SECTORS.
So we should be safe from overflows here.

>  static int
> -coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> -                                   int nb_sectors, BdrvRequestFlags flags)
> +coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
> +                                    int count, BdrvRequestFlags flags)
>  {
>      IscsiLun *iscsilun = bs->opaque;
>      struct IscsiTask iTask;
> @@ -978,7 +985,7 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>      uint32_t nb_blocks;
>      bool use_16_for_ws = iscsilun->use_16_for_rw;
> 
> -    if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
> +    if (!is_byte_request_lun_aligned(offset, count, iscsilun)) {
>          return -EINVAL;
>      }

Should this become -ENOTSUP so that emulation can take over rather than
failing the request?

We should probably also always set bs->bl.pwrite_zeroes_alignment, with
a fallback to iscsilun->block_size if we don't have iscsilun->lbp.lbpws.
But that's a separate patch.

Kevin