[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyStat
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState |
Date: |
Mon, 9 Sep 2019 14:59:37 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 30.08.19 18:12, Vladimir Sementsov-Ogievskiy wrote:
> Split copying code part from backup to "block-copy", including separate
> state structure and function renaming. This is needed to share it with
> backup-top filter driver in further commits.
>
> Notes:
>
> 1. As BlockCopyState keeps own BlockBackend objects, remaining
> job->common.blk users only use it to get bs by blk_bs() call, so clear
> job->commen.blk permissions set in block_job_create and add
> job->source_bs to be used instead of blk_bs(job->common.blk), to keep
> it more clear which bs we use when introduce backup-top filter in
> further commit.
>
> 2. Rename s/initializing_bitmap/skip_unallocated/ to sound a bit better
> as interface to BlockCopyState
>
> 3. Split is not very clean: there left some duplicated fields, backup
> code uses some BlockCopyState fields directly, let's postpone it for
> further improvements and keep this comment simpler for review.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> block/backup.c | 357 ++++++++++++++++++++++++++++-----------------
> block/trace-events | 12 +-
> 2 files changed, 231 insertions(+), 138 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index abb5099fa3..002dee4d7f 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -35,12 +35,43 @@ typedef struct CowRequest {
> CoQueue wait_queue; /* coroutines blocked on this request */
> } CowRequest;
>
> +typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
> +typedef void (*ProgressResetCallbackFunc)(void *opaque);
> +typedef struct BlockCopyState {
> + BlockBackend *source;
> + BlockBackend *target;
> + BdrvDirtyBitmap *copy_bitmap;
> + int64_t cluster_size;
> + bool use_copy_range;
> + int64_t copy_range_size;
> + uint64_t len;
> +
> + BdrvRequestFlags write_flags;
> +
> + /*
> + * skip_unallocated: if true, on copy operation firstly reset areas
> + * unallocated in top layer of source (and then of course don't copy
> + * corresponding clusters). If some bytes reset, call
> + * progress_reset_callback.
> + */
It isn’t quite clear that this refers to the copy_bitmap. Maybe
something like
“If true, the copy operation prepares a sync=top job: It scans the
source's top layer to find all unallocated areas and resets them in the
copy_bitmap (so they will not be copied). Whenever any such area is
cleared, progress_reset_callback will be invoked.
Once the whole top layer has been scanned, skip_unallocated is cleared
and the actual copying begins.”
instead?
> + bool skip_unallocated;
> +
> + /* progress_bytes_callback called when some copying progress is done. */
Maybe add a colon after the attribute name? (Or drop the name altogether)
> + ProgressBytesCallbackFunc progress_bytes_callback;
> +
> + /*
> + * progress_reset_callback called when some bytes reset from copy_bitmap
> + * (see @skip_unallocated above)
Maybe you should keep the note you before on what to do then, i.e. that
the callee should probably recalculate how many bytes remain based on
the copy_bitmap’s dirty bit count.
> + */
> + ProgressResetCallbackFunc progress_reset_callback;
> + void *progress_opaque;
> +} BlockCopyState;
[...]
> @@ -415,16 +535,16 @@ static void backup_abort(Job *job)
> static void backup_clean(Job *job)
> {
> BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> - BlockDriverState *bs = blk_bs(s->common.blk);
> + BlockCopyState *bcs = s->bcs;
>
> - if (s->copy_bitmap) {
> - bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> - s->copy_bitmap = NULL;
> - }
> + /*
> + * Zero pointer first, to not interleave with backup_drain during some
> + * yield. TODO: just block_copy_state_free(s->bcs) after backup_drain
> + * dropped.
> + */
I suppose that‘s now. :-)
> + s->bcs = NULL;
>
> - assert(s->target);
> - blk_unref(s->target);
> - s->target = NULL;
> + block_copy_state_free(bcs);
> }
[...]
> @@ -449,8 +569,8 @@ static void backup_drain(BlockJob *job)
> /* Need to keep a reference in case blk_drain triggers execution
> * of backup_complete...
> */
> - if (s->target) {
> - BlockBackend *target = s->target;
> + if (s->bcs) {
> + BlockBackend *target = s->bcs->target;
> blk_ref(target);
> blk_drain(target);
> blk_unref(target);
(And this hunk can go away now.)
[...]
> diff --git a/block/trace-events b/block/trace-events
> index 04209f058d..453792ed87 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -40,12 +40,12 @@ mirror_yield_in_flight(void *s, int64_t offset, int
> in_flight) "s %p offset %" P
> # backup.c
> backup_do_cow_enter(void *job, int64_t start, int64_t offset, uint64_t
> bytes) "job %p start %" PRId64 " offset %" PRId64 " bytes %" PRIu64
> backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret)
> "job %p offset %" PRId64 " bytes %" PRIu64 " ret %d"
> -backup_do_cow_skip(void *job, int64_t start) "job %p start %"PRId64
> -backup_do_cow_skip_range(void *job, int64_t start, uint64_t bytes) "job %p
> start %"PRId64" bytes %"PRId64
> -backup_do_cow_process(void *job, int64_t start) "job %p start %"PRId64
> -backup_do_cow_read_fail(void *job, int64_t start, int ret) "job %p start
> %"PRId64" ret %d"
> -backup_do_cow_write_fail(void *job, int64_t start, int ret) "job %p start
> %"PRId64" ret %d"
> -backup_do_cow_copy_range_fail(void *job, int64_t start, int ret) "job %p
> start %"PRId64" ret %d"
> +block_copy_skip(void *bcs, int64_t start) "job %p start %"PRId64
> +block_copy_skip_range(void *bcs, int64_t start, uint64_t bytes) "job %p
> start %"PRId64" bytes %"PRId64
> +block_copy_process(void *bcs, int64_t start) "job %p start %"PRId64
> +block_copy_with_bounce_buffer_read_fail(void *bcs, int64_t start, int ret)
> "job %p start %"PRId64" ret %d"
> +block_copy_with_bounce_buffer_write_fail(void *bcs, int64_t start, int ret)
> "job %p start %"PRId64" ret %d"
> +block_copy_with_offload_fail(void *bcs, int64_t start, int ret) "job %p
> start %"PRId64" ret %d"
The messages probably should stop calling it a “job”, too.
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState,
Max Reitz <=
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Vladimir Sementsov-Ogievskiy, 2019/09/09
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Max Reitz, 2019/09/09
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Vladimir Sementsov-Ogievskiy, 2019/09/09
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Max Reitz, 2019/09/10
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Vladimir Sementsov-Ogievskiy, 2019/09/10
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Max Reitz, 2019/09/10
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Vladimir Sementsov-Ogievskiy, 2019/09/10
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Max Reitz, 2019/09/10
- Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState, Vladimir Sementsov-Ogievskiy, 2019/09/10