On 26.08.19 18:13, 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
I suppose these should be BdrvChild objects at some point, but doing it now would just mean effectively duplicating code from block-backend.c. (“now” = before we have a backup-top filter to attach the children to.) > 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. > > 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 Are there any but cluster_size and len (and source, in a sense)? > 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 <vsement...@virtuozzo.com> > --- > block/backup.c | 324 +++++++++++++++++++++++++++------------------ > block/trace-events | 12 +- > 2 files changed, 200 insertions(+), 136 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 13a1d80157..f52ac622e0 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -35,12 +35,35 @@ typedef struct CowRequest { > CoQueue wait_queue; /* coroutines blocked on this request */ > } CowRequest; > > +/* > + * ProgressCallbackFunc > + * > + * Called when some progress is done in context of BlockCopyState: > + * 1. When some bytes copied, called with @bytes > 0. > + * 2. When some bytes resetted from copy_bitmap, called with @bytes = 0 > (user *reset > + * may recalculate remaining bytes from copy_bitmap dirty count. > + */ > +typedef void (*ProgressCallbackFunc)(int64_t bytes, void *opaque); Maybe there should be two callbacks instead, one for “We’ve actively made progress” (bytes > 0) and one for “The expected length has changed” (bytes == 0)? > +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; > + bool skip_unallocated; The rename seems reasonable, although I think this should get a comment, because it doesn’t mean just to skip unallocated clusters; it also means to clear unallocated clusters from the bitmap. > + > + ProgressCallbackFunc progress_callback; > + void *progress_opaque; > +} BlockCopyState; > + > typedef struct BackupBlockJob { > BlockJob common; > - BlockBackend *target; > > BdrvDirtyBitmap *sync_bitmap; > - BdrvDirtyBitmap *copy_bitmap; > > MirrorSyncMode sync_mode; > BitmapSyncMode bitmap_mode; [...] > @@ -99,9 +118,83 @@ static void cow_request_end(CowRequest *req) > qemu_co_queue_restart_all(&req->wait_queue); > } > > +static void block_copy_state_free(BlockCopyState *s) > +{ > + if (!s) { > + return; > + } > + > + bdrv_release_dirty_bitmap(blk_bs(s->source), s->copy_bitmap); > + blk_unref(s->source); > + s->source = NULL; > + blk_unref(s->target); > + s->target = NULL; I’m not quite sure why you NULL these pointers when you free the whole object next anyway. > + g_free(s); > +} > + > +static BlockCopyState *block_copy_state_new( > + BlockDriverState *source, BlockDriverState *target, > + int64_t cluster_size, BdrvRequestFlags write_flags, > + ProgressCallbackFunc progress_callback, void *progress_opaque, > + Error **errp) > +{ > + BlockCopyState *s; > + int ret; > + uint64_t no_resize = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | > + BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD; > + BdrvDirtyBitmap *copy_bitmap = > + bdrv_create_dirty_bitmap(source, cluster_size, NULL, errp); > + This probably were easier to read if you didn’t initialize copy_bitmap with the bdrv_create_dirty_bitmap() call but instead moved that call right above the if () here (it still fits on a single line). > + if (!copy_bitmap) { > + return NULL; > + } > + bdrv_disable_dirty_bitmap(copy_bitmap); > + > + s = g_new0(BlockCopyState, 1); With the following compound literal, you don’t need to allocate zero-initialized memory here. > + *s = (BlockCopyState) { > + .source = blk_new(bdrv_get_aio_context(source), > + BLK_PERM_CONSISTENT_READ, no_resize), > + .target = blk_new(bdrv_get_aio_context(target), > + BLK_PERM_WRITE, no_resize), Maybe we want to assert that source’s and target’s context are the same? > + .copy_bitmap = copy_bitmap, > + .cluster_size = cluster_size, > + .len = bdrv_dirty_bitmap_size(copy_bitmap), > + .write_flags = write_flags, > + .use_copy_range = !(write_flags & BDRV_REQ_WRITE_COMPRESSED), > + .progress_callback = progress_callback, > + .progress_opaque = progress_opaque, > + }; > + > + s->copy_range_size = QEMU_ALIGN_UP(MIN(blk_get_max_transfer(s->source), > + blk_get_max_transfer(s->target)), > + s->cluster_size), Nice simplification. > + > + blk_set_disable_request_queuing(s->source, true); > + blk_set_allow_aio_context_change(s->source, true); > + blk_set_disable_request_queuing(s->target, true); > + blk_set_allow_aio_context_change(s->target, true); Hm. Doesn’t creating new BBs here mean that we have to deal with the fallout of changing the AioContext on either of them somewhere? [...] > @@ -449,8 +542,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 && s->bcs->target) { bcs->target should always be non-NULL, shouldn’t it? > + BlockBackend *target = s->bcs->target; > blk_ref(target); > blk_drain(target); > blk_unref(target); [...] > @@ -730,57 +821,34 @@ BlockJob *backup_job_create(const char *job_id, > BlockDriverState *bs, [...] > - /* > - * Set write flags: > - * 1. Detect image-fleecing (and similar) schemes > - * 2. Handle compression > - */ > - job->write_flags = > - (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) | > - (compress ? BDRV_REQ_WRITE_COMPRESSED : 0); > + job->bcs = block_copy_state_new( > + bs, target, cluster_size, > + /* > + * Set write flags: > + * 1. Detect image-fleecing (and similar) schemes > + * 2. Handle compression > + */ > + (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) | > + (compress ? BDRV_REQ_WRITE_COMPRESSED : 0), This is a bit hard to read. Why not add a dedicated variable for it? > + backup_progress_callback, job, errp); > + if (!job->bcs) { > + goto error; > + } > > job->cluster_size = cluster_size; > - job->copy_bitmap = copy_bitmap; > - copy_bitmap = NULL; > - job->use_copy_range = !compress; /* compression isn't supported for it */ > - job->copy_range_size = > MIN_NON_ZERO(blk_get_max_transfer(job->common.blk), > - blk_get_max_transfer(job->target)); > - job->copy_range_size = MAX(job->cluster_size, > - QEMU_ALIGN_UP(job->copy_range_size, > - job->cluster_size)); > > /* Required permissions are already taken with target's blk_new() */ > block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL, [...] > diff --git a/block/trace-events b/block/trace-events > index 04209f058d..ad1454f539 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 *job, int64_t start) "job %p start %"PRId64 > +block_copy_skip_range(void *job, int64_t start, uint64_t bytes) "job %p > start %"PRId64" bytes %"PRId64 > +block_copy_process(void *job, int64_t start) "job %p start %"PRId64 > +block_copy_with_bounce_buffer_read_fail(void *job, int64_t start, int ret) > "job %p start %"PRId64" ret %d" > +block_copy_with_bounce_buffer_write_fail(void *job, int64_t start, int ret) > "job %p start %"PRId64" ret %d" > +block_copy_with_offload_fail(void *job, int64_t start, int ret) "job %p > start %"PRId64" ret %d" The pointer is no longer a job pointer, though. Max > > # ../blockdev.c > qmp_block_job_cancel(void *job) "job %p" >
signature.asc
Description: OpenPGP digital signature