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

So, prepare bdrv_co_do_copy_on_readv() now.

Series: 64bit-block-status
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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,
-    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 <ebl...@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,
sheepdog mailing list

Reply via email to