[Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based

Eric Blake posted 21 patches 8 years, 3 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based
Posted by Eric Blake 8 years, 3 months ago
We are gradually converting to byte-based interfaces, as they are
easier to reason about than sector-based.  Convert another internal
function (no semantic change).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>

---
v3-v4: no change
v2: rebase to earlier changes
---
 block/mirror.c | 75 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 42 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 374cefd..d3325d0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -196,7 +196,7 @@ static inline int mirror_clip_sectors(MirrorBlockJob *s,
 /* Round offset and/or bytes to target cluster if COW is needed, and
  * return the offset of the adjusted tail against original. */
 static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
-                            unsigned int *bytes)
+                            uint64_t *bytes)
 {
     bool need_cow;
     int ret = 0;
@@ -204,6 +204,7 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
     unsigned int align_bytes = *bytes;
     int max_bytes = s->granularity * s->max_iov;

+    assert(*bytes < INT_MAX);
     need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
     need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
                           s->cow_bitmap);
@@ -239,59 +240,50 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
 }

 /* Submit async read while handling COW.
- * Returns: The number of sectors copied after and including sector_num,
- *          excluding any sectors copied prior to sector_num due to alignment.
- *          This will be nb_sectors if no alignment is necessary, or
- *          (new_end - sector_num) if tail is rounded up or down due to
+ * Returns: The number of bytes copied after and including offset,
+ *          excluding any bytes copied prior to offset due to alignment.
+ *          This will be @bytes if no alignment is necessary, or
+ *          (new_end - offset) if tail is rounded up or down due to
  *          alignment or buffer limit.
  */
-static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
-                          int nb_sectors)
+static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
+                               uint64_t bytes)
 {
     BlockBackend *source = s->common.blk;
-    int sectors_per_chunk, nb_chunks;
-    int ret;
+    int nb_chunks;
+    uint64_t ret;
     MirrorOp *op;
-    int max_sectors;
+    uint64_t max_bytes;

-    sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
-    max_sectors = sectors_per_chunk * s->max_iov;
+    max_bytes = s->granularity * s->max_iov;

     /* We can only handle as much as buf_size at a time. */
-    nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
-    nb_sectors = MIN(max_sectors, nb_sectors);
-    assert(nb_sectors);
-    assert(nb_sectors < BDRV_REQUEST_MAX_SECTORS);
-    ret = nb_sectors;
+    bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
+    assert(bytes);
+    assert(bytes < BDRV_REQUEST_MAX_BYTES);
+    ret = bytes;

     if (s->cow_bitmap) {
-        int64_t offset = sector_num * BDRV_SECTOR_SIZE;
-        unsigned int bytes = nb_sectors * BDRV_SECTOR_SIZE;
-        int gap;
-
-        gap = mirror_cow_align(s, &offset, &bytes);
-        sector_num = offset / BDRV_SECTOR_SIZE;
-        nb_sectors = bytes / BDRV_SECTOR_SIZE;
-        ret += gap / BDRV_SECTOR_SIZE;
+        ret += mirror_cow_align(s, &offset, &bytes);
     }
-    assert(nb_sectors << BDRV_SECTOR_BITS <= s->buf_size);
-    /* The sector range must meet granularity because:
+    assert(bytes <= s->buf_size);
+    /* The range will be sector-aligned because:
      * 1) Caller passes in aligned values;
-     * 2) mirror_cow_align is used only when target cluster is larger. */
-    assert(!(sector_num % sectors_per_chunk));
-    nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
+     * 2) mirror_cow_align is used only when target cluster is larger.
+     * But it might not be cluster-aligned at end-of-file. */
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
+    nb_chunks = DIV_ROUND_UP(bytes, s->granularity);

     while (s->buf_free_count < nb_chunks) {
-        trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE,
-                                     s->in_flight);
+        trace_mirror_yield_in_flight(s, offset, s->in_flight);
         mirror_wait_for_io(s);
     }

     /* Allocate a MirrorOp that is used as an AIO callback.  */
     op = g_new(MirrorOp, 1);
     op->s = s;
-    op->offset = sector_num * BDRV_SECTOR_SIZE;
-    op->bytes = nb_sectors * BDRV_SECTOR_SIZE;
+    op->offset = offset;
+    op->bytes = bytes;

     /* Now make a QEMUIOVector taking enough granularity-sized chunks
      * from s->buf_free.
@@ -299,7 +291,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
     qemu_iovec_init(&op->qiov, nb_chunks);
     while (nb_chunks-- > 0) {
         MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
-        size_t remaining = nb_sectors * BDRV_SECTOR_SIZE - op->qiov.size;
+        size_t remaining = bytes - op->qiov.size;

         QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
         s->buf_free_count--;
@@ -308,12 +300,10 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,

     /* Copy the dirty cluster.  */
     s->in_flight++;
-    s->bytes_in_flight += nb_sectors * BDRV_SECTOR_SIZE;
-    trace_mirror_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
-                               nb_sectors * BDRV_SECTOR_SIZE);
+    s->bytes_in_flight += bytes;
+    trace_mirror_one_iteration(s, offset, bytes);

-    blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, 0,
-                   mirror_read_complete, op);
+    blk_aio_preadv(source, offset, &op->qiov, 0, mirror_read_complete, op);
     return ret;
 }

@@ -461,8 +451,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         io_sectors = mirror_clip_sectors(s, sector_num, io_sectors);
         switch (mirror_method) {
         case MIRROR_METHOD_COPY:
-            io_sectors = mirror_do_read(s, sector_num, io_sectors);
-            io_bytes_acct = io_sectors * BDRV_SECTOR_SIZE;
+            io_bytes_acct = mirror_do_read(s, sector_num * BDRV_SECTOR_SIZE,
+                                           io_sectors * BDRV_SECTOR_SIZE);
+            io_sectors = io_bytes_acct / BDRV_SECTOR_SIZE;
             break;
         case MIRROR_METHOD_ZERO:
         case MIRROR_METHOD_DISCARD:
-- 
2.9.4


Re: [Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based
Posted by Kevin Wolf 8 years, 3 months ago
Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Jeff Cody <jcody@redhat.com>

> -    /* The sector range must meet granularity because:
> +    assert(bytes <= s->buf_size);
> +    /* The range will be sector-aligned because:
>       * 1) Caller passes in aligned values;
> -     * 2) mirror_cow_align is used only when target cluster is larger. */
> -    assert(!(sector_num % sectors_per_chunk));
> -    nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
> +     * 2) mirror_cow_align is used only when target cluster is larger.
> +     * But it might not be cluster-aligned at end-of-file. */
> +    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +    nb_chunks = DIV_ROUND_UP(bytes, s->granularity);

The assertion in the old code was about sector_num (i.e.  that the start
of the range is cluster-aligned), not about nb_sectors, so while you add
a new assertion, it is asserting something different. This explains
why you had to switch to sector aligned even though the semantics
shouldn't be changed by this patch.

Is this intentional or did you confuse sector_num and nb_sectors? I
think we can just have both assertions.

The rest of the patch looks okay.

Kevin

Re: [Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based
Posted by Eric Blake 8 years, 3 months ago
On 07/06/2017 08:30 AM, Kevin Wolf wrote:
> Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
>> We are gradually converting to byte-based interfaces, as they are
>> easier to reason about than sector-based.  Convert another internal
>> function (no semantic change).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Jeff Cody <jcody@redhat.com>
> 
>> -    /* The sector range must meet granularity because:
>> +    assert(bytes <= s->buf_size);
>> +    /* The range will be sector-aligned because:
>>       * 1) Caller passes in aligned values;
>> -     * 2) mirror_cow_align is used only when target cluster is larger. */
>> -    assert(!(sector_num % sectors_per_chunk));

So the strict translation would be assert(!(offset % s->granularity)),
or rewritten to be assert(QEMU_IS_ALIGNED(offset, s->granularity)).

>> -    nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
>> +     * 2) mirror_cow_align is used only when target cluster is larger.
>> +     * But it might not be cluster-aligned at end-of-file. */
>> +    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));

and you're right that this appears to be a new assertion.

>> +    nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
> 
> The assertion in the old code was about sector_num (i.e.  that the start
> of the range is cluster-aligned), not about nb_sectors, so while you add
> a new assertion, it is asserting something different. This explains
> why you had to switch to sector aligned even though the semantics
> shouldn't be changed by this patch.
> 
> Is this intentional or did you confuse sector_num and nb_sectors? I
> think we can just have both assertions.

At this point, I'm not sure if I confused things, or if I hit an actual
iotest failure later in the series that I traced back to this point.  If
I have a reason to respin, I'll see if both assertions still hold (the
rewritten alignment check on offset, and the new check on bytes being
sector-aligned), through all the tests.  Also, while asserting that
bytes is sector-aligned is good in the short term, it may be overkill by
the time dirty-bitmap is changed to byte alignment (right now,
bdrv_getlength() is always sector-aligned, because it rounds up; but if
we ever make it byte-accurate, then the trailing cluster might not be a
sector-aligned bytes length).

> 
> The rest of the patch looks okay.
> 
> Kevin
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based
Posted by Kevin Wolf 8 years, 3 months ago
Am 06.07.2017 um 16:25 hat Eric Blake geschrieben:
> On 07/06/2017 08:30 AM, Kevin Wolf wrote:
> > Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
> >> We are gradually converting to byte-based interfaces, as they are
> >> easier to reason about than sector-based.  Convert another internal
> >> function (no semantic change).
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> Reviewed-by: John Snow <jsnow@redhat.com>
> >> Reviewed-by: Jeff Cody <jcody@redhat.com>
> > 
> >> -    /* The sector range must meet granularity because:
> >> +    assert(bytes <= s->buf_size);
> >> +    /* The range will be sector-aligned because:
> >>       * 1) Caller passes in aligned values;
> >> -     * 2) mirror_cow_align is used only when target cluster is larger. */
> >> -    assert(!(sector_num % sectors_per_chunk));
> 
> So the strict translation would be assert(!(offset % s->granularity)),
> or rewritten to be assert(QEMU_IS_ALIGNED(offset, s->granularity)).

Right.

> >> -    nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
> >> +     * 2) mirror_cow_align is used only when target cluster is larger.
> >> +     * But it might not be cluster-aligned at end-of-file. */
> >> +    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> 
> and you're right that this appears to be a new assertion.
> 
> >> +    nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
> > 
> > The assertion in the old code was about sector_num (i.e.  that the start
> > of the range is cluster-aligned), not about nb_sectors, so while you add
> > a new assertion, it is asserting something different. This explains
> > why you had to switch to sector aligned even though the semantics
> > shouldn't be changed by this patch.
> > 
> > Is this intentional or did you confuse sector_num and nb_sectors? I
> > think we can just have both assertions.
> 
> At this point, I'm not sure if I confused things, or if I hit an actual
> iotest failure later in the series that I traced back to this point.

But if you did, wouldn't this indicate a real bug in the patch?

> If I have a reason to respin, I'll see if both assertions still hold
> (the rewritten alignment check on offset, and the new check on bytes
> being sector-aligned), through all the tests.

Actually, I think this one, while seemingly minor, is a reason to
respin. This kind of assertions is exactly for preventing bugs when
doing changes like this patch does, so removing the assertion in such a
patch looks really suspicious.

Also, with the new assertion, the comment doesn't really make that much
sense any more either.

> Also, while asserting that bytes is sector-aligned is good in the
> short term, it may be overkill by the time dirty-bitmap is changed to
> byte alignment (right now, bdrv_getlength() is always sector-aligned,
> because it rounds up; but if we ever make it byte-accurate, then the
> trailing cluster might not be a sector-aligned bytes length).

This is true, but I think for the time being the assertion is worth it.

Kevin