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.