mirror: correct buf_size

PATCH Superseded Wen Congyang 3 participants 5 messages May 15, 2015 Block layer

Share & tools

Wen Congyang

author

2015-05-15 01:33 UTC #0 lore raw

PATCH v3 1/1 mirror: correct buf_size

If bus_size is less than 0, the command fails.
If buf_size % granularity is not 0, mirror_free_init() will do dangerous things.

Signed-off-by Wen Congyang <[email protected]>

---
 block/mirror.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

block/mirror.c +5 -1

index 58f391a..7732f8b 100644
@@ -671,6 +671,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (buf_size < 0) {
+        error_setg(errp, "Invalid parameter '%s'", "buf-size");
+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {

@@ -684,7 +688,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
-    s->buf_size = MAX(buf_size, granularity);
+    s->buf_size = ROUND_UP(buf_size, granularity);
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {

Signature · 1 line

2.1.0

Fam Zheng

Reviewed-by on v3 1/1

2015-05-15 01:53 UTC #1 lore raw

Quoted 14 lines · Wen Congyang

If bus_size is less than 0, the command fails.
If buf_size % granularity is not 0, mirror_free_init() will do dangerous things.
Signed-off-by Wen Congyang <[email protected]>
---
 block/mirror.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
block/mirror.c +0 -0
index 58f391a..7732f8b 100644

block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 675

Quoted 5 lines

block/mirror.c +2 -0

@@ -671,6 +671,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (buf_size < 0) {
+        error_setg(errp, "Invalid parameter '%s'", "buf-size");

Just "Invalid parameter 'buf-size'", If you fix that,

Reviewed-by Fam Zheng <[email protected]>

Quoted 16 lines

block/mirror.c +3 -1

+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {

@@ -684,7 +688,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
-    s->buf_size = MAX(buf_size, granularity);
+    s->buf_size = ROUND_UP(buf_size, granularity);
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {

Signature · 1 line

2.1.0

Paolo Bonzini

2015-05-15 06:45 UTC #2 lore raw

In reply to Wen Congyang

Quoted 14 lines · Wen Congyang

If bus_size is less than 0, the command fails.
If buf_size % granularity is not 0, mirror_free_init() will do dangerous things.
Signed-off-by Wen Congyang <[email protected]>
---
 block/mirror.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
block/mirror.c +0 -0
index 58f391a..7732f8b 100644

block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 674

Quoted 4 lines

block/mirror.c +1 -0

@@ -671,6 +671,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (buf_size < 0) {

This should be <=.

Jeff, can you adjust when committing?

Paolo

Quoted 16 lines

block/mirror.c +4 -1

+        error_setg(errp, "Invalid parameter '%s'", "buf-size");
+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {

@@ -684,7 +688,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
-    s->buf_size = MAX(buf_size, granularity);
+    s->buf_size = ROUND_UP(buf_size, granularity);
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {

Wen Congyang

author

2015-05-15 06:50 UTC #3 lore raw

Quoted 20 lines · Paolo Bonzini

Quoted 17 lines · Wen Congyang
If bus_size is less than 0, the command fails.
If buf_size % granularity is not 0, mirror_free_init() will do dangerous things.
Signed-off-by Wen Congyang <[email protected]>
---
 block/mirror.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
block/mirror.c +0 -0
index 58f391a..7732f8b 100644
@@ -671,6 +671,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 

Quoted 3 lines

block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 674
Quoted 1 line
block/mirror.c +1 -0
+    if (buf_size < 0) {
This should be <=.

But commit_active_start() passes 0...

Thanks
Wen Congyang

Quoted 23 lines

Jeff, can you adjust when committing?

Paolo

Quoted 8 lines

block/mirror.c +3 -0

+        error_setg(errp, "Invalid parameter '%s'", "buf-size");
+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {

block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 694

Quoted 8 lines

block/mirror.c +1 -1

@@ -684,7 +688,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
-    s->buf_size = MAX(buf_size, granularity);
+    s->buf_size = ROUND_UP(buf_size, granularity);
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {

Paolo Bonzini

2015-05-15 07:26 UTC #4 lore raw

Quoted 21 lines · Wen Congyang

Quoted 20 lines · Paolo Bonzini
Quoted 17 lines · Wen Congyang
If bus_size is less than 0, the command fails.
If buf_size % granularity is not 0, mirror_free_init() will do dangerous things.
Signed-off-by Wen Congyang <[email protected]>
---
 block/mirror.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
block/mirror.c +0 -0
index 58f391a..7732f8b 100644
@@ -671,6 +671,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 

Quoted 5 lines

Quoted 3 lines
block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 674
Quoted 1 line
block/mirror.c +1 -0
+    if (buf_size < 0) {
This should be <=.
But commit_active_start() passes 0...

This was changed to granularity by MAX, while it would be 0 after ROUND_UP. The right value would be DEFAULT_MIRROR_BUF_SIZE.

block_commit should have an optional buf_size, but for now I guess you can change a 0 to DEFAULT_MIRROR_BUF_SIZE in mirror_start_job, and let qmp_drive_mirror do this:

     if (!has_buf_size) {
-        buf_size = DEFAULT_MIRROR_BUF_SIZE;
+        buf_size = 0;
     }

Paolo

Quoted 29 lines

Thanks
Wen Congyang

Quoted 23 lines

Jeff, can you adjust when committing?

Paolo

Quoted 8 lines

block/mirror.c +3 -0

+        error_setg(errp, "Invalid parameter '%s'", "buf-size");
+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {

block/mirror.c static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, · line 694

Quoted 8 lines

block/mirror.c +1 -1

@@ -684,7 +688,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
-    s->buf_size = MAX(buf_size, granularity);
+    s->buf_size = ROUND_UP(buf_size, granularity);
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {