qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Eric Blake <eblake@redhat.com>, qemu-block@nongnu.org
Cc: kwolf@redhat.com, fam@euphon.net, integration@gluster.org,
	berto@igalia.com, pavel.dovgaluk@ispras.ru, dillaman@redhat.com,
	qemu-devel@nongnu.org, sw@weilnetz.de, pl@kamp.de,
	ronniesahlberg@gmail.com, mreitz@redhat.com, den@openvz.org,
	sheepdog@lists.wpkg.org, stefanha@redhat.com,
	namei.unix@gmail.com, pbonzini@redhat.com, jsnow@redhat.com,
	ari@tuxera.com
Subject: Re: [PATCH v3 07/17] block/io: support int64_t bytes in bdrv_co_do_copy_on_readv()
Date: Fri, 22 May 2020 09:30:01 +0300	[thread overview]
Message-ID: <c89426e8-acc5-b2b3-2c4a-dd051ea07779@virtuozzo.com> (raw)
In-Reply-To: <918dca07-00f2-af0f-b410-54537524e5ef@redhat.com>

22.05.2020 01:29, Eric Blake wrote:
> On 4/30/20 6:10 AM, 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, prepare bdrv_co_do_copy_on_readv() now.
>>
>> Series: 64bit-block-status
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c         | 6 +++---
>>   block/trace-events | 2 +-
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 8bb4ea6285..6990d8cabe 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1088,7 +1088,7 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, int64_t offset,
>>   }
>>   static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>> -        int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov,
>>           size_t qiov_offset, int flags)
> 
> Widens from 32-bit to 63-bit.  One caller:
> 
> bdrv_aligned_preadv() passes unsigned int (for now) - safe
> 
>>   {
>>       BlockDriverState *bs = child->bs;
>> @@ -1103,11 +1103,11 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>       BlockDriver *drv = bs->drv;
>>       int64_t cluster_offset;
>>       int64_t cluster_bytes;
>> -    size_t skip_bytes;
>> +    int64_t skip_bytes;
>>       int ret;
>>       int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
>>                                       BDRV_REQUEST_MAX_BYTES);
>> -    unsigned int progress = 0;
>> +    int64_t progress = 0;
>>       bool skip_write;
> 
> Use of 'bytes', 'sskip_bytes', and 'progress' within the function:
>      bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
>   - safe, takes int64_t. Pre-patch, cluster_bytes could be 33 bits (a misaligned request just under UINT_MAX can expand to > UINT_MAX when aligned to clusters), but only if bytes could be larger than our <2G cap that we use elsewhere.  But even if we relax that 2G cap in later patches, we should be okay even if 'bytes' starts at larger than 32 bits, because we don't allow images that would overflow INT64_MAX when rounded up to cluster boundaries
> 
>      skip_bytes = offset - cluster_offset;
>   - actually oversized - the difference is never going to be larger than a cluster (which is capped at 2M for qcow2, for example), but doesn't hurt that it is now a 64-bit value
> 
>      trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
>   - safe, tweaked in this patch
> 
>                  assert(progress >= bytes);
>   - safe: progress never exceeds pnum, and both variables are same type pre- and post-patch
> 
>              assert(skip_bytes < pnum);
>   - safe
> 
>                  qemu_iovec_from_buf(qiov, qiov_offset + progress,
>                                      bounce_buffer + skip_bytes,
>                                      MIN(pnum - skip_bytes, bytes - progress));
>   - tricky - pre-patch, pnum was int64_t, post-patch, we have three more int64_t entities.  Either way, we're passing int64_t to a size_t parameter, which narrows on 64-bit.  However, we're safe: this call is in a loop where pnum is clamped at MAX_BOUNCE_BUFFER which is less than 32 bits, and the MIN() here means we never overflow
> 
>              ret = bdrv_driver_preadv(bs, offset + progress,
>                                       MIN(pnum - skip_bytes, bytes - progress),
>                                       qiov, qiov_offset + progress, 0);
>   - safe - takes int64_t (earlier in this series), and same analysis about the MIN() picking something clamped at MAX_BOUNCE_BUFFER
> 
>          progress += pnum - skip_bytes;
>          skip_bytes = 0;
>   - safe
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks! Hmm. I'm afraid that I'll rebase this back to master, postponing "[PATCH v2 0/9] block/io: safer inc/dec in_flight sections", as I doubt that it is needed..

