Re: [Qemu-devel] [PATCH 7/9] mirror: allow to save buffer for QEMUIOVector in MirrorOp

[Eric Blake <eblake@redhat.com>]

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

On 06/14/2016 09:25 AM, Denis V. Lunev wrote:

In the subject: 'allow to save buffer' is not idiomatic English; better
would be 'allow saving the buffer' or simply 'save the buffer'

> Properly cook MirrorOp initialization/deinitialization. The field is not
> yet used actually.

Another "what" but not "why" - expanding the commit message to mention
"why" makes it easier to review.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  block/mirror.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index d8be80a..7471211 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -73,6 +73,7 @@ typedef struct MirrorOp {
>      QEMUIOVector qiov;
>      int64_t sector_num;
>      int nb_sectors;
> +    void *buf;
>  } MirrorOp;
>  
>  static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
> @@ -100,24 +101,28 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>      s->in_flight--;
>      s->sectors_in_flight -= op->nb_sectors;
>      iov = op->qiov.iov;
> -    for (i = 0; i < op->qiov.niov; i++) {
> -        MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
> -        QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
> -        s->buf_free_count++;
> -    }
>  
> -    sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> -    chunk_num = op->sector_num / sectors_per_chunk;
> -    nb_chunks = DIV_ROUND_UP(op->nb_sectors, sectors_per_chunk);
> -    bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
> -    if (ret >= 0) {
> -        if (s->cow_bitmap) {
> -            bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
> +    if (op->buf == NULL) {
> +        for (i = 0; i < op->qiov.niov; i++) {
> +            MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
> +            QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
> +            s->buf_free_count++;
> +        }
> +
> +        sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> +        chunk_num = op->sector_num / sectors_per_chunk;
> +        nb_chunks = DIV_ROUND_UP(op->nb_sectors, sectors_per_chunk);
> +        bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);

I still think it might be smarter to fix bitmap operations to work on
byte inputs (still sectors, or rather granularity chunks, under the
hood, but no need to make users scale things just to have it scaled again).

> +        if (ret >= 0) {
> +            if (s->cow_bitmap) {
> +                bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
> +            }
> +            s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
>          }
> -        s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
>      }
>  
>      qemu_iovec_destroy(&op->qiov);
> +    g_free(op->buf);
>      g_free(op);
>  
>      if (s->waiting_for_io) {
> @@ -255,6 +260,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
>      op->s = s;
>      op->sector_num = sector_num;
>      op->nb_sectors = nb_sectors;
> +    op->buf = NULL;
>  
>      /* Now make a QEMUIOVector taking enough granularity-sized chunks
>       * from s->buf_free.
> 

-- 
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 --]