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

[Vladimir Sementsov-Ogievskiy]

23.09.2021 23:33, Eric Blake wrote:
> 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 at 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.

I agree, <= should be right thing, thanks!

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


-- 
Best regards,
Vladimir