[Qemu-devel] [PATCH 0/5] Byte-based block limits

[Qemu-devel] [PATCH 0/5] Byte-based block limits

[Eric Blake]

BlockLimits is currently an ugly mix of byte limits vs.
sector limits.  Unify it.  Fix some bugs I found in
bdrv_aligned_preadv() while at it.

Probably conflicts with Kevin's ongoing work to migrate
bdrv_aligned_preadv() to be byte-based, but I found this
handy before tackling conversion of 'discard' interfaces,
and before implementing an auto-fragmenting to max_transfer
size at the block layer.

Also available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-limits-v1

Eric Blake (5):
  block: Tighter assertions on bdrv_aligned_preadv()
  block: Honor flags during bdrv_aligned_preadv()
  block: Switch transfer length bounds to byte-based
  block: Switch discard length bounds to byte-based
  block: Move request_alignment into BlockLimit

 include/block/block_int.h      | 43 +++++++++++++++++++----------
 include/sysemu/block-backend.h |  2 +-
 block.c                        |  4 +--
 block/blkdebug.c               |  4 +--
 block/block-backend.c          |  9 +++---
 block/bochs.c                  |  2 +-
 block/cloop.c                  |  2 +-
 block/dmg.c                    |  2 +-
 block/io.c                     | 62 ++++++++++++++++++++++--------------------
 block/iscsi.c                  | 37 ++++++++++++-------------
 block/nbd.c                    |  4 +--
 block/raw-posix.c              | 18 ++++++------
 block/raw-win32.c              |  6 ++--
 block/vvfat.c                  |  2 +-
 hw/block/virtio-blk.c          | 12 ++++----
 hw/scsi/scsi-generic.c         | 14 +++++-----
 qemu-img.c                     |  9 +++---
 17 files changed, 126 insertions(+), 106 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/5] block: Tighter assertions on bdrv_aligned_preadv()

[Eric Blake]

Make sure everything is aligned to the passed-in alignment,
not just sectors.  Also makes sure that the computation of
max_nb_sectors later in the function will not divide by
zero, by guaranteeing align is at least a sector.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index dff4384..81618b9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -947,8 +947,9 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
     int64_t sector_num = offset >> BDRV_SECTOR_BITS;
     unsigned int nb_sectors = bytes >> BDRV_SECTOR_BITS;

-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(align / BDRV_SECTOR_SIZE);
+    assert((offset & (align - 1)) == 0);
+    assert((bytes & (align - 1)) == 0);
     assert(!qiov || bytes == qiov->size);
     assert((bs->open_flags & BDRV_O_NO_IO) == 0);

-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/5] block: Tighter assertions on bdrv_aligned_preadv()

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> Make sure everything is aligned to the passed-in alignment,
> not just sectors.  Also makes sure that the computation of
> max_nb_sectors later in the function will not divide by
> zero, by guaranteeing align is at least a sector.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Trivial conflict with my work, can rebase.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 2/5] block: Honor flags during bdrv_aligned_preadv()

[Eric Blake]

Not that we pass any flags during reads yet, but we may want to
support BDRV_REQ_FUA on reads in the future.  So don't throw
away the input flags.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index 81618b9..3c7b233 100644
--- a/block/io.c
+++ b/block/io.c
@@ -983,7 +983,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,

     /* Forward the request to the BlockDriver */
     if (!bs->zero_beyond_eof) {
-        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
+        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
     } else {
         /* Read zeros after EOF */
         int64_t total_sectors, max_nb_sectors;
@@ -997,7 +997,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
         max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
                                   align >> BDRV_SECTOR_BITS);
         if (nb_sectors < max_nb_sectors) {
-            ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
+            ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
         } else if (max_nb_sectors > 0) {
             QEMUIOVector local_qiov;

@@ -1007,7 +1007,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,

             ret = bdrv_driver_preadv(bs, offset,
                                      max_nb_sectors * BDRV_SECTOR_SIZE,
-                                     &local_qiov, 0);
+                                     &local_qiov, flags);

             qemu_iovec_destroy(&local_qiov);
         } else {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/5] block: Honor flags during bdrv_aligned_preadv()

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> Not that we pass any flags during reads yet, but we may want to
> support BDRV_REQ_FUA on reads in the future.  So don't throw
> away the input flags.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Do we want to pass flags to bdrv_co_do_copy_on_readv(), too? I guess we
would use them for the preadv call there, but continue to use 0 as the
pwritev flags.

We may also want to introduce a .supported_read_flags, but perhaps this
is not something to do in this patch. And we don't even use
.supported_write_flags for drivers that have a native .bdrv_co_pwritev.
Of course, that's even less related to this patch. I guess I should send
a fix for that.

Looks good otherwise.

Kevin

Re: [Qemu-devel] [PATCH 2/5] block: Honor flags during bdrv_aligned_preadv()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

On 06/07/2016 06:12 AM, Kevin Wolf wrote:
> Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
>> Not that we pass any flags during reads yet, but we may want to
>> support BDRV_REQ_FUA on reads in the future.  So don't throw
>> away the input flags.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Do we want to pass flags to bdrv_co_do_copy_on_readv(), too? I guess we
> would use them for the preadv call there, but continue to use 0 as the
> pwritev flags.

What about BDRV_REQ_MAY_UNMAP if bdrv_co_do_copy_on_readv() detects a
block of zeroes?

This just got a lot trickier, so I think my short-term solution is to
just assert(!flags) and add a comment that whoever implements flags on
read has to solve the issue at that time, so that I'm not stalling the
rest of this series on thinking about things that don't matter yet.

> 
> We may also want to introduce a .supported_read_flags, but perhaps this
> is not something to do in this patch.

Yeah, I think we'll want that, at the point where we start worrying
about read flags.

> And we don't even use
> .supported_write_flags for drivers that have a native .bdrv_co_pwritev.
> Of course, that's even less related to this patch. I guess I should send
> a fix for that.

Thanks.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based

[Eric Blake]

Sector-based limits are awkward to think about; in our on-going
quest to move to byte-based interfaces, convert max_transfer_length
and opt_transfer_length.  Rename them (dropping the _length suffix)
so that the compiler will help us catch the change in semantics
across any rebased code.  Improve the documentation, and guarantee
that blk_get_max_transfer() returns a non-zero value.  Use unsigned
values, so that we don't have to worry about negative values and
so that bit-twiddling is easier; however, we are still constrained
by 2^31 of signed int in most APIs.

Of note: the iscsi calculations use a 64-bit number internally,
but the final result is guaranteed to fit in 32 bits.  NBD is
fixed to advertise the maximum length of 32M that the qemu and
kernel servers enforce, rather than a number of sectors that
would overflow int when converted back to bytes.  scsi-generic
now advertises a maximum always, rather than just when the
underlying device advertised a maximum.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h      | 10 ++++++----
 include/sysemu/block-backend.h |  2 +-
 block/block-backend.c          |  9 +++++----
 block/io.c                     | 23 +++++++++++------------
 block/iscsi.c                  | 18 ++++++++++--------
 block/nbd.c                    |  2 +-
 block/raw-posix.c              |  2 +-
 hw/block/virtio-blk.c          | 12 +++++++-----
 hw/scsi/scsi-generic.c         | 14 +++++++-------
 qemu-img.c                     |  8 ++++----
 10 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a4963c..96e8476 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -336,11 +336,13 @@ typedef struct BlockLimits {
      * power of 2, and less than max_pwrite_zeroes if that is set */
     uint32_t pwrite_zeroes_alignment;

-    /* optimal transfer length in sectors */
-    int opt_transfer_length;
+    /* optimal transfer length in bytes (must be power of 2, and
+     * multiple of bs->request_alignment), or 0 if no preferred size */
+    uint32_t opt_transfer;

-    /* maximal transfer length in sectors */
-    int max_transfer_length;
+    /* maximal transfer length in bytes (need not be power of 2, but
+     * should be multiple of opt_transfer), or 0 for no 32-bit limit */
+    uint32_t max_transfer;

     /* memory alignment so that no bounce buffer is needed */
     size_t min_mem_alignment;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..2469a1c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -170,7 +170,7 @@ bool blk_is_available(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
-int blk_get_max_transfer_length(BlockBackend *blk);
+uint32_t blk_get_max_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_try_blockalign(BlockBackend *blk, size_t size);
diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..19c1f7f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1303,15 +1303,16 @@ int blk_get_flags(BlockBackend *blk)
     }
 }

-int blk_get_max_transfer_length(BlockBackend *blk)
+/* Returns the maximum transfer length, in bytes; guaranteed nonzero */
+uint32_t blk_get_max_transfer(BlockBackend *blk)
 {
     BlockDriverState *bs = blk_bs(blk);
+    uint32_t max = 0;

     if (bs) {
-        return bs->bl.max_transfer_length;
-    } else {
-        return 0;
+        max = bs->bl.max_transfer;
     }
+    return MIN_NON_ZERO(max, BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS);
 }

 int blk_get_max_iov(BlockBackend *blk)
diff --git a/block/io.c b/block/io.c
index 3c7b233..a2bc254 100644
--- a/block/io.c
+++ b/block/io.c
@@ -85,8 +85,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
             error_propagate(errp, local_err);
             return;
         }
-        bs->bl.opt_transfer_length = bs->file->bs->bl.opt_transfer_length;
-        bs->bl.max_transfer_length = bs->file->bs->bl.max_transfer_length;
+        bs->bl.opt_transfer = bs->file->bs->bl.opt_transfer;
+        bs->bl.max_transfer = bs->file->bs->bl.max_transfer;
         bs->bl.min_mem_alignment = bs->file->bs->bl.min_mem_alignment;
         bs->bl.opt_mem_alignment = bs->file->bs->bl.opt_mem_alignment;
         bs->bl.max_iov = bs->file->bs->bl.max_iov;
@@ -104,12 +104,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
             error_propagate(errp, local_err);
             return;
         }
-        bs->bl.opt_transfer_length =
-            MAX(bs->bl.opt_transfer_length,
-                bs->backing->bs->bl.opt_transfer_length);
-        bs->bl.max_transfer_length =
-            MIN_NON_ZERO(bs->bl.max_transfer_length,
-                         bs->backing->bs->bl.max_transfer_length);
+        bs->bl.opt_transfer = MAX(bs->bl.opt_transfer,
+                                  bs->backing->bs->bl.opt_transfer);
+        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
+                                           bs->backing->bs->bl.max_transfer);
         bs->bl.opt_mem_alignment =
             MAX(bs->bl.opt_mem_alignment,
                 bs->backing->bs->bl.opt_mem_alignment);
@@ -1119,7 +1117,8 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
     return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
 }

-#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
+/* Maximum buffer for write zeroes fallback, in bytes */
+#define MAX_WRITE_ZEROES_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)

 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags)
@@ -1177,7 +1176,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,

         if (ret == -ENOTSUP) {
             /* Fall back to bounce buffer if write zeroes is unsupported */
-            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
+            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer,
                                             MAX_WRITE_ZEROES_BOUNCE_BUFFER);
             BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;

@@ -1188,7 +1187,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
                 write_flags &= ~BDRV_REQ_FUA;
                 need_flush = true;
             }
-            num = MIN(num, max_xfer_len << BDRV_SECTOR_BITS);
+            num = MIN(num, max_xfer_len);
             iov.iov_len = num;
             if (iov.iov_base == NULL) {
                 iov.iov_base = qemu_try_blockalign(bs, num);
@@ -1205,7 +1204,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
             /* Keep bounce buffer around if it is big enough for all
              * all future requests.
              */
-            if (num < max_xfer_len << BDRV_SECTOR_BITS) {
+            if (num < max_xfer_len) {
                 qemu_vfree(iov.iov_base);
                 iov.iov_base = NULL;
             }
diff --git a/block/iscsi.c b/block/iscsi.c
index 7e78ade..c5855be 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -473,9 +473,10 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
         return -EINVAL;
     }

-    if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) {
+    if (bs->bl.max_transfer &&
+        nb_sectors << BDRV_SECTOR_BITS > bs->bl.max_transfer) {
         error_report("iSCSI Error: Write of %d sectors exceeds max_xfer_len "
-                     "of %d sectors", nb_sectors, bs->bl.max_transfer_length);
+                     "of %" PRIu32 " bytes", nb_sectors, bs->bl.max_transfer);
         return -EINVAL;
     }

@@ -650,9 +651,10 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
         return -EINVAL;
     }

-    if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) {
+    if (bs->bl.max_transfer &&
+        nb_sectors << BDRV_SECTOR_BITS > bs->bl.max_transfer) {
         error_report("iSCSI Error: Read of %d sectors exceeds max_xfer_len "
-                     "of %d sectors", nb_sectors, bs->bl.max_transfer_length);
+                     "of %" PRIu32 " bytes", nb_sectors, bs->bl.max_transfer);
         return -EINVAL;
     }

@@ -1706,13 +1708,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
      * iscsi_open(): iscsi targets don't change their limits. */

     IscsiLun *iscsilun = bs->opaque;
-    uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
+    uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;

     if (iscsilun->bl.max_xfer_len) {
         max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
     }

-    bs->bl.max_transfer_length = sector_limits_lun2qemu(max_xfer_len, iscsilun);
+    bs->bl.max_transfer = MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
+                              max_xfer_len * iscsilun->block_size);

     if (iscsilun->lbp.lbpu) {
         if (iscsilun->bl.max_unmap < 0xffffffff) {
@@ -1735,8 +1738,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
     } else {
         bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
     }
-    bs->bl.opt_transfer_length =
-        sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
+    bs->bl.opt_transfer = iscsilun->bl.opt_xfer_len * iscsilun->block_size;
 }

 /* Note that this will not re-establish a connection with an iSCSI target - it
diff --git a/block/nbd.c b/block/nbd.c
index 6015e8b..2ce7b4d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -363,7 +363,7 @@ static int nbd_co_flush(BlockDriverState *bs)
 static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
-    bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS;
+    bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
 }

 static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..f3bd5ef 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -752,7 +752,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
         if (S_ISBLK(st.st_mode)) {
             int ret = hdev_get_max_transfer_length(s->fd);
             if (ret >= 0) {
-                bs->bl.max_transfer_length = ret;
+                bs->bl.max_transfer = ret << BDRV_SECTOR_BITS;
             }
         }
     }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 284e646..fbaf8f5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -382,7 +382,7 @@ static int multireq_compare(const void *a, const void *b)
 void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
 {
     int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0;
-    int max_xfer_len = 0;
+    uint32_t max_xfer_len = 0;
     int64_t sector_num = 0;

     if (mrb->num_reqs == 1) {
@@ -391,8 +391,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
         return;
     }

-    max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
-    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
+    max_xfer_len = blk_get_max_transfer(mrb->reqs[0]->dev->blk);
+    assert(max_xfer_len &&
+           max_xfer_len >> BDRV_SECTOR_BITS <= BDRV_REQUEST_MAX_SECTORS);

     qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
           &multireq_compare);
@@ -408,8 +409,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
              */
             if (sector_num + nb_sectors != req->sector_num ||
                 niov > blk_get_max_iov(blk) - req->qiov.niov ||
-                req->qiov.size / BDRV_SECTOR_SIZE > max_xfer_len ||
-                nb_sectors > max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE) {
+                req->qiov.size > max_xfer_len ||
+                nb_sectors > (max_xfer_len -
+                              req->qiov.size) / BDRV_SECTOR_SIZE) {
                 submit_requests(blk, mrb, start, num_reqs, niov);
                 num_reqs = 0;
             }
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 71372a8..c6fef32 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -225,13 +225,13 @@ static void scsi_read_complete(void * opaque, int ret)
     if (s->type == TYPE_DISK &&
         r->req.cmd.buf[0] == INQUIRY &&
         r->req.cmd.buf[2] == 0xb0) {
-        uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk);
-        if (max_xfer_len) {
-            stl_be_p(&r->buf[8], max_xfer_len);
-            /* Also take care of the opt xfer len. */
-            if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
-                stl_be_p(&r->buf[12], max_xfer_len);
-            }
+        uint32_t max_xfer_len =
+            blk_get_max_transfer(s->conf.blk) >> BDRV_SECTOR_BITS;
+
+        stl_be_p(&r->buf[8], max_xfer_len);
+        /* Also take care of the opt xfer len. */
+        if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
+            stl_be_p(&r->buf[12], max_xfer_len);
         }
     }
     scsi_req_data(&r->req, len);
diff --git a/qemu-img.c b/qemu-img.c
index 4b56ad3..577df0d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2074,13 +2074,13 @@ static int img_convert(int argc, char **argv)
     }
     out_bs = blk_bs(out_blk);

-    /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
+    /* increase bufsectors from the default 4096 (2M) if opt_transfer
      * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
      * as maximum. */
     bufsectors = MIN(32768,
-                     MAX(bufsectors, MAX(out_bs->bl.opt_transfer_length,
-                                         out_bs->bl.discard_alignment))
-                    );
+                     MAX(bufsectors,
+                         MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
+                             out_bs->bl.discard_alignment)));

     if (skip_create) {
         int64_t output_sectors = blk_nb_sectors(out_blk);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_transfer_length
> and opt_transfer_length.  Rename them (dropping the _length suffix)
> so that the compiler will help us catch the change in semantics
> across any rebased code.  Improve the documentation, and guarantee
> that blk_get_max_transfer() returns a non-zero value.  Use unsigned
> values, so that we don't have to worry about negative values and
> so that bit-twiddling is easier; however, we are still constrained
> by 2^31 of signed int in most APIs.
> 
> Of note: the iscsi calculations use a 64-bit number internally,
> but the final result is guaranteed to fit in 32 bits.  NBD is
> fixed to advertise the maximum length of 32M that the qemu and
> kernel servers enforce, rather than a number of sectors that
> would overflow int when converted back to bytes.  scsi-generic
> now advertises a maximum always, rather than just when the
> underlying device advertised a maximum.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

> @@ -1177,7 +1176,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> 
>          if (ret == -ENOTSUP) {
>              /* Fall back to bounce buffer if write zeroes is unsupported */
> -            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
> +            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer,
>                                              MAX_WRITE_ZEROES_BOUNCE_BUFFER);

You could consider renaming the variable to max_transfer to keep it
consistent with the BlockLimits field name and to make it even more
obvious that you converted all uses.

>              BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
> 
> @@ -1188,7 +1187,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>                  write_flags &= ~BDRV_REQ_FUA;
>                  need_flush = true;
>              }
> -            num = MIN(num, max_xfer_len << BDRV_SECTOR_BITS);
> +            num = MIN(num, max_xfer_len);
>              iov.iov_len = num;
>              if (iov.iov_base == NULL) {
>                  iov.iov_base = qemu_try_blockalign(bs, num);
> @@ -1205,7 +1204,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>              /* Keep bounce buffer around if it is big enough for all
>               * all future requests.
>               */
> -            if (num < max_xfer_len << BDRV_SECTOR_BITS) {
> +            if (num < max_xfer_len) {
>                  qemu_vfree(iov.iov_base);
>                  iov.iov_base = NULL;
>              }
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 7e78ade..c5855be 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -473,9 +473,10 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>          return -EINVAL;
>      }
> 
> -    if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) {
> +    if (bs->bl.max_transfer &&
> +        nb_sectors << BDRV_SECTOR_BITS > bs->bl.max_transfer) {
>          error_report("iSCSI Error: Write of %d sectors exceeds max_xfer_len "
> -                     "of %d sectors", nb_sectors, bs->bl.max_transfer_length);
> +                     "of %" PRIu32 " bytes", nb_sectors, bs->bl.max_transfer);
>          return -EINVAL;
>      }
> 
> @@ -650,9 +651,10 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>          return -EINVAL;
>      }
> 
> -    if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) {
> +    if (bs->bl.max_transfer &&
> +        nb_sectors << BDRV_SECTOR_BITS > bs->bl.max_transfer) {
>          error_report("iSCSI Error: Read of %d sectors exceeds max_xfer_len "
> -                     "of %d sectors", nb_sectors, bs->bl.max_transfer_length);
> +                     "of %" PRIu32 " bytes", nb_sectors, bs->bl.max_transfer);
>          return -EINVAL;
>      }
> 
> @@ -1706,13 +1708,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>       * iscsi_open(): iscsi targets don't change their limits. */
> 
>      IscsiLun *iscsilun = bs->opaque;
> -    uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
> +    uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
> 
>      if (iscsilun->bl.max_xfer_len) {
>          max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
>      }
> 
> -    bs->bl.max_transfer_length = sector_limits_lun2qemu(max_xfer_len, iscsilun);
> +    bs->bl.max_transfer = MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
> +                              max_xfer_len * iscsilun->block_size);

Why don't you simply use 0 when you defined it as no limit?

If we make some drivers put 0 there and others INT_MAX, chances are that
we'll introduce places where we fail to handle 0 correctly.

>      if (iscsilun->lbp.lbpu) {
>          if (iscsilun->bl.max_unmap < 0xffffffff) {
> @@ -1735,8 +1738,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>      } else {
>          bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
>      }
> -    bs->bl.opt_transfer_length =
> -        sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
> +    bs->bl.opt_transfer = iscsilun->bl.opt_xfer_len * iscsilun->block_size;
>  }
> 
>  /* Note that this will not re-establish a connection with an iSCSI target - it
> diff --git a/block/nbd.c b/block/nbd.c
> index 6015e8b..2ce7b4d 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -363,7 +363,7 @@ static int nbd_co_flush(BlockDriverState *bs)
>  static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
> -    bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS;
> +    bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
>  }

This introduces a semantic difference and should therefore be a separate
patch. Here, it should become UINT32_MAX through mechanical conversion.

Or actually, it can't because that's not a power of two. So maybe have
the NBD patch first...

>  static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ce2e20f..f3bd5ef 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -752,7 +752,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>          if (S_ISBLK(st.st_mode)) {
>              int ret = hdev_get_max_transfer_length(s->fd);
>              if (ret >= 0) {
> -                bs->bl.max_transfer_length = ret;
> +                bs->bl.max_transfer = ret << BDRV_SECTOR_BITS;

I assume that we don't care about overflows here because in practice the
values are very low? Should we check (or maybe better just cap) it
anyway?

>              }
>          }
>      }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 284e646..fbaf8f5 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -382,7 +382,7 @@ static int multireq_compare(const void *a, const void *b)
>  void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>  {
>      int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0;
> -    int max_xfer_len = 0;
> +    uint32_t max_xfer_len = 0;
>      int64_t sector_num = 0;

Again, consider renaming the variable.

>      if (mrb->num_reqs == 1) {
> @@ -391,8 +391,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>          return;
>      }
> 
> -    max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
> -    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
> +    max_xfer_len = blk_get_max_transfer(mrb->reqs[0]->dev->blk);
> +    assert(max_xfer_len &&
> +           max_xfer_len >> BDRV_SECTOR_BITS <= BDRV_REQUEST_MAX_SECTORS);

Why can we assert this here? The comment for BlockLimits.max_xfer_len
doesn't document any maximum. Of course, as I already said above, it
might not happen in practice, but INT_MAX + 1 is theoretically valid and
would fail the assertion.

>      qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
>            &multireq_compare);
> @@ -408,8 +409,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>               */
>              if (sector_num + nb_sectors != req->sector_num ||
>                  niov > blk_get_max_iov(blk) - req->qiov.niov ||
> -                req->qiov.size / BDRV_SECTOR_SIZE > max_xfer_len ||
> -                nb_sectors > max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE) {
> +                req->qiov.size > max_xfer_len ||
> +                nb_sectors > (max_xfer_len -
> +                              req->qiov.size) / BDRV_SECTOR_SIZE) {
>                  submit_requests(blk, mrb, start, num_reqs, niov);
>                  num_reqs = 0;
>              }
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 71372a8..c6fef32 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -225,13 +225,13 @@ static void scsi_read_complete(void * opaque, int ret)
>      if (s->type == TYPE_DISK &&
>          r->req.cmd.buf[0] == INQUIRY &&
>          r->req.cmd.buf[2] == 0xb0) {
> -        uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk);
> -        if (max_xfer_len) {
> -            stl_be_p(&r->buf[8], max_xfer_len);
> -            /* Also take care of the opt xfer len. */
> -            if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
> -                stl_be_p(&r->buf[12], max_xfer_len);
> -            }
> +        uint32_t max_xfer_len =
> +            blk_get_max_transfer(s->conf.blk) >> BDRV_SECTOR_BITS;
> +
> +        stl_be_p(&r->buf[8], max_xfer_len);
> +        /* Also take care of the opt xfer len. */
> +        if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
> +            stl_be_p(&r->buf[12], max_xfer_len);
>          }
>      }

This is another hidden semantic change. Can we have a separate
scsi-generic patch first that changes the handling for the
max_transfer == 0 case and only then make the change in
blk_get_max_transfer() as pure refactoring without a change in
behaviour?

>      scsi_req_data(&r->req, len);
> diff --git a/qemu-img.c b/qemu-img.c
> index 4b56ad3..577df0d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2074,13 +2074,13 @@ static int img_convert(int argc, char **argv)
>      }
>      out_bs = blk_bs(out_blk);
> 
> -    /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
> +    /* increase bufsectors from the default 4096 (2M) if opt_transfer
>       * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
>       * as maximum. */
>      bufsectors = MIN(32768,
> -                     MAX(bufsectors, MAX(out_bs->bl.opt_transfer_length,
> -                                         out_bs->bl.discard_alignment))
> -                    );
> +                     MAX(bufsectors,
> +                         MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
> +                             out_bs->bl.discard_alignment)));
> 
>      if (skip_create) {
>          int64_t output_sectors = blk_nb_sectors(out_blk);
> -- 
> 2.5.5

Kevin

Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6887 bytes --]

On 06/07/2016 06:45 AM, Kevin Wolf wrote:
> Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
>> Sector-based limits are awkward to think about; in our on-going
>> quest to move to byte-based interfaces, convert max_transfer_length
>> and opt_transfer_length.  Rename them (dropping the _length suffix)
>> so that the compiler will help us catch the change in semantics
>> across any rebased code.  Improve the documentation, and guarantee
>> that blk_get_max_transfer() returns a non-zero value.  Use unsigned
>> values, so that we don't have to worry about negative values and
>> so that bit-twiddling is easier; however, we are still constrained
>> by 2^31 of signed int in most APIs.
>>
>> Of note: the iscsi calculations use a 64-bit number internally,
>> but the final result is guaranteed to fit in 32 bits.  NBD is
>> fixed to advertise the maximum length of 32M that the qemu and
>> kernel servers enforce, rather than a number of sectors that
>> would overflow int when converted back to bytes.  scsi-generic
>> now advertises a maximum always, rather than just when the
>> underlying device advertised a maximum.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
>> @@ -1177,7 +1176,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>
>>          if (ret == -ENOTSUP) {
>>              /* Fall back to bounce buffer if write zeroes is unsupported */
>> -            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
>> +            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer,
>>                                              MAX_WRITE_ZEROES_BOUNCE_BUFFER);
> 
> You could consider renaming the variable to max_transfer to keep it
> consistent with the BlockLimits field name and to make it even more
> obvious that you converted all uses.

Good idea.

>> @@ -1706,13 +1708,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>>       * iscsi_open(): iscsi targets don't change their limits. */
>>
>>      IscsiLun *iscsilun = bs->opaque;
>> -    uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
>> +    uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
>>
>>      if (iscsilun->bl.max_xfer_len) {
>>          max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
>>      }
>>
>> -    bs->bl.max_transfer_length = sector_limits_lun2qemu(max_xfer_len, iscsilun);
>> +    bs->bl.max_transfer = MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
>> +                              max_xfer_len * iscsilun->block_size);
> 
> Why don't you simply use 0 when you defined it as no limit?
> 
> If we make some drivers put 0 there and others INT_MAX, chances are that
> we'll introduce places where we fail to handle 0 correctly.

So if I'm understanding correctly, we want something like:

if (max_xfer_len * iscsilun->block_size > INT_MAX) {
    bs->bl.max_transfer = 0;
} else {
    bs->bl.max_transfer = max_xfer_len * iscsilun->block_size;
}

and make sure that 0 continues to mean 'no signed 32-bit limit'.

>> +++ b/block/nbd.c
>> @@ -363,7 +363,7 @@ static int nbd_co_flush(BlockDriverState *bs)
>>  static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
>>  {
>>      bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
>> -    bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS;
>> +    bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
>>  }
> 
> This introduces a semantic difference and should therefore be a separate
> patch. Here, it should become UINT32_MAX through mechanical conversion.
> 
> Or actually, it can't because that's not a power of two. So maybe have
> the NBD patch first...

Will split.  I don't see any problem with a max_transfer that is not a
power of two, as long as it is a multiple of request_alignment (iscsi is
a case in point - the max_transfer can be 0xffff blocks).

> 
>>  static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ce2e20f..f3bd5ef 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -752,7 +752,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>          if (S_ISBLK(st.st_mode)) {
>>              int ret = hdev_get_max_transfer_length(s->fd);
>>              if (ret >= 0) {
>> -                bs->bl.max_transfer_length = ret;
>> +                bs->bl.max_transfer = ret << BDRV_SECTOR_BITS;
> 
> I assume that we don't care about overflows here because in practice the
> values are very low? Should we check (or maybe better just cap) it
> anyway?

Will add a check.

>> @@ -391,8 +391,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>>          return;
>>      }
>>
>> -    max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
>> -    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
>> +    max_xfer_len = blk_get_max_transfer(mrb->reqs[0]->dev->blk);
>> +    assert(max_xfer_len &&
>> +           max_xfer_len >> BDRV_SECTOR_BITS <= BDRV_REQUEST_MAX_SECTORS);
> 
> Why can we assert this here? The comment for BlockLimits.max_xfer_len
> doesn't document any maximum. Of course, as I already said above, it
> might not happen in practice, but INT_MAX + 1 is theoretically valid and
> would fail the assertion.

As part of this patch, blk_get_max_transfer() guarantees that its result
is non-zero and no larger than INT_MAX.


>> +++ b/hw/scsi/scsi-generic.c
>> @@ -225,13 +225,13 @@ static void scsi_read_complete(void * opaque, int ret)
>>      if (s->type == TYPE_DISK &&
>>          r->req.cmd.buf[0] == INQUIRY &&
>>          r->req.cmd.buf[2] == 0xb0) {
>> -        uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk);
>> -        if (max_xfer_len) {
>> -            stl_be_p(&r->buf[8], max_xfer_len);
>> -            /* Also take care of the opt xfer len. */
>> -            if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
>> -                stl_be_p(&r->buf[12], max_xfer_len);
>> -            }
>> +        uint32_t max_xfer_len =
>> +            blk_get_max_transfer(s->conf.blk) >> BDRV_SECTOR_BITS;
>> +
>> +        stl_be_p(&r->buf[8], max_xfer_len);
>> +        /* Also take care of the opt xfer len. */
>> +        if (ldl_be_p(&r->buf[12]) > max_xfer_len) {
>> +            stl_be_p(&r->buf[12], max_xfer_len);
>>          }
>>      }
> 
> This is another hidden semantic change. Can we have a separate
> scsi-generic patch first that changes the handling for the
> max_transfer == 0 case and only then make the change in
> blk_get_max_transfer() as pure refactoring without a change in
> behaviour?

Will split.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3450 bytes --]

Am 12.06.2016 um 00:06 hat Eric Blake geschrieben:
> On 06/07/2016 06:45 AM, Kevin Wolf wrote:
> > Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> >> Sector-based limits are awkward to think about; in our on-going
> >> quest to move to byte-based interfaces, convert max_transfer_length
> >> and opt_transfer_length.  Rename them (dropping the _length suffix)
> >> so that the compiler will help us catch the change in semantics
> >> across any rebased code.  Improve the documentation, and guarantee
> >> that blk_get_max_transfer() returns a non-zero value.  Use unsigned
> >> values, so that we don't have to worry about negative values and
> >> so that bit-twiddling is easier; however, we are still constrained
> >> by 2^31 of signed int in most APIs.
> >>
> >> Of note: the iscsi calculations use a 64-bit number internally,
> >> but the final result is guaranteed to fit in 32 bits.  NBD is
> >> fixed to advertise the maximum length of 32M that the qemu and
> >> kernel servers enforce, rather than a number of sectors that
> >> would overflow int when converted back to bytes.  scsi-generic
> >> now advertises a maximum always, rather than just when the
> >> underlying device advertised a maximum.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> >> @@ -1177,7 +1176,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> >>
> >>          if (ret == -ENOTSUP) {
> >>              /* Fall back to bounce buffer if write zeroes is unsupported */
> >> -            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
> >> +            int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer,
> >>                                              MAX_WRITE_ZEROES_BOUNCE_BUFFER);
> > 
> > You could consider renaming the variable to max_transfer to keep it
> > consistent with the BlockLimits field name and to make it even more
> > obvious that you converted all uses.
> 
> Good idea.
> 
> >> @@ -1706,13 +1708,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
> >>       * iscsi_open(): iscsi targets don't change their limits. */
> >>
> >>      IscsiLun *iscsilun = bs->opaque;
> >> -    uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
> >> +    uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff;
> >>
> >>      if (iscsilun->bl.max_xfer_len) {
> >>          max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
> >>      }
> >>
> >> -    bs->bl.max_transfer_length = sector_limits_lun2qemu(max_xfer_len, iscsilun);
> >> +    bs->bl.max_transfer = MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
> >> +                              max_xfer_len * iscsilun->block_size);
> > 
> > Why don't you simply use 0 when you defined it as no limit?
> > 
> > If we make some drivers put 0 there and others INT_MAX, chances are that
> > we'll introduce places where we fail to handle 0 correctly.
> 
> So if I'm understanding correctly, we want something like:
> 
> if (max_xfer_len * iscsilun->block_size > INT_MAX) {
>     bs->bl.max_transfer = 0;
> } else {
>     bs->bl.max_transfer = max_xfer_len * iscsilun->block_size;
> }
> 
> and make sure that 0 continues to mean 'no signed 32-bit limit'.

Forget that, brain fart. Somehow I was thinking that just doing the
assignment without MIN() would do the right thing. Which it very
obviously doesn't.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH 4/5] block: Switch discard length bounds to byte-based

[Eric Blake]

Sector-based limits are awkward to think about; in our on-going
quest to move to byte-based interfaces, convert max_discard and
discard_alignment.  Rename them, using 'pdiscard' as an aid to
track which remaining discard interfaces need conversion, and so
that the compiler will help us catch the change in semantics
across any rebased code.  In iscsi.c, sector_limits_lun2qemu()
is no longer needed; and the BlockLimits type is now completely
byte-based.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h | 21 +++++++++++++++------
 block/io.c                | 16 +++++++++-------
 block/iscsi.c             | 17 ++++++-----------
 block/nbd.c               |  2 +-
 qemu-img.c                |  3 ++-
 5 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 96e8476..9dc7af4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -322,18 +322,27 @@ struct BlockDriver {
 };

 typedef struct BlockLimits {
-    /* maximum number of sectors that can be discarded at once */
-    int max_discard;
+    /* maximum number of bytes that can be discarded at once (since it
+     * is signed, it must be < 2G, if set), should be multiple of
+     * pdiscard_alignment, but need not be power of 2. May be 0 if no
+     * inherent 32-bit limit */
+    int32_t max_pdiscard;

-    /* optimal alignment for discard requests in sectors */
-    int64_t discard_alignment;
+    /* optimal alignment for discard requests in bytes, must be power
+     * of 2, less than max_discard if that is set, and multiple of
+     * bs->request_alignment. May be 0 if bs->request_alignment is
+     * good enough */
+    uint32_t pdiscard_alignment;

     /* maximum number of bytes that can zeroized at once (since it is
-     * signed, it must be < 2G, if set) */
+     * signed, it must be < 2G, if set), should be multiple of
+     * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
     int32_t max_pwrite_zeroes;

     /* optimal alignment for write zeroes requests in bytes, must be
-     * power of 2, and less than max_pwrite_zeroes if that is set */
+     * power of 2, less than max_pwrite_zeroes if that is set, and
+     * multiple of bs->request_alignment. May be 0 if
+     * bs->request_alignment is good enough */
     uint32_t pwrite_zeroes_alignment;

     /* optimal transfer length in bytes (must be power of 2, and
diff --git a/block/io.c b/block/io.c
index a2bc254..e845ef9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2255,19 +2255,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                           BDRV_TRACKED_DISCARD);
     bdrv_set_dirty(bs, sector_num, nb_sectors);

-    max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
+    max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS,
+                               BDRV_REQUEST_MAX_SECTORS);
     while (nb_sectors > 0) {
         int ret;
         int num = nb_sectors;
+        int discard_alignment = bs->bl.pdiscard_alignment >> BDRV_SECTOR_BITS;

         /* align request */
-        if (bs->bl.discard_alignment &&
-            num >= bs->bl.discard_alignment &&
-            sector_num % bs->bl.discard_alignment) {
-            if (num > bs->bl.discard_alignment) {
-                num = bs->bl.discard_alignment;
+        if (discard_alignment &&
+            num >= discard_alignment &&
+            sector_num % discard_alignment) {
+            if (num > discard_alignment) {
+                num = discard_alignment;
             }
-            num -= sector_num % bs->bl.discard_alignment;
+            num -= sector_num % discard_alignment;
         }

         /* limit request size */
diff --git a/block/iscsi.c b/block/iscsi.c
index c5855be..1486443 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1697,11 +1697,6 @@ static void iscsi_close(BlockDriverState *bs)
     memset(iscsilun, 0, sizeof(IscsiLun));
 }

-static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun)
-{
-    return MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1);
-}
-
 static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     /* We don't actually refresh here, but just return data queried in
@@ -1718,14 +1713,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
                               max_xfer_len * iscsilun->block_size);

     if (iscsilun->lbp.lbpu) {
-        if (iscsilun->bl.max_unmap < 0xffffffff) {
-            bs->bl.max_discard =
-                sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun);
+        if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) {
+            bs->bl.max_pdiscard =
+                iscsilun->bl.max_unmap * iscsilun->block_size;
         }
-        bs->bl.discard_alignment =
-            sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
+        bs->bl.pdiscard_alignment =
+            iscsilun->bl.opt_unmap_gran * iscsilun->block_size;
     } else {
-        bs->bl.discard_alignment = iscsilun->block_size >> BDRV_SECTOR_BITS;
+        bs->bl.pdiscard_alignment = iscsilun->block_size;
     }

     if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) {
diff --git a/block/nbd.c b/block/nbd.c
index 2ce7b4d..a3de9bc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -362,7 +362,7 @@ static int nbd_co_flush(BlockDriverState *bs)

 static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
 {
-    bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
+    bs->bl.max_pdiscard = INT32_MAX;
     bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
 }

diff --git a/qemu-img.c b/qemu-img.c
index 577df0d..26500dc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2080,7 +2080,8 @@ static int img_convert(int argc, char **argv)
     bufsectors = MIN(32768,
                      MAX(bufsectors,
                          MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
-                             out_bs->bl.discard_alignment)));
+                             out_bs->bl.pdiscard_alignment >>
+                             BDRV_SECTOR_BITS)));

     if (skip_create) {
         int64_t output_sectors = blk_nb_sectors(out_blk);
-- 
2.5.5

Re: [Qemu-devel] [PATCH 4/5] block: Switch discard length bounds to byte-based

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_discard and
> discard_alignment.  Rename them, using 'pdiscard' as an aid to
> track which remaining discard interfaces need conversion, and so
> that the compiler will help us catch the change in semantics
> across any rebased code.  In iscsi.c, sector_limits_lun2qemu()
> is no longer needed; and the BlockLimits type is now completely
> byte-based.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

> diff --git a/block/nbd.c b/block/nbd.c
> index 2ce7b4d..a3de9bc 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -362,7 +362,7 @@ static int nbd_co_flush(BlockDriverState *bs)
> 
>  static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
> -    bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS;
> +    bs->bl.max_pdiscard = INT32_MAX;
>      bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
>  }

Another non-mechanical change that might deserve its own patch (or
probably one NBD patch that changes both values).

Kevin

[Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake]

It makes more sense to have ALL block size limit constraints
in the same struct.  Improve the documentation while at it.

Note that bdrv_refresh_limits() has to keep things alive across
a memset() of BlockLimits.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h | 12 ++++++++----
 block.c                   |  4 ++--
 block/blkdebug.c          |  4 ++--
 block/bochs.c             |  2 +-
 block/cloop.c             |  2 +-
 block/dmg.c               |  2 +-
 block/io.c                | 12 +++++++-----
 block/iscsi.c             |  2 +-
 block/raw-posix.c         | 16 ++++++++--------
 block/raw-win32.c         |  6 +++---
 block/vvfat.c             |  2 +-
 11 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9dc7af4..0bc7ff4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -322,6 +322,12 @@ struct BlockDriver {
 };

 typedef struct BlockLimits {
+    /* Alignment requirement, in bytes, for offset/length of I/O
+     * requests. Must be a power of 2, and should not exceed
+     * INT_MAX. For now, a value of 0 defaults to 512, but may change
+     * to default to 1. */
+    uint32_t request_alignment;
+
     /* maximum number of bytes that can be discarded at once (since it
      * is signed, it must be < 2G, if set), should be multiple of
      * pdiscard_alignment, but need not be power of 2. May be 0 if no
@@ -353,10 +359,10 @@ typedef struct BlockLimits {
      * should be multiple of opt_transfer), or 0 for no 32-bit limit */
     uint32_t max_transfer;

-    /* memory alignment so that no bounce buffer is needed */
+    /* memory alignment, in bytes so that no bounce buffer is needed */
     size_t min_mem_alignment;

-    /* memory alignment for bounce buffer */
+    /* memory alignment, in bytes, for bounce buffer */
     size_t opt_mem_alignment;

     /* maximum number of iovec elements */
@@ -463,8 +469,6 @@ struct BlockDriverState {
     /* Whether produces zeros when read beyond eof */
     bool zero_beyond_eof;

-    /* Alignment requirement for offset/length of I/O requests */
-    unsigned int request_alignment;
     /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
     unsigned int supported_write_flags;
     /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
diff --git a/block.c b/block.c
index f54bc25..1b56161 100644
--- a/block.c
+++ b/block.c
@@ -937,7 +937,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto fail_opts;
     }

-    bs->request_alignment = 512;
+    bs->bl.request_alignment = 512;
     bs->zero_beyond_eof = true;
     bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

@@ -1018,7 +1018,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,

     assert(bdrv_opt_mem_align(bs) != 0);
     assert(bdrv_min_mem_align(bs) != 0);
-    assert(is_power_of_2(bs->request_alignment) || bdrv_is_sg(bs));
+    assert(is_power_of_2(bs->bl.request_alignment) || bdrv_is_sg(bs));

     qemu_opts_del(opts);
     return 0;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 20d25bd..d9bf3ff 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -382,9 +382,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     }

     /* Set request alignment */
-    align = qemu_opt_get_size(opts, "align", bs->request_alignment);
+    align = qemu_opt_get_size(opts, "align", bs->bl.request_alignment);
     if (align > 0 && align < INT_MAX && !(align & (align - 1))) {
-        bs->request_alignment = align;
+        bs->bl.request_alignment = align;
     } else {
         error_setg(errp, "Invalid alignment");
         ret = -EINVAL;
diff --git a/block/bochs.c b/block/bochs.c
index 6c8d0f3..7bf20f5 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,7 +105,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;

     bs->read_only = 1; // no write support yet
-    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */

     ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index ea5a92b..71237f7 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,7 +67,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;

     bs->read_only = 1;
-    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */

     /* read header */
     ret = bdrv_pread(bs->file->bs, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index 1ea5f22..ef2eeb4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -440,7 +440,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;

     bs->read_only = 1;
-    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */

     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/io.c b/block/io.c
index e845ef9..aaeedaa 100644
--- a/block/io.c
+++ b/block/io.c
@@ -71,8 +71,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     Error *local_err = NULL;
+    uint32_t request_alignment = bs->bl.request_alignment;

     memset(&bs->bl, 0, sizeof(bs->bl));
+    bs->bl.request_alignment = request_alignment;

     if (!drv) {
         return;
@@ -429,7 +431,7 @@ static int bdrv_get_cluster_size(BlockDriverState *bs)

     ret = bdrv_get_info(bs, &bdi);
     if (ret < 0 || bdi.cluster_size == 0) {
-        return bs->request_alignment;
+        return bs->bl.request_alignment;
     } else {
         return bdi.cluster_size;
     }
@@ -1036,7 +1038,7 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
     BdrvTrackedRequest req;

     /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->bl.request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
@@ -1133,7 +1135,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,

     int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
     int alignment = MAX(bs->bl.pwrite_zeroes_alignment ?: 1,
-                        bs->request_alignment);
+                        bs->bl.request_alignment);

     assert(is_power_of_2(alignment));
     head = offset & (alignment - 1);
@@ -1291,7 +1293,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
     uint8_t *buf = NULL;
     QEMUIOVector local_qiov;
     struct iovec iov;
-    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->bl.request_alignment);
     unsigned int head_padding_bytes, tail_padding_bytes;
     int ret = 0;

@@ -1379,7 +1381,7 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
 {
     BdrvTrackedRequest req;
     /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->bl.request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
diff --git a/block/iscsi.c b/block/iscsi.c
index 1486443..baa3e15 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1590,7 +1590,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
-    bs->request_alignment = iscsilun->block_size;
+    bs->bl.request_alignment = iscsilun->block_size;

     /* We don't have any emulation for devices other than disks and CD-ROMs, so
      * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f3bd5ef..10a3957 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -302,22 +302,22 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     /* For SCSI generic devices the alignment is not really used.
        With buffered I/O, we don't have any restrictions. */
     if (bdrv_is_sg(bs) || !s->needs_alignment) {
-        bs->request_alignment = 1;
+        bs->bl.request_alignment = 1;
         s->buf_align = 1;
         return;
     }

-    bs->request_alignment = 0;
+    bs->bl.request_alignment = 0;
     s->buf_align = 0;
     /* Let's try to use the logical blocksize for the alignment. */
-    if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
-        bs->request_alignment = 0;
+    if (probe_logical_blocksize(fd, &bs->bl.request_alignment) < 0) {
+        bs->bl.request_alignment = 0;
     }
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
         struct dioattr da;
         if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
-            bs->request_alignment = da.d_miniosz;
+            bs->bl.request_alignment = da.d_miniosz;
             /* The kernel returns wrong information for d_mem */
             /* s->buf_align = da.d_mem; */
         }
@@ -337,19 +337,19 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
         qemu_vfree(buf);
     }

-    if (!bs->request_alignment) {
+    if (!bs->bl.request_alignment) {
         size_t align;
         buf = qemu_memalign(s->buf_align, max_align);
         for (align = 512; align <= max_align; align <<= 1) {
             if (raw_is_io_aligned(fd, buf, align)) {
-                bs->request_alignment = align;
+                bs->bl.request_alignment = align;
                 break;
             }
         }
         qemu_vfree(buf);
     }

-    if (!s->buf_align || !bs->request_alignment) {
+    if (!s->buf_align || !bs->bl.request_alignment) {
         error_setg(errp, "Could not find working O_DIRECT alignment. "
                          "Try cache.direct=off.");
     }
diff --git a/block/raw-win32.c b/block/raw-win32.c
index fd23891..7afdb10 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -230,14 +230,14 @@ static void raw_probe_alignment(BlockDriverState *bs)
     BOOL status;

     if (s->type == FTYPE_CD) {
-        bs->request_alignment = 2048;
+        bs->bl.request_alignment = 2048;
         return;
     }
     if (s->type == FTYPE_HARDDISK) {
         status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
                                  NULL, 0, &dg, sizeof(dg), &count, NULL);
         if (status != 0) {
-            bs->request_alignment = dg.Geometry.BytesPerSector;
+            bs->bl.request_alignment = dg.Geometry.BytesPerSector;
             return;
         }
         /* try GetDiskFreeSpace too */
@@ -247,7 +247,7 @@ static void raw_probe_alignment(BlockDriverState *bs)
         GetDiskFreeSpace(s->drive_path, &sectorsPerCluster,
                          &dg.Geometry.BytesPerSector,
                          &freeClusters, &totalClusters);
-        bs->request_alignment = dg.Geometry.BytesPerSector;
+        bs->bl.request_alignment = dg.Geometry.BytesPerSector;
     }
 }

diff --git a/block/vvfat.c b/block/vvfat.c
index a39dbe6..aed186d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1180,7 +1180,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         bs->read_only = 0;
     }

-    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
+    bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */
     bs->total_sectors = cyls * heads * secs;

     if (init_directories(s, dirname, heads, secs, errp)) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On 06/03/2016 11:03 AM, Eric Blake wrote:
> It makes more sense to have ALL block size limit constraints
> in the same struct.  Improve the documentation while at it.
> 
> Note that bdrv_refresh_limits() has to keep things alive across
> a memset() of BlockLimits.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block_int.h | 12 ++++++++----
>  block.c                   |  4 ++--
>  block/blkdebug.c          |  4 ++--
>  block/bochs.c             |  2 +-
>  block/cloop.c             |  2 +-
>  block/dmg.c               |  2 +-
>  block/io.c                | 12 +++++++-----
>  block/iscsi.c             |  2 +-
>  block/raw-posix.c         | 16 ++++++++--------
>  block/raw-win32.c         |  6 +++---
>  block/vvfat.c             |  2 +-
>  11 files changed, 35 insertions(+), 29 deletions(-)

Something in this patch is causing qemu-iotests 77 to infloop; we may
decide it is just easier to drop this patch rather than find all the
places where the request_alignment must be preserved across what
otherwise zeroes out limits.

> +++ b/block/io.c
> @@ -71,8 +71,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      Error *local_err = NULL;
> +    uint32_t request_alignment = bs->bl.request_alignment;
> 
>      memset(&bs->bl, 0, sizeof(bs->bl));
> +    bs->bl.request_alignment = request_alignment;

Or put another way, it looks like I missed another case where moving the
scope of the variable can impact who is expecting the value to remain
unchanged across certain operations.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On 06/03/2016 11:49 AM, Eric Blake wrote:
> On 06/03/2016 11:03 AM, Eric Blake wrote:
>> It makes more sense to have ALL block size limit constraints
>> in the same struct.  Improve the documentation while at it.
>>
>> Note that bdrv_refresh_limits() has to keep things alive across
>> a memset() of BlockLimits.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  include/block/block_int.h | 12 ++++++++----
>>  block.c                   |  4 ++--
>>  block/blkdebug.c          |  4 ++--
>>  block/bochs.c             |  2 +-
>>  block/cloop.c             |  2 +-
>>  block/dmg.c               |  2 +-
>>  block/io.c                | 12 +++++++-----
>>  block/iscsi.c             |  2 +-
>>  block/raw-posix.c         | 16 ++++++++--------
>>  block/raw-win32.c         |  6 +++---
>>  block/vvfat.c             |  2 +-
>>  11 files changed, 35 insertions(+), 29 deletions(-)
> 
> Something in this patch is causing qemu-iotests 77 to infloop; we may
> decide it is just easier to drop this patch rather than find all the
> places where the request_alignment must be preserved across what
> otherwise zeroes out limits.

Found it; squash this in (or use it as an argument why we don't want
request_alignment in bs->bl after all):

diff --git i/block/raw_bsd.c w/block/raw_bsd.c
index b1d5237..c3c2246 100644
--- i/block/raw_bsd.c
+++ w/block/raw_bsd.c
@@ -152,7 +152,11 @@ static int raw_get_info(BlockDriverState *bs,
BlockDriverInfo *bdi)

 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
+    /* Inherit all limits except for request_alignment */
+    int request_alignment = bs->bl.request_alignment;
+
     bs->bl = bs->file->bs->bl;
+    bs->bl.request_alignment = request_alignment;
 }

 static int raw_truncate(BlockDriverState *bs, int64_t offset)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

Am 03.06.2016 um 23:43 hat Eric Blake geschrieben:
> On 06/03/2016 11:49 AM, Eric Blake wrote:
> > On 06/03/2016 11:03 AM, Eric Blake wrote:
> >> It makes more sense to have ALL block size limit constraints
> >> in the same struct.  Improve the documentation while at it.
> >>
> >> Note that bdrv_refresh_limits() has to keep things alive across
> >> a memset() of BlockLimits.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  include/block/block_int.h | 12 ++++++++----
> >>  block.c                   |  4 ++--
> >>  block/blkdebug.c          |  4 ++--
> >>  block/bochs.c             |  2 +-
> >>  block/cloop.c             |  2 +-
> >>  block/dmg.c               |  2 +-
> >>  block/io.c                | 12 +++++++-----
> >>  block/iscsi.c             |  2 +-
> >>  block/raw-posix.c         | 16 ++++++++--------
> >>  block/raw-win32.c         |  6 +++---
> >>  block/vvfat.c             |  2 +-
> >>  11 files changed, 35 insertions(+), 29 deletions(-)
> > 
> > Something in this patch is causing qemu-iotests 77 to infloop; we may
> > decide it is just easier to drop this patch rather than find all the
> > places where the request_alignment must be preserved across what
> > otherwise zeroes out limits.
> 
> Found it; squash this in (or use it as an argument why we don't want
> request_alignment in bs->bl after all):

This hunk doesn't make sense to me. For the correctness of the code it
shouldn't make a difference whether the alignment happens before passing
the request to file/raw-posix or already in the raw format layer.

The cause for the hang you're seeing is probably that the request is
already aligned before the blkdebug layer and therefore the blkdebug
events aren't generated any more. That's a problem with the test (I'm
considering the blkdebug events part of the test infrastructure),
however, and not with the code.

Kevin

> diff --git i/block/raw_bsd.c w/block/raw_bsd.c
> index b1d5237..c3c2246 100644
> --- i/block/raw_bsd.c
> +++ w/block/raw_bsd.c
> @@ -152,7 +152,11 @@ static int raw_get_info(BlockDriverState *bs,
> BlockDriverInfo *bdi)
> 
>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
> +    /* Inherit all limits except for request_alignment */
> +    int request_alignment = bs->bl.request_alignment;
> +
>      bs->bl = bs->file->bs->bl;
> +    bs->bl.request_alignment = request_alignment;
>  }
> 
>  static int raw_truncate(BlockDriverState *bs, int64_t offset)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Paolo Bonzini]

On 07/06/2016 12:08, Kevin Wolf wrote:
>>> > > Something in this patch is causing qemu-iotests 77 to infloop; we may
>>> > > decide it is just easier to drop this patch rather than find all the
>>> > > places where the request_alignment must be preserved across what
>>> > > otherwise zeroes out limits.
>> > 
>> > Found it; squash this in (or use it as an argument why we don't want
>> > request_alignment in bs->bl after all):
> This hunk doesn't make sense to me. For the correctness of the code it
> shouldn't make a difference whether the alignment happens before passing
> the request to file/raw-posix or already in the raw format layer.
> 
> The cause for the hang you're seeing is probably that the request is
> already aligned before the blkdebug layer and therefore the blkdebug
> events aren't generated any more. That's a problem with the test (I'm
> considering the blkdebug events part of the test infrastructure),
> however, and not with the code.

Perhaps you could add an alignment option to blkdebug (or in general
options to force block limits on blkdebug), which would help testing in
general?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

Am 07.06.2016 um 13:04 hat Paolo Bonzini geschrieben:
> On 07/06/2016 12:08, Kevin Wolf wrote:
> >>> > > Something in this patch is causing qemu-iotests 77 to infloop; we may
> >>> > > decide it is just easier to drop this patch rather than find all the
> >>> > > places where the request_alignment must be preserved across what
> >>> > > otherwise zeroes out limits.
> >> > 
> >> > Found it; squash this in (or use it as an argument why we don't want
> >> > request_alignment in bs->bl after all):
> > This hunk doesn't make sense to me. For the correctness of the code it
> > shouldn't make a difference whether the alignment happens before passing
> > the request to file/raw-posix or already in the raw format layer.
> > 
> > The cause for the hang you're seeing is probably that the request is
> > already aligned before the blkdebug layer and therefore the blkdebug
> > events aren't generated any more. That's a problem with the test (I'm
> > considering the blkdebug events part of the test infrastructure),
> > however, and not with the code.
> 
> Perhaps you could add an alignment option to blkdebug (or in general
> options to force block limits on blkdebug), which would help testing in
> general?

It exists and is exactly what this test uses.

The problem is just that the raw format driver inherits the alignment,
so the RMW cycle that we want to test with blkdebug breakpoints happens
too early and the blkdebug layer (more precisely, the block/io.c
functions for the blkdebug BDS before they call into the driver) already
sees aligned requests and we don't get the events any more that the test
is looking for.

Kevin

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

On 06/07/2016 04:08 AM, Kevin Wolf wrote:

>> Found it; squash this in (or use it as an argument why we don't want
>> request_alignment in bs->bl after all):
> 
> This hunk doesn't make sense to me. For the correctness of the code it
> shouldn't make a difference whether the alignment happens before passing
> the request to file/raw-posix or already in the raw format layer.
> 
> The cause for the hang you're seeing is probably that the request is
> already aligned before the blkdebug layer and therefore the blkdebug
> events aren't generated any more. That's a problem with the test (I'm
> considering the blkdebug events part of the test infrastructure),
> however, and not with the code.
> 

Yes, it's definitely a hang caused by the test expecting an unalignment
event, but the inheritance chain now causes things to be aligned to
begin with and nothing unaligned happens after all.

> Kevin
> 
>> diff --git i/block/raw_bsd.c w/block/raw_bsd.c
>> index b1d5237..c3c2246 100644
>> --- i/block/raw_bsd.c
>> +++ w/block/raw_bsd.c
>> @@ -152,7 +152,11 @@ static int raw_get_info(BlockDriverState *bs,
>> BlockDriverInfo *bdi)
>>
>>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>  {
>> +    /* Inherit all limits except for request_alignment */
>> +    int request_alignment = bs->bl.request_alignment;
>> +
>>      bs->bl = bs->file->bs->bl;
>> +    bs->bl.request_alignment = request_alignment;

Any ideas on how to fix the test, then?  Have two blkdebug devices
nested atop one another, since those are the devices where we can
explicitly override alignment? (normally, you'd _expect_ the chain to
inherit the worst-case alignment of all BDS in the chain, so blkdebug is
the way around it).

That's the only thing left before I repost the series, so I may just
post the last patch as RFC and play with it a bit more...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3484 bytes --]

Am 14.06.2016 um 06:39 hat Eric Blake geschrieben:
> On 06/07/2016 04:08 AM, Kevin Wolf wrote:
> 
> >> Found it; squash this in (or use it as an argument why we don't want
> >> request_alignment in bs->bl after all):
> > 
> > This hunk doesn't make sense to me. For the correctness of the code it
> > shouldn't make a difference whether the alignment happens before passing
> > the request to file/raw-posix or already in the raw format layer.
> > 
> > The cause for the hang you're seeing is probably that the request is
> > already aligned before the blkdebug layer and therefore the blkdebug
> > events aren't generated any more. That's a problem with the test (I'm
> > considering the blkdebug events part of the test infrastructure),
> > however, and not with the code.
> > 
> 
> Yes, it's definitely a hang caused by the test expecting an unalignment
> event, but the inheritance chain now causes things to be aligned to
> begin with and nothing unaligned happens after all.
> 
> > Kevin
> > 
> >> diff --git i/block/raw_bsd.c w/block/raw_bsd.c
> >> index b1d5237..c3c2246 100644
> >> --- i/block/raw_bsd.c
> >> +++ w/block/raw_bsd.c
> >> @@ -152,7 +152,11 @@ static int raw_get_info(BlockDriverState *bs,
> >> BlockDriverInfo *bdi)
> >>
> >>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >>  {
> >> +    /* Inherit all limits except for request_alignment */
> >> +    int request_alignment = bs->bl.request_alignment;
> >> +
> >>      bs->bl = bs->file->bs->bl;
> >> +    bs->bl.request_alignment = request_alignment;
> 
> Any ideas on how to fix the test, then?  Have two blkdebug devices
> nested atop one another, since those are the devices where we can
> explicitly override alignment?

Interesting idea. Maybe that's a good option if it works.

> (normally, you'd _expect_ the chain to
> inherit the worst-case alignment of all BDS in the chain, so blkdebug is
> the way around it).

Actually, I think there are two cases.

The first one is that you get a request and want to know what to do with
it. Here you don't want to inherit the worst-case alignment. You'd
rather want to enforce alignment only when it is actually needed. For
example, if you have a cache=none backing file with 4k alignment, but a
cache=writeback overlay, and you don't actually access the backing file
with this request, it would be wasteful to align the request. You also
don't really know that a driver will issue a misaligned request (or any
request at all) to the lower layer just because it got called with one.

The second case is when you want to issue a request. For example, let's
take the qcow2 cache here, which has 64k cached and needs to update a
few bytes. Currently it always writes the full 64k (and with 1 MB
clusters a full megabyte), but what it really should do is consider the
worst-case alignment.

This is comparable to the difference between bdrv_opt_mem_align(), which
is used in qemu_blockalign() where we want to create a buffer that works
even in the worst case, and bdrv_min_mem_align(), which is used in
bdrv_qiov_is_aligned() in order to determine whether we must align an
existing request.

> That's the only thing left before I repost the series, so I may just
> post the last patch as RFC and play with it a bit more...

And in the light of the above, maybe the solution is as easy as changing
raw_refresh_limits() to set bs->bl.request_alignment = 1.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

On 06/14/2016 02:05 AM, Kevin Wolf wrote:

>>>>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>>>  {
>>>> +    /* Inherit all limits except for request_alignment */
>>>> +    int request_alignment = bs->bl.request_alignment;
>>>> +
>>>>      bs->bl = bs->file->bs->bl;
>>>> +    bs->bl.request_alignment = request_alignment;
>>
>> Any ideas on how to fix the test, then?  Have two blkdebug devices
>> nested atop one another, since those are the devices where we can
>> explicitly override alignment?
> 
> Interesting idea. Maybe that's a good option if it works.
> 
>> (normally, you'd _expect_ the chain to
>> inherit the worst-case alignment of all BDS in the chain, so blkdebug is
>> the way around it).
> 
> Actually, I think there are two cases.
> 
> The first one is that you get a request and want to know what to do with
> it. Here you don't want to inherit the worst-case alignment. You'd
> rather want to enforce alignment only when it is actually needed. For
> example, if you have a cache=none backing file with 4k alignment, but a
> cache=writeback overlay, and you don't actually access the backing file
> with this request, it would be wasteful to align the request. You also
> don't really know that a driver will issue a misaligned request (or any
> request at all) to the lower layer just because it got called with one.
> 
> The second case is when you want to issue a request. For example, let's
> take the qcow2 cache here, which has 64k cached and needs to update a
> few bytes. Currently it always writes the full 64k (and with 1 MB
> clusters a full megabyte), but what it really should do is consider the
> worst-case alignment.
> 
> This is comparable to the difference between bdrv_opt_mem_align(), which
> is used in qemu_blockalign() where we want to create a buffer that works
> even in the worst case, and bdrv_min_mem_align(), which is used in
> bdrv_qiov_is_aligned() in order to determine whether we must align an
> existing request.
> 
>> That's the only thing left before I repost the series, so I may just
>> post the last patch as RFC and play with it a bit more...
> 
> And in the light of the above, maybe the solution is as easy as changing
> raw_refresh_limits() to set bs->bl.request_alignment = 1.

Or maybe split the main bdrv_refresh_limits() into two pieces: one that
merges limits from one BDS into another (the limits that are worth
merging, and in the correct direction: opt merges to MAX, max merges to
MIN_NON_ZERO, request_alignment is not merged), the other that calls
merge(bs, bs->file->bs); then have raw_refresh_limits() also use the
common merge functionality rather than straight copy.  Testing that
approach now...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

Am 14.06.2016 um 16:47 hat Eric Blake geschrieben:
> On 06/14/2016 02:05 AM, Kevin Wolf wrote:
> 
> >>>>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >>>>  {
> >>>> +    /* Inherit all limits except for request_alignment */
> >>>> +    int request_alignment = bs->bl.request_alignment;
> >>>> +
> >>>>      bs->bl = bs->file->bs->bl;
> >>>> +    bs->bl.request_alignment = request_alignment;
> >>
> >> Any ideas on how to fix the test, then?  Have two blkdebug devices
> >> nested atop one another, since those are the devices where we can
> >> explicitly override alignment?
> > 
> > Interesting idea. Maybe that's a good option if it works.
> > 
> >> (normally, you'd _expect_ the chain to
> >> inherit the worst-case alignment of all BDS in the chain, so blkdebug is
> >> the way around it).
> > 
> > Actually, I think there are two cases.
> > 
> > The first one is that you get a request and want to know what to do with
> > it. Here you don't want to inherit the worst-case alignment. You'd
> > rather want to enforce alignment only when it is actually needed. For
> > example, if you have a cache=none backing file with 4k alignment, but a
> > cache=writeback overlay, and you don't actually access the backing file
> > with this request, it would be wasteful to align the request. You also
> > don't really know that a driver will issue a misaligned request (or any
> > request at all) to the lower layer just because it got called with one.
> > 
> > The second case is when you want to issue a request. For example, let's
> > take the qcow2 cache here, which has 64k cached and needs to update a
> > few bytes. Currently it always writes the full 64k (and with 1 MB
> > clusters a full megabyte), but what it really should do is consider the
> > worst-case alignment.
> > 
> > This is comparable to the difference between bdrv_opt_mem_align(), which
> > is used in qemu_blockalign() where we want to create a buffer that works
> > even in the worst case, and bdrv_min_mem_align(), which is used in
> > bdrv_qiov_is_aligned() in order to determine whether we must align an
> > existing request.
> > 
> >> That's the only thing left before I repost the series, so I may just
> >> post the last patch as RFC and play with it a bit more...
> > 
> > And in the light of the above, maybe the solution is as easy as changing
> > raw_refresh_limits() to set bs->bl.request_alignment = 1.
> 
> Or maybe split the main bdrv_refresh_limits() into two pieces: one that
> merges limits from one BDS into another (the limits that are worth
> merging, and in the correct direction: opt merges to MAX, max merges to
> MIN_NON_ZERO, request_alignment is not merged), the other that calls
> merge(bs, bs->file->bs); then have raw_refresh_limits() also use the
> common merge functionality rather than straight copy.  Testing that
> approach now...

So you don't agree with what I said above?

I think that block drivers should only set limits that they require for
themselves. The block layer bdrv_refresh_limits() function can then
merge things where needed; drivers shouldn't be involved there.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> It makes more sense to have ALL block size limit constraints
> in the same struct.  Improve the documentation while at it.
> 
> Note that bdrv_refresh_limits() has to keep things alive across
> a memset() of BlockLimits.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block_int.h | 12 ++++++++----
>  block.c                   |  4 ++--
>  block/blkdebug.c          |  4 ++--
>  block/bochs.c             |  2 +-
>  block/cloop.c             |  2 +-
>  block/dmg.c               |  2 +-
>  block/io.c                | 12 +++++++-----
>  block/iscsi.c             |  2 +-
>  block/raw-posix.c         | 16 ++++++++--------
>  block/raw-win32.c         |  6 +++---
>  block/vvfat.c             |  2 +-
>  11 files changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9dc7af4..0bc7ff4 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -322,6 +322,12 @@ struct BlockDriver {
>  };
> 
>  typedef struct BlockLimits {
> +    /* Alignment requirement, in bytes, for offset/length of I/O
> +     * requests. Must be a power of 2, and should not exceed
> +     * INT_MAX. For now, a value of 0 defaults to 512, but may change
> +     * to default to 1. */
> +    uint32_t request_alignment;
> +
>      /* maximum number of bytes that can be discarded at once (since it
>       * is signed, it must be < 2G, if set), should be multiple of
>       * pdiscard_alignment, but need not be power of 2. May be 0 if no
> @@ -353,10 +359,10 @@ typedef struct BlockLimits {
>       * should be multiple of opt_transfer), or 0 for no 32-bit limit */
>      uint32_t max_transfer;
> 
> -    /* memory alignment so that no bounce buffer is needed */
> +    /* memory alignment, in bytes so that no bounce buffer is needed */
>      size_t min_mem_alignment;
> 
> -    /* memory alignment for bounce buffer */
> +    /* memory alignment, in bytes, for bounce buffer */
>      size_t opt_mem_alignment;
> 
>      /* maximum number of iovec elements */
> @@ -463,8 +469,6 @@ struct BlockDriverState {
>      /* Whether produces zeros when read beyond eof */
>      bool zero_beyond_eof;
> 
> -    /* Alignment requirement for offset/length of I/O requests */
> -    unsigned int request_alignment;
>      /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
>      unsigned int supported_write_flags;
>      /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
> diff --git a/block.c b/block.c
> index f54bc25..1b56161 100644
> --- a/block.c
> +++ b/block.c
> @@ -937,7 +937,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
>          goto fail_opts;
>      }
> 
> -    bs->request_alignment = 512;
> +    bs->bl.request_alignment = 512;
>      bs->zero_beyond_eof = true;
>      bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

We don't explicitly initialise any other BlockLimits here, the function
calls bdrv_refresh_limits() to do that.

> diff --git a/block/io.c b/block/io.c
> index e845ef9..aaeedaa 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -71,8 +71,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      Error *local_err = NULL;
> +    uint32_t request_alignment = bs->bl.request_alignment;
> 
>      memset(&bs->bl, 0, sizeof(bs->bl));
> +    bs->bl.request_alignment = request_alignment;

This looks fishy, and it's because you didn't fully convert
request_alignment to work like all the other limits.

You should set the default of 512 here in bdrv_refresh_limits() before
calling into drv->bdrv_refresh_limits() where it can be overridden. No
other fields are set earlier and then preserved here, we start over with
everything.

Of course, this means that drivers that set request_alignment must be
changed so that they don't set it in .bdrv_open(), but only in their
.bdrv_refresh_limits() callback.

Kevin

[Qemu-devel] [PATCH 6/5] block: Fix harmless off-by-one in bdrv_aligned_preadv()

[Eric Blake]

If the amount of data to read ends exactly on the total size
of the bs, then we were wasting time creating a local qiov
to read the data in preparation for what would normally be
appending zeroes beyond the end, even though this corner case
has nothing further to do.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index aaeedaa..c8b323e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -996,7 +996,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,

         max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
                                   align >> BDRV_SECTOR_BITS);
-        if (nb_sectors < max_nb_sectors) {
+        if (nb_sectors <= max_nb_sectors) {
             ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
         } else if (max_nb_sectors > 0) {
             QEMUIOVector local_qiov;
-- 
2.5.5

Re: [Qemu-devel] [PATCH 6/5] block: Fix harmless off-by-one in bdrv_aligned_preadv()

[Kevin Wolf]

Am 04.06.2016 um 01:06 hat Eric Blake geschrieben:
> If the amount of data to read ends exactly on the total size
> of the bs, then we were wasting time creating a local qiov
> to read the data in preparation for what would normally be
> appending zeroes beyond the end, even though this corner case
> has nothing further to do.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 7/5] block: Refactor zero_beyond_eof hack in bdrv_aligned_preadv()

[Eric Blake]

Reindent code so that later removal of zero_beyond_eof doesn't
interfere as much with upcoming changes to the normal path.

Patch viewed best with 'git diff -b'.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 74 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/block/io.c b/block/io.c
index c8b323e..243d18a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -944,6 +944,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
 {
     int ret;

+    int64_t total_sectors, max_nb_sectors;
     int64_t sector_num = offset >> BDRV_SECTOR_BITS;
     unsigned int nb_sectors = bytes >> BDRV_SECTOR_BITS;

@@ -981,46 +982,45 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
         }
     }

-    /* Forward the request to the BlockDriver */
+    /* Hack for stashing vmstate beyond guest visibility */
     if (!bs->zero_beyond_eof) {
         ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
+        goto out;
+    }
+
+    /* Forward the request to the BlockDriver */
+    total_sectors = bdrv_nb_sectors(bs);
+    if (total_sectors < 0) {
+        ret = total_sectors;
+        goto out;
+    }
+
+    max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
+                              align >> BDRV_SECTOR_BITS);
+    if (nb_sectors <= max_nb_sectors) {
+        ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
+    } else if (max_nb_sectors > 0) {
+        QEMUIOVector local_qiov;
+
+        qemu_iovec_init(&local_qiov, qiov->niov);
+        qemu_iovec_concat(&local_qiov, qiov, 0,
+                          max_nb_sectors * BDRV_SECTOR_SIZE);
+
+        ret = bdrv_driver_preadv(bs, offset,
+                                 max_nb_sectors * BDRV_SECTOR_SIZE,
+                                 &local_qiov, flags);
+
+        qemu_iovec_destroy(&local_qiov);
     } else {
-        /* Read zeros after EOF */
-        int64_t total_sectors, max_nb_sectors;
-
-        total_sectors = bdrv_nb_sectors(bs);
-        if (total_sectors < 0) {
-            ret = total_sectors;
-            goto out;
-        }
-
-        max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
-                                  align >> BDRV_SECTOR_BITS);
-        if (nb_sectors <= max_nb_sectors) {
-            ret = bdrv_driver_preadv(bs, offset, bytes, qiov, flags);
-        } else if (max_nb_sectors > 0) {
-            QEMUIOVector local_qiov;
-
-            qemu_iovec_init(&local_qiov, qiov->niov);
-            qemu_iovec_concat(&local_qiov, qiov, 0,
-                              max_nb_sectors * BDRV_SECTOR_SIZE);
-
-            ret = bdrv_driver_preadv(bs, offset,
-                                     max_nb_sectors * BDRV_SECTOR_SIZE,
-                                     &local_qiov, flags);
-
-            qemu_iovec_destroy(&local_qiov);
-        } else {
-            ret = 0;
-        }
-
-        /* Reading beyond end of file is supposed to produce zeroes */
-        if (ret == 0 && total_sectors < sector_num + nb_sectors) {
-            uint64_t offset = MAX(0, total_sectors - sector_num);
-            uint64_t bytes = (sector_num + nb_sectors - offset) *
-                              BDRV_SECTOR_SIZE;
-            qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes);
-        }
+        ret = 0;
+    }
+
+    /* Reading beyond end of file is supposed to produce zeroes */
+    if (ret == 0 && total_sectors < sector_num + nb_sectors) {
+        uint64_t offset = MAX(0, total_sectors - sector_num);
+        uint64_t bytes = (sector_num + nb_sectors - offset) *
+                          BDRV_SECTOR_SIZE;
+        qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes);
     }

 out:
-- 
2.5.5

Re: [Qemu-devel] [PATCH 7/5] block: Refactor zero_beyond_eof hack in bdrv_aligned_preadv()

[Kevin Wolf]

Am 04.06.2016 um 01:13 hat Eric Blake geschrieben:
> Reindent code so that later removal of zero_beyond_eof doesn't
> interfere as much with upcoming changes to the normal path.
> 
> Patch viewed best with 'git diff -b'.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

If you don't mind, I would prefer to leave this patch out because it
conflicts with my series. Once I remove bs->zero_beyond_eof, we should
end up in the same place anyway.

Kevin