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 <vsement...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
