Re: [Qemu-devel] [PATCH v3 2/8] block: add basic backup support to block driver

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

> From: Dietmar Maurer <dietmar@proxmox.com>
> 
> backup_start() creates a block job that copies a point-in-time snapshot
> of a block device to a target block device.
> 
> We call backup_do_cow() for each write during backup. That function
> reads the original data from the block device before it gets
> overwritten.  The data is then written to the target device.
> 
> Currently backup cluster size is hardcoded to 65536 bytes.
> 
> [I made a number of changes to Dietmar's original patch and folded them
> in to make code review easy.  Here is the full list:
> 
>   * Drop BackupDumpFunc interface in favor of a target block device
>   * Detect zero clusters with buffer_is_zero()
>   * Don't write zero clusters to the target
>   * Use 0 delay instead of 1us, like other block jobs
>   * Unify creation/start functions into backup_start()
>   * Simplify cleanup, free bitmap in backup_run() instead of cb
>   * function
>   * Use HBitmap to avoid duplicating bitmap code
>   * Use bdrv_getlength() instead of accessing ->total_sectors
>   * directly
>   * Delete the backup.h header file, it is no longer necessary
>   * Move ./backup.c to block/backup.c
>   * Remove #ifdefed out code
>   * Coding style and whitespace cleanups
>   * Use bdrv_add_before_write_cb() instead of blockjob-specific hooks
>   * Keep our own in-flight CowRequest list instead of using block.c
>     tracked requests
> 
> -- stefanha]
> 
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/Makefile.objs       |   1 +
>   block/backup.c            | 282 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block_int.h |  16 +++
>   3 files changed, 299 insertions(+)
>   create mode 100644 block/backup.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 5f0358a..88bd101 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -20,5 +20,6 @@ endif
>   common-obj-y += stream.o
>   common-obj-y += commit.o
>   common-obj-y += mirror.o
> +common-obj-y += backup.o
> 
>   $(obj)/curl.o: QEMU_CFLAGS+=$(CURL_CFLAGS)
> diff --git a/block/backup.c b/block/backup.c
> new file mode 100644
> index 0000000..01d2fd3
> --- /dev/null
> +++ b/block/backup.c
> @@ -0,0 +1,282 @@
> +/*
> + * QEMU backup
> + *
> + * Copyright (C) 2013 Proxmox Server Solutions
> + *
> + * Authors:
> + *  Dietmar Maurer (dietmar@proxmox.com)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include "block/block.h"
> +#include "block/block_int.h"
> +#include "block/blockjob.h"
> +#include "qemu/ratelimit.h"
> +
> +#define DEBUG_BACKUP 0
> +
> +#define DPRINTF(fmt, ...) \
> +    do { \
> +        if (DEBUG_BACKUP) { \
> +            fprintf(stderr, "backup: " fmt, ## __VA_ARGS__); \
> +        } \
> +    } while (0)
> +
> +#define BACKUP_CLUSTER_BITS 16
> +#define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> +#define BACKUP_BLOCKS_PER_CLUSTER (BACKUP_CLUSTER_SIZE / BDRV_SECTOR_SIZE)
> +
> +#define SLICE_TIME 100000000ULL /* ns */
> +
> +typedef struct CowRequest {
> +    int64_t start;
> +    int64_t end;
> +    QLIST_ENTRY(CowRequest) list;
> +    CoQueue wait_queue; /* coroutines blocked on this request */
> +} CowRequest;
> +
> +typedef struct BackupBlockJob {
> +    BlockJob common;
> +    BlockDriverState *target;
> +    RateLimit limit;
> +    CoRwlock flush_rwlock;
> +    uint64_t sectors_read;
> +    HBitmap *bitmap;
> +    QLIST_HEAD(, CowRequest) inflight_reqs;
> +} BackupBlockJob;
> +
> +/* See if in-flight requests overlap and wait for them to complete */
> +static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> +                                                       int64_t start,
> +                                                       int64_t end)
> +{
> +    CowRequest *req;
> +    bool retry;
> +
> +    do {
> +        retry = false;
> +        QLIST_FOREACH(req, &job->inflight_reqs, list) {
> +            if (end > req->start && start < req->end) {
> +                qemu_co_queue_wait(&req->wait_queue);
> +                retry = true;
> +                break;
> +            }
> +        }
> +    } while (retry);
> +}
> +

  In my understanding, there will be possible two program routines entering
here at same time since it holds read lock instead of write lock, and
they may also modify job->inflight_reqs, is it possible a race
condition here? I am not sure whether back-ground job will becomes a
thread.



-- 
Best Regards

Wenchao Xia