-- 
Best regards,
Vladimir


  reply	other threads:[~2020-05-22  6:30 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-30 11:10 [PATCH v3 00/17] 64bit block-layer Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes Vladimir Sementsov-Ogievskiy
2020-05-11 15:28   ` Alberto Garcia
2020-04-30 11:10 ` [PATCH v3 02/17] block: use int64_t as bytes type in tracked requests Vladimir Sementsov-Ogievskiy
2020-05-11 15:32   ` Alberto Garcia
2020-05-22 19:09   ` Eric Blake
2020-04-30 11:10 ` [PATCH v3 03/17] block/io: use int64_t bytes parameter in bdrv_check_byte_request() Vladimir Sementsov-Ogievskiy
2020-05-11 15:57   ` Alberto Garcia
2020-04-30 11:10 ` [PATCH v3 04/17] block/io: use int64_t bytes in driver wrappers Vladimir Sementsov-Ogievskiy
2020-05-11 16:30   ` Alberto Garcia
2020-04-30 11:10 ` [PATCH v3 05/17] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes() Vladimir Sementsov-Ogievskiy
2020-05-08 18:20   ` Eric Blake
2020-05-11 17:17   ` Alberto Garcia
2020-05-11 18:34     ` Eric Blake
2020-06-23 10:20       ` Vladimir Sementsov-Ogievskiy
2020-06-23 16:37         ` Eric Blake
2020-04-30 11:10 ` [PATCH v3 06/17] block/io: support int64_t bytes in bdrv_aligned_pwritev() Vladimir Sementsov-Ogievskiy
2020-05-08 20:38   ` Eric Blake
2020-06-18 14:29   ` Alberto Garcia
2020-04-30 11:10 ` [PATCH v3 07/17] block/io: support int64_t bytes in bdrv_co_do_copy_on_readv() Vladimir Sementsov-Ogievskiy
2020-05-21 22:29   ` Eric Blake
2020-05-22  6:30     ` Vladimir Sementsov-Ogievskiy [this message]
2020-04-30 11:10 ` [PATCH v3 08/17] block/io: support int64_t bytes in bdrv_aligned_preadv() Vladimir Sementsov-Ogievskiy
2020-05-22 15:14   ` Eric Blake
2020-06-18 14:35     ` Alberto Garcia
2020-06-18 14:47       ` Eric Blake
2020-04-30 11:10 ` [PATCH v3 09/17] block/io: support int64_t bytes in bdrv_co_p{read, write}v_part() Vladimir Sementsov-Ogievskiy
2020-05-22 19:34   ` [PATCH v3 09/17] block/io: support int64_t bytes in bdrv_co_p{read,write}v_part() Eric Blake
2020-04-30 11:10 ` [PATCH v3 10/17] block/io: support int64_t bytes in read/write wrappers Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 11/17] block/io: use int64_t bytes in copy_range Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 12/17] block/block-backend: convert blk io path to use int64_t parameters Vladimir Sementsov-Ogievskiy
2020-06-23 22:11   ` Eric Blake
2020-04-30 11:10 ` [PATCH v3 13/17] block: use int64_t instead of uint64_t in driver read handlers Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 14/17] block: use int64_t instead of uint64_t in driver write handlers Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 15/17] block: use int64_t instead of uint64_t in copy_range driver handlers Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 16/17] block: use int64_t instead of int in driver write_zeroes handlers Vladimir Sementsov-Ogievskiy
2020-04-30 11:10 ` [PATCH v3 17/17] block: use int64_t instead of int in driver discard handlers Vladimir Sementsov-Ogievskiy
2020-05-06  6:40   ` Vladimir Sementsov-Ogievskiy
2020-04-30 20:51 ` [PATCH v3 00/17] 64bit block-layer no-reply
2020-05-06  6:39   ` Vladimir Sementsov-Ogievskiy
2020-04-30 20:57 ` no-reply
2020-12-01 16:07 ` Vladimir Sementsov-Ogievskiy
2020-12-01 16:56   ` Vladimir Sementsov-Ogievskiy
2020-12-01 21:50   ` Eric Blake

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c89426e8-acc5-b2b3-2c4a-dd051ea07779@virtuozzo.com \
    --to=vsementsov@virtuozzo.com \
    --cc=ari@tuxera.com \
    --cc=berto@igalia.com \
    --cc=den@openvz.org \
    --cc=dillaman@redhat.com \
    --cc=eblake@redhat.com \
    --cc=fam@euphon.net \
    --cc=integration@gluster.org \
    --cc=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=namei.unix@gmail.com \
    --cc=pavel.dovgaluk@ispras.ru \
    --cc=pbonzini@redhat.com \
    --cc=pl@kamp.de \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=sheepdog@lists.wpkg.org \
    --cc=stefanha@redhat.com \
    --cc=sw@weilnetz.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).