[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