[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 07/11] block: use int64_t instead of int in driver write_z
From: |
Eric Blake |
Subject: |
Re: [PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers |
Date: |
Thu, 23 Sep 2021 15:33:45 -0500 |
User-agent: |
NeoMutt/20210205-773-8890a5 |
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
- [PATCH v6 01/11] block/io: bring request check to bdrv_co_(read, write)v_vmstate, (continued)
- [PATCH v6 01/11] block/io: bring request check to bdrv_co_(read, write)v_vmstate, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 02/11] qcow2: check request on vmstate save/load path, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 03/11] block: use int64_t instead of uint64_t in driver read handlers, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 05/11] block: use int64_t instead of uint64_t in copy_range driver handlers, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 06/11] block: make BlockLimits::max_pwrite_zeroes 64bit, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 04/11] block: use int64_t instead of uint64_t in driver write handlers, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers, Vladimir Sementsov-Ogievskiy, 2021/09/03
- Re: [PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers,
Eric Blake <=
- [PATCH v6 08/11] block/io: allow 64bit write-zeroes requests, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 09/11] block: make BlockLimits::max_pdiscard 64bit, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 10/11] block: use int64_t instead of int in driver discard handlers, Vladimir Sementsov-Ogievskiy, 2021/09/03
- [PATCH v6 11/11] block/io: allow 64bit discard requests, Vladimir Sementsov-Ogievskiy, 2021/09/03
- Re: [PATCH v6 00/11] 64bit block-layer: part II, Vladimir Sementsov-Ogievskiy, 2021/09/22