Skip to content
Snippets Groups Projects
  1. Feb 03, 2021
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: support int64_t bytes in bdrv_aligned_preadv() · 8b0c5d76
      Vladimir Sementsov-Ogievskiy authored
      
      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_aligned_preadv() now.
      
      Make the bytes variable in bdrv_padding_rmw_read() int64_t, as it is
      only used for pass-through to bdrv_aligned_preadv().
      
      All bdrv_aligned_preadv() callers are safe as type is widening. Let's
      look inside:
      
       - add a new-style assertion that request is good.
       - callees bdrv_is_allocated(), bdrv_co_do_copy_on_readv() supports
         int64_t bytes
       - conversion of bytes_remaining is OK, as we never have requests
         overflowing BDRV_MAX_LENGTH
       - looping through bytes_remaining is ok, num is updated to int64_t
         - for bdrv_driver_preadv we have same limit of max_transfer
         - qemu_iovec_memset is OK, as bytes+qiov_offset should not overflow
           qiov->size anyway (thanks to bdrv_check_qiov_request())
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-14-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      [eblake: grammar tweak]
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      8b0c5d76
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: support int64_t bytes in bdrv_co_do_copy_on_readv() · 9df5afbd
      Vladimir Sementsov-Ogievskiy authored
      
      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.
      
      'bytes' type widening, so callers are safe. Look at the function
      itself:
      
      bytes, skip_bytes and progress become int64_t.
      
      bdrv_round_to_clusters() is OK, cluster_bytes now may be large.
      trace_bdrv_co_do_copy_on_readv() is OK
      
      looping through cluster_bytes is still OK.
      
      pnum is still capped to max_transfer, and to MAX_BOUNCE_BUFFER when we
      are going to do COR operation. Therefor calculations in
      qemu_iovec_from_buf() and bdrv_driver_preadv() should not change.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-13-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      9df5afbd
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: support int64_t bytes in bdrv_aligned_pwritev() · fcfd9ade
      Vladimir Sementsov-Ogievskiy authored
      
      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_aligned_pwritev() now and convert the dependencies:
      bdrv_co_write_req_prepare() and bdrv_co_write_req_finish() to signed
      type bytes.
      
      Conversion of bdrv_co_write_req_prepare() and
      bdrv_co_write_req_finish() is definitely safe, as all requests in
      block/io must not overflow BDRV_MAX_LENGTH. Still add assertions.
      
      For bdrv_aligned_pwritev() 'bytes' type is widened, so callers are
      safe. Let's check usage of the parameter inside the function.
      
      Passing to bdrv_co_write_req_prepare() and bdrv_co_write_req_finish()
      is OK.
      
      Passing to qemu_iovec_* is OK after new assertion. All other callees
      are already updated to int64_t.
      
      Checking alignment is not changed, offset + bytes and qiov_offset +
      bytes calculations are safe (thanks to new assertions).
      
      max_transfer is kept to be int for now. It has a default of INT_MAX
      here, and some drivers may rely on it. It's to be refactored later.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-12-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      fcfd9ade
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes() · 5ae07b14
      Vladimir Sementsov-Ogievskiy authored
      
      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_pwrite_zeroes() now.
      
      Callers are safe, as converting int to int64_t is safe. Concentrate on
      'bytes' usage in the function (thx to Eric Blake):
      
          compute 'int tail' via % 'int alignment' - safe
          fragmentation loop 'int num' - still fragments with a cap on
            max_transfer
      
          use of 'num' within the loop
          MIN(bytes, max_transfer) as well as %alignment - still works, so
               calculations in if (head) {} are safe
          clamp size by 'int max_write_zeroes' - safe
          drv->bdrv_co_pwrite_zeroes(int) - safe because of clamping
          clamp size by 'int max_transfer' - safe
          buf allocation is still clamped to max_transfer
          qemu_iovec_init_buf(size_t) - safe because of clamping
          bdrv_driver_pwritev(uint64_t) - safe
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-11-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      5ae07b14
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: use int64_t bytes in driver wrappers · 17abcbee
      Vladimir Sementsov-Ogievskiy authored
      
      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 wrappers parameters which are already 64bit to
      signed type.
      
      Requests in block/io.c must never exceed BDRV_MAX_LENGTH (which is less
      than INT64_MAX), which makes the conversion to signed 64bit type safe.
      
      Add corresponding assertions.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-10-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      17abcbee
    • Eric Blake's avatar
      block: use int64_t as bytes type in tracked requests · 80247264
      Eric Blake authored
      
      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).
      
      All requests in block/io must not overflow BDRV_MAX_LENGTH, all
      external users of BdrvTrackedRequest already have corresponding
      assertions, so we are safe. Add some assertions still.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-9-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      80247264
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: improve bdrv_check_request: check qiov too · 63f4ad11
      Vladimir Sementsov-Ogievskiy authored
      
      Operations with qiov add more restrictions on bytes, let's cover it.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-8-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      63f4ad11
    • Vladimir Sementsov-Ogievskiy's avatar
      block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes · 801625e6
      Vladimir Sementsov-Ogievskiy authored
      
      The function is called from 64bit io handlers, and bytes is just passed
      to throttle_account() which is 64bit too (unsigned though). So, let's
      convert intermediate argument to 64bit too.
      
      This patch is a first in the 64-bit-blocklayer series, so 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).
      
      Patch-correctness audit by Eric Blake:
      
        Caller has 32-bit, this patch now causes widening which is safe:
        block/block-backend.c: blk_do_preadv() passes 'unsigned int'
        block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
        block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
        block/throttle.c: throttle_co_pdiscard() passes 'int'
      
        Caller has 64-bit, this patch fixes potential bug where pre-patch
        could narrow, except it's easy enough to trace that callers are still
        capped at 2G actions:
        block/throttle.c: throttle_co_preadv() passes 'uint64_t'
        block/throttle.c: throttle_co_pwritev() passes 'uint64_t'
      
        Implementation in question: block/throttle-groups.c
        throttle_group_co_io_limits_intercept() takes 'unsigned int bytes'
        and uses it: argument to util/throttle.c throttle_account(uint64_t)
      
        All safe: it patches a latent bug, and does not introduce any 64-bit
        gotchas once throttle_co_p{read,write}v are relaxed, and assuming
        throttle_account() is not buggy.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Reviewed-by: default avatarAlberto Garcia <berto@igalia.com>
      Message-Id: <20201211183934.169161-7-vsementsov@virtuozzo.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      801625e6
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: bdrv_pad_request(): support qemu_iovec_init_extended failure · 98ca4549
      Vladimir Sementsov-Ogievskiy authored
      
      Make bdrv_pad_request() honest: return error if
      qemu_iovec_init_extended() failed.
      
      Update also bdrv_padding_destroy() to clean the structure for safety.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-6-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      98ca4549
    • Vladimir Sementsov-Ogievskiy's avatar
      block/io: refactor bdrv_pad_request(): move bdrv_pad_request() up · f0deecff
      Vladimir Sementsov-Ogievskiy authored
      
      Prepare for the following patch when bdrv_pad_request() will be able to
      fail. Update the comments.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-5-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      [eblake: grammar tweak]
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      f0deecff
    • Vladimir Sementsov-Ogievskiy's avatar
      block: fix theoretical overflow in bdrv_init_padding() · a56ed80c
      Vladimir Sementsov-Ogievskiy authored
      
      Calculation of sum may theoretically overflow, so use 64bit type and
      add some good assertions.
      
      Use int64_t constantly.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-4-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      [eblake: tweak assertion order]
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      a56ed80c
    • Vladimir Sementsov-Ogievskiy's avatar
      util/iov: make qemu_iovec_init_extended() honest · 4c002cef
      Vladimir Sementsov-Ogievskiy authored
      
      Actually, we can't extend the io vector in all cases. Handle possible
      MAX_IOV and size_t overflows.
      
      For now add assertion to callers (actually they rely on success anyway)
      and fix them in the following patch.
      
      Add also some additional good assertions to qemu_iovec_init_slice()
      while being here.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-3-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      4c002cef
    • Vladimir Sementsov-Ogievskiy's avatar
      block: refactor bdrv_check_request: add errp · 69b55e03
      Vladimir Sementsov-Ogievskiy authored
      
      It's better to pass &error_abort than just assert that result is 0: on
      crash, we'll immediately see the reason in the backtrace.
      
      Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
      Message-Id: <20201211183934.169161-2-vsementsov@virtuozzo.com>
      Reviewed-by: default avatarEric Blake <eblake@redhat.com>
      [eblake: fix iotest 206 fallout]
      Signed-off-by: default avatarEric Blake <eblake@redhat.com>
      69b55e03
  2. Feb 02, 2021
  3. Jan 28, 2021
  4. Jan 27, 2021
    • Kevin Wolf's avatar
      block: Separate blk_is_writable() and blk_supports_write_perm() · 86b1cf32
      Kevin Wolf authored
      Currently, blk_is_read_only() tells whether a given BlockBackend can
      only be used in read-only mode because its root node is read-only. Some
      callers actually try to answer a slightly different question: Is the
      BlockBackend configured to be writable, by taking write permissions on
      the root node?
      
      This can differ, for example, for CD-ROM devices which don't take write
      permissions, but may be backed by a writable image file. scsi-cd allows
      write requests to the drive if blk_is_read_only() returns false.
      However, the write request will immediately run into an assertion
      failure because the write permission is missing.
      
      This patch introduces separate functions for both questions.
      blk_supports_write_perm() answers the question whether the block
      node/image file can support writable devices, whereas blk_is_writable()
      tells whether the BlockBackend is currently configured to be writable.
      
      All calls of blk_is_read_only() are converted to one of the two new
      functions.
      
      Fixes: https://bugs.launchpad.net/bugs/1906693
      
      
      Cc: qemu-stable@nongnu.org
      Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
      Message-Id: <20210118123448.307825-2-kwolf@redhat.com>
      Reviewed-by: default avatarPhilippe Mathieu-Daudé <philmd@redhat.com>
      Reviewed-by: default avatarMax Reitz <mreitz@redhat.com>
      Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
      86b1cf32
  5. Jan 26, 2021
Loading