diff mbox

[1/3] mirror: clarify mirror_do_read return code

Message ID 1466625064-11280-2-git-send-email-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Snow June 22, 2016, 7:51 p.m. UTC
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(-)

Comments

Eric Blake June 22, 2016, 8:03 p.m. UTC | #1
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>
Fam Zheng June 23, 2016, 12:36 a.m. UTC | #2
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
>
diff mbox

Patch

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);