[Qemu-devel] [PATCH 0/3] drive-mirror: limit niov to MAX_IOV

[Qemu-devel] [PATCH 0/3] drive-mirror: limit niov to MAX_IOV

[John Snow]

e5b43573 caused a regression in the preparation of our IO vectors, such
that if a small granularity but a large buffer size is chosen, we may
accidentally exceed MAX_IOV and the request will fail.

This has been fixed before in cae98cb8, and now we'll fix it again.
To keep it fixed, we'll add an iotest this time.

[Thanks to Max for finding the root cause.]

John Snow (3):
  mirror: clarify mirror_do_read return code
  mirror: limit niov to IOV_MAX elements, again
  iotests: add small-granularity mirror test

 block/mirror.c             | 10 ++++++++--
 tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 ++--
 3 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.4.11

[Qemu-devel] [PATCH 1/3] mirror: clarify mirror_do_read return code

[John Snow]

mirror_do_read intends to return the number of sectors processed after
the starting sector, without regard to how many sectors were processed
before the starting sector due to alignment.

Clean up the comments and code to hopefully illustrate this more clearly.

This also fixes an issue in initialization where if the mirror buffer size
is initialized to smaller than the number of sectors being requested for
transfer, we report back an incorrectly large number to the caller.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index a04ed9c..2ba9642 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -218,7 +218,9 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
 }
 
 /* Submit async read while handling COW.
- * Returns: nb_sectors if no alignment is necessary, or
+ * 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
  *          alignment or buffer limit.
  */
@@ -227,7 +229,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
 {
     BlockBackend *source = s->common.blk;
     int sectors_per_chunk, nb_chunks;
-    int ret = nb_sectors;
+    int ret;
     MirrorOp *op;
 
     sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
@@ -235,6 +237,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
     /* We can only handle as much as buf_size at a time. */
     nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
     assert(nb_sectors);
+    ret = nb_sectors;
 
     if (s->cow_bitmap) {
         ret += mirror_cow_align(s, &sector_num, &nb_sectors);
-- 
2.4.11

Re: [Qemu-devel] [PATCH 1/3] mirror: clarify mirror_do_read return code

[Eric Blake]

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

On 06/22/2016 01:51 PM, John Snow wrote:
> mirror_do_read intends to return the number of sectors processed after
> the starting sector, without regard to how many sectors were processed
> before the starting sector due to alignment.
> 

Eventually, we may want this to be in terms of bytes instead of sectors;
but for now the new wording is an improvement.

> Clean up the comments and code to hopefully illustrate this more clearly.
> 
> This also fixes an issue in initialization where if the mirror buffer size
> is initialized to smaller than the number of sectors being requested for
> transfer, we report back an incorrectly large number to the caller.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 1/3] mirror: clarify mirror_do_read return code

[Fam Zheng]

On Wed, 06/22 15:51, John Snow wrote:
> mirror_do_read intends to return the number of sectors processed after
> the starting sector, without regard to how many sectors were processed
> before the starting sector due to alignment.
> 
> Clean up the comments and code to hopefully illustrate this more clearly.
> 
> This also fixes an issue in initialization where if the mirror buffer size
> is initialized to smaller than the number of sectors being requested for
> transfer, we report back an incorrectly large number to the caller.

Good catch!

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..2ba9642 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -218,7 +218,9 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
>  }
>  
>  /* Submit async read while handling COW.
> - * Returns: nb_sectors if no alignment is necessary, or
> + * 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
>   *          alignment or buffer limit.
>   */
> @@ -227,7 +229,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
>  {
>      BlockBackend *source = s->common.blk;
>      int sectors_per_chunk, nb_chunks;
> -    int ret = nb_sectors;
> +    int ret;
>      MirrorOp *op;
>  
>      sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> @@ -235,6 +237,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
>      /* We can only handle as much as buf_size at a time. */
>      nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
>      assert(nb_sectors);
> +    ret = nb_sectors;
>  
>      if (s->cow_bitmap) {
>          ret += mirror_cow_align(s, &sector_num, &nb_sectors);
> -- 
> 2.4.11
> 

[Qemu-devel] [PATCH 2/3] mirror: limit niov to IOV_MAX elements, again

[John Snow]

During the refactor of mirror_iteration in e5b43573,
we regressed the fix introduced in cae98cb8.

This patch re-adds IOV_MAX checking to cases where we
aren't checking alignment (and size) already.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 2ba9642..377339d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -231,11 +231,14 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
     int sectors_per_chunk, nb_chunks;
     int ret;
     MirrorOp *op;
+    int max_sectors;
 
     sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
+    max_sectors = sectors_per_chunk * 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);
     ret = nb_sectors;
 
-- 
2.4.11

Re: [Qemu-devel] [PATCH 2/3] mirror: limit niov to IOV_MAX elements, again

[Eric Blake]

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

On 06/22/2016 01:51 PM, John Snow wrote:
> During the refactor of mirror_iteration in e5b43573,
> we regressed the fix introduced in cae98cb8.
> 
> This patch re-adds IOV_MAX checking to cases where we
> aren't checking alignment (and size) already.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2ba9642..377339d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -231,11 +231,14 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
>      int sectors_per_chunk, nb_chunks;
>      int ret;
>      MirrorOp *op;
> +    int max_sectors;
>  
>      sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> +    max_sectors = sectors_per_chunk * 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);
>      ret = nb_sectors;
>  
> 

-- 
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 2/3] mirror: limit niov to IOV_MAX elements, again

[Fam Zheng]

On Wed, 06/22 15:51, John Snow wrote:
> During the refactor of mirror_iteration in e5b43573,
> we regressed the fix introduced in cae98cb8.
> 
> This patch re-adds IOV_MAX checking to cases where we
> aren't checking alignment (and size) already.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2ba9642..377339d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -231,11 +231,14 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
>      int sectors_per_chunk, nb_chunks;
>      int ret;
>      MirrorOp *op;
> +    int max_sectors;
>  
>      sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> +    max_sectors = sectors_per_chunk * 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);
>      ret = nb_sectors;
>  
> -- 
> 2.4.11
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 3/3] iotests: add small-granularity mirror test

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ed1d9d4..cbf5e0b 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -727,6 +727,36 @@ class TestUnbackedSource(iotests.QMPTestCase):
         self.complete_and_wait()
         self.assert_no_active_block_jobs()
 
+class TestGranularity(iotests.QMPTestCase):
+    image_len = 10 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img,
+                 str(TestGranularity.image_len))
+        qemu_io('-c', 'write 0 %d' % (self.image_len),
+                test_img)
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+        os.remove(test_img)
+        os.remove(target_img)
+
+    def test_granularity(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-mirror', device='drive0',
+                             sync='full', target=target_img,
+                             mode='absolute-paths', granularity=8192)
+        self.assert_qmp(result, 'return', {})
+        event = self.vm.get_qmp_event(wait=60.0)
+        # Failures will manifest as COMPLETED/ERROR.
+        self.assert_qmp(event, 'event', 'BLOCK_JOB_READY')
+        self.complete_and_wait(drive='drive0', wait_ready=False)
+        self.assert_no_active_block_jobs()
+
 class TestRepairQuorum(iotests.QMPTestCase):
     """ This class test quorum file repair using drive-mirror.
         It's mostly a fork of TestSingleDrive """
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index b0cadc8..b67d050 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................................................................
+............................................................................
 ----------------------------------------------------------------------
-Ran 75 tests
+Ran 76 tests
 
 OK
-- 
2.4.11

Re: [Qemu-devel] [PATCH 3/3] iotests: add small-granularity mirror test

[Eric Blake]

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

On 06/22/2016 01:51 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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/3] iotests: add small-granularity mirror test

[Fam Zheng]

On Wed, 06/22 15:51, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  2 files changed, 32 insertions(+), 2 deletions(-)

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] drive-mirror: limit niov to MAX_IOV

[John Snow]

Ping, all good?

Jeff Cody, I think this is yours?

On 06/22/2016 03:51 PM, John Snow wrote:
> e5b43573 caused a regression in the preparation of our IO vectors, such
> that if a small granularity but a large buffer size is chosen, we may
> accidentally exceed MAX_IOV and the request will fail.
> 
> This has been fixed before in cae98cb8, and now we'll fix it again.
> To keep it fixed, we'll add an iotest this time.
> 
> [Thanks to Max for finding the root cause.]
> 
> John Snow (3):
>   mirror: clarify mirror_do_read return code
>   mirror: limit niov to IOV_MAX elements, again
>   iotests: add small-granularity mirror test
> 
>  block/mirror.c             | 10 ++++++++--
>  tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/3] drive-mirror: limit niov to MAX_IOV

[Jeff Cody]

On Wed, Jun 22, 2016 at 03:51:01PM -0400, John Snow wrote:
> e5b43573 caused a regression in the preparation of our IO vectors, such
> that if a small granularity but a large buffer size is chosen, we may
> accidentally exceed MAX_IOV and the request will fail.
> 
> This has been fixed before in cae98cb8, and now we'll fix it again.
> To keep it fixed, we'll add an iotest this time.
> 
> [Thanks to Max for finding the root cause.]
> 
> John Snow (3):
>   mirror: clarify mirror_do_read return code
>   mirror: limit niov to IOV_MAX elements, again
>   iotests: add small-granularity mirror test
> 
>  block/mirror.c             | 10 ++++++++--
>  tests/qemu-iotests/041     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out |  4 ++--
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> -- 
> 2.4.11
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff