[GEDI] [PATCH v5 10/11] block: use int64_t instead of int in driver discard handlers

Vladimir Sementsov-Ogievskiy vsementsov at virtuozzo.com
Tue Jun 8 08:38:26 UTC 2021


07.06.2021 21:13, Eric Blake wrote:
> On Wed, May 05, 2021 at 10:50:00AM +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 discard handlers bytes parameter to int64_t.
>>
>> The only caller of all updated function is bdrv_co_pdiscard in
>> block/io.c. It is already prepared to work with 64bit requests, but
>> pass at most max(bs->bl.max_pdiscard, INT_MAX) to the driver.
>>
>> Let's look at all updated functions:
>>
>> backup-top: pass to bdrv_co_pdiscard which is 64bit
> 
> and to backup_top_cbw, but that is also 64-bit
> 
>>
>> blkdebug: all calculations are still OK, thanks to
>>    bdrv_check_qiov_request().
>>    both rule_check and bdrv_co_pdiscard are 64bit
>>
>> blklogwrites: pass to blk_loc_writes_co_log which is 64bit
>>
>> blkreply, copy-on-read, filter-compress: pass to bdrv_co_pdiscard, OK
> 
> blkreplay
> 
>>
>> file-posix: one handler calls raw_account_discard() is 64bit and both
>>    handlers calls raw_do_pdiscard(). Update raw_do_pdiscard, which pass
>>    to RawPosixAIOData::aio_nbytes, which is 64bit (and calls
>>    raw_account_discard())
>>
>> gluster: somehow, third argument of glfs_discard_async is size_t.
>>    Let's set max_pdiscard accordingly.
>>
>> iscsi: iscsi_allocmap_set_invalid is 64bit,
>>    !is_byte_request_lun_aligned is 64bit.
>>    list.num is uint32_t. Let's clarify max_pdiscard and
>>    pdiscard_alignment.
> 
> The patch tweaks max_pdiscard, but doesn't change pdiscard_alignment.
> 
>>
>> mirror_top, preallocate: pass to bdrv_mirror_top_do_write() which is
>>    64bit
> 
> file is mirror.c, not mirror-top.c.  But it matches the BlockDriver
> bdrv_mirror_top name.  preallocate does not call
> bdrv_mirror_top_do_write, so it's probably worth separating that line
> out.
> 
>>
>> nbd: protocol limitation. max_pdiscard is alredy set strict enough,
>>    keep it as is for now.
>>
>> nvmd: buf.nlb is uint32_t and we do shift. So, add corresponding limits
>>    to nvme_refresh_limits().
> 
> nvme
> 
>>
>> qcow2: calculations are still OK, thanks to bdrv_check_qiov_request(),
>>    qcow2_cluster_discard() is 64bit.
>>
>> raw-format: raw_adjust_offset() is 64bit, bdrv_co_pdiscard too.
>>
>> sheepdog: the format is deprecated. Don't care and just make old
>>    INT_MAX limit to be explicit
>>
>> throttle: pass to bdrv_co_pdiscard() which is 64bit and to
>>    throttle_group_co_io_limits_intercept() which is 64bit as well.
>>
>> test-block-iothread: bytes argument is unused
>>
>> Great! Now all drivers are prepared to 64bit discard requests or has
>> explicit max_pdiscard limit.
> 
> are prepared to handle 64-bit discard requests, or else have explicit
> max_pdiscard limits.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov at virtuozzo.com>
>> ---
>>   include/block/block_int.h        |  2 +-
>>   block/backup-top.c               |  2 +-
>>   block/blkdebug.c                 |  2 +-
>>   block/blklogwrites.c             |  4 ++--
>>   block/blkreplay.c                |  2 +-
>>   block/copy-on-read.c             |  2 +-
>>   block/file-posix.c               |  7 ++++---
>>   block/filter-compress.c          |  2 +-
>>   block/gluster.c                  |  7 +++++--
>>   block/iscsi.c                    | 10 +++++-----
>>   block/mirror.c                   |  2 +-
>>   block/nbd.c                      |  6 ++++--
>>   block/nvme.c                     | 14 +++++++++++++-
>>   block/preallocate.c              |  2 +-
>>   block/qcow2.c                    |  2 +-
>>   block/raw-format.c               |  2 +-
>>   block/sheepdog.c                 | 15 ++++++++++++++-
>>   block/throttle.c                 |  2 +-
>>   tests/unit/test-block-iothread.c |  2 +-
>>   block/trace-events               |  4 ++--
>>   20 files changed, 61 insertions(+), 30 deletions(-)
>>
> 
>> +++ b/block/gluster.c
>> @@ -891,6 +891,7 @@ out:
>>   static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
>>   {
>>       bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
>> +    bs->bl.max_pdiscard = SIZE_MAX;
> 
> We probably want this to be MIN(GLUSTER_MAX_TRANSFER, SIZE_MAX). Also,
> do we want to round it down to alignment boundaries?

I don't think so.. We just call glfs_discard_async() function which is not part of Qemu. So we shouldn't assume any extra restrictions except for argument types I think. byte is size_t, so maximum is SIZE_MAX.

> 
>> +++ b/block/iscsi.c
>> @@ -1141,7 +1141,8 @@ iscsi_getlength(BlockDriverState *bs)
>>   }
>>   
>>   static int
>> -coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
>> +coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset,
>> +                               int64_t bytes)
>>   {
>>       IscsiLun *iscsilun = bs->opaque;
>>       struct IscsiTask iTask;
> 
> Did you want to add some sort of assert(bytes / iscsilun->block_size
> <= UINT32_MAX), or a comment that we are relying on bl.max_pdiscard?

Yes, will add, we are storing it to list.num which is uint32_t and don't want it to be overflowed

> 
>> +++ b/block/sheepdog.c
> 
>> +static void sd_refresh_limits(BlockDriverState *bs, Error **errp)
>> +{
>> +    bs->bl.max_pdiscard = INT_MAX;
> 
> Do we want to round this down to alignment?
> 

anyway, block/sheepdog.c is absent now :)


-- 
Best regards,
Vladimir


More information about the integration mailing list