Re: [PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers

[Eric Blake <eblake@redhat.com>]

On Fri, Sep 03, 2021 at 01:28:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, convert driver write_zeroes handlers bytes parameter to int64_t.
> 
> The only caller of all updated function is bdrv_co_do_pwrite_zeroes().
> 
> bdrv_co_do_pwrite_zeroes() itself is of course OK with widening of
> callee parameter type. Also, bdrv_co_do_pwrite_zeroes()'s
> max_write_zeroes is limited to INT_MAX. So, updated functions all are
> safe, they will not get "bytes" larger than before.
> 
> Still, let's look through all updated functions, and add assertions to
> the ones which are actually unprepared to values larger than INT_MAX.
> For these drivers also set explicit max_pwrite_zeroes limit.
> 
[snip]
> 
> At this point all block drivers are prepared to support 64bit
> write-zero requests, or have explicitly set max_pwrite_zeroes.

The long commit message is essential, but the analysis looks sane.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

> +++ b/block/iscsi.c

> @@ -1250,11 +1250,21 @@ coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
>      if (use_16_for_ws) {
> +        /*
> +         * iscsi_writesame16_task num_blocks argument is uint32_t. We rely here
> +         * on our max_pwrite_zeroes limit.
> +         */
> +        assert(nb_blocks < UINT32_MAX);
>          iTask.task = iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                              iscsilun->zeroblock, iscsilun->block_size,
>                                              nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),
>                                              0, 0, iscsi_co_generic_cb, &iTask);

Should this be <= instead of < ?

>      } else {
> +        /*
> +         * iscsi_writesame10_task num_blocks argument is uint16_t. We rely here
> +         * on our max_pwrite_zeroes limit.
> +         */
> +        assert(nb_blocks < UINT16_MAX);
>          iTask.task = iscsi_writesame10_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                              iscsilun->zeroblock, iscsilun->block_size,
>                                              nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),

here too.  The 16-bit limit is where we're most likely to run into
someone actually trying to zeroize that much at once.

> +++ b/block/nbd.c
> @@ -1407,15 +1407,17 @@ static int nbd_client_co_pwritev(BlockDriverState *bs, int64_t offset,
>  }
>  
>  static int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
> -                                       int bytes, BdrvRequestFlags flags)
> +                                       int64_t bytes, BdrvRequestFlags flags)
>  {
>      BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>      NBDRequest request = {
>          .type = NBD_CMD_WRITE_ZEROES,
>          .from = offset,
> -        .len = bytes,
> +        .len = bytes,  /* .len is uint32_t actually */
>      };
>  
> +    assert(bytes < UINT32_MAX); /* relay on max_pwrite_zeroes */

And again.  Here, you happen to get by with < because we clamped
bl.max_pwrite_zeroes at BDRV_REQUEST_MAX_BYTES, which is INT_MAX
rounded down.  But I had to check; whereas using <= would be less
worrisome, even if we never get a request that large.

If you agree with my analysis, I can make that change while preparing
my pull request.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org