Re: [PATCH v2 7/7] block/block-copy: hide structure definitions

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 27/11/2019 21:08, Vladimir Sementsov-Ogievskiy wrote:
> Hide structure definitions and add explicit API instead, to keep an
> eye on the scope of the shared fields.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block-copy.h | 57 +++------------------------------
>   block/backup-top.c         |  6 ++--
>   block/backup.c             | 27 ++++++++--------
>   block/block-copy.c         | 64 ++++++++++++++++++++++++++++++++++++++
>   4 files changed, 86 insertions(+), 68 deletions(-)
> 
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index d96b097267..753fa663ac 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -18,61 +18,9 @@
>   #include "block/block.h"
>   #include "qemu/co-shared-resource.h"
>   
> -typedef struct BlockCopyInFlightReq {
> -    int64_t offset;
> -    int64_t bytes;
> -    QLIST_ENTRY(BlockCopyInFlightReq) list;
> -    CoQueue wait_queue; /* coroutines blocked on this request */
> -} BlockCopyInFlightReq;
> -
>   typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
>   typedef void (*ProgressResetCallbackFunc)(void *opaque);
> -typedef struct BlockCopyState {
> -    /*
> -     * BdrvChild objects are not owned or managed by block-copy. They are
> -     * provided by block-copy user and user is responsible for appropriate
> -     * permissions on these children.
> -     */
> -    BdrvChild *source;
> -    BdrvChild *target;
> -    BdrvDirtyBitmap *copy_bitmap;
> -    int64_t cluster_size;
> -    bool use_copy_range;
> -    int64_t copy_size;
> -    uint64_t len;
> -    QLIST_HEAD(, BlockCopyInFlightReq) inflight_reqs;
> -
> -    BdrvRequestFlags write_flags;
> -
> -    /*
> -     * skip_unallocated:
> -     *
> -     * Used by sync=top jobs, which first scan the source node for unallocated
> -     * areas and clear them in the copy_bitmap.  During this process, the bitmap
> -     * is thus not fully initialized: It may still have bits set for areas that
> -     * are unallocated and should actually not be copied.
> -     *
> -     * This is indicated by skip_unallocated.
> -     *
> -     * In this case, block_copy() will query the source’s allocation status,
> -     * skip unallocated regions, clear them in the copy_bitmap, and invoke
> -     * block_copy_reset_unallocated() every time it does.
> -     */
> -    bool skip_unallocated;
> -
> -    /* progress_bytes_callback: called when some copying progress is done. */
> -    ProgressBytesCallbackFunc progress_bytes_callback;
> -
> -    /*
> -     * progress_reset_callback: called when some bytes reset from copy_bitmap
> -     * (see @skip_unallocated above). The callee is assumed to recalculate how
> -     * many bytes remain based on the dirty bit count of copy_bitmap.
> -     */
> -    ProgressResetCallbackFunc progress_reset_callback;
> -    void *progress_opaque;
> -
> -    SharedResource *mem;
> -} BlockCopyState;
> +typedef struct BlockCopyState BlockCopyState;
>   
>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>                                        int64_t cluster_size,
> @@ -93,4 +41,7 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
>   int coroutine_fn block_copy(BlockCopyState *s, int64_t start, uint64_t bytes,
>                               bool *error_is_read);
>   
> +BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
> +void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
> +
>   #endif /* BLOCK_COPY_H */
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 7cdb1f8eba..1026628b57 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -38,6 +38,7 @@ typedef struct BDRVBackupTopState {
>       BlockCopyState *bcs;
>       BdrvChild *target;
>       bool active;
> +    int64_t cluster_size;
>   } BDRVBackupTopState;
>   
>   static coroutine_fn int backup_top_co_preadv(
> @@ -51,8 +52,8 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
>                                          uint64_t bytes)
>   {
>       BDRVBackupTopState *s = bs->opaque;
> -    uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
> -    uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
> +    uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->cluster_size);
> +    uint64_t off = QEMU_ALIGN_DOWN(offset, s->cluster_size);
>   
>       return block_copy(s->bcs, off, end - off, NULL);
>   }
> @@ -227,6 +228,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>           goto failed_after_append;
>       }
>   
> +    state->cluster_size = cluster_size;
>       state->bcs = block_copy_state_new(top->backing, state->target,
>                                         cluster_size, write_flags, &local_err);
>       if (local_err) {
> diff --git a/block/backup.c b/block/backup.c
> index cf62b1a38c..acab0d08da 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -48,6 +48,7 @@ typedef struct BackupBlockJob {
>       int64_t cluster_size;
>   
>       BlockCopyState *bcs;
> +    BdrvDirtyBitmap *bcs_bitmap;
>   } BackupBlockJob;
>   
>   static const BlockJobDriver backup_job_driver;
> @@ -63,7 +64,7 @@ static void backup_progress_bytes_callback(int64_t bytes, void *opaque)
>   static void backup_progress_reset_callback(void *opaque)
>   {
>       BackupBlockJob *s = opaque;
> -    uint64_t estimate = bdrv_get_dirty_count(s->bcs->copy_bitmap);
> +    uint64_t estimate = bdrv_get_dirty_count(s->bcs_bitmap);
>   
>       job_progress_set_remaining(&s->common.job, estimate);
>   }
> @@ -111,8 +112,7 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>   
>       if (ret < 0 && job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>           /* If we failed and synced, merge in the bits we didn't copy: */
> -        bdrv_dirty_bitmap_merge_internal(bm, job->bcs->copy_bitmap,
> -                                         NULL, true);
> +        bdrv_dirty_bitmap_merge_internal(bm, job->bcs_bitmap, NULL, true);
>       }
>   }
>   
> @@ -151,7 +151,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
>           return;
>       }
>   
> -    bdrv_set_dirty_bitmap(backup_job->bcs->copy_bitmap, 0, backup_job->len);
> +    bdrv_set_dirty_bitmap(backup_job->bcs_bitmap, 0, backup_job->len);
>   }
>   
>   static BlockErrorAction backup_error_action(BackupBlockJob *job,
> @@ -196,7 +196,7 @@ static int coroutine_fn backup_loop(BackupBlockJob *job)
>       BdrvDirtyBitmapIter *bdbi;
>       int ret = 0;
>   
> -    bdbi = bdrv_dirty_iter_new(job->bcs->copy_bitmap);
> +    bdbi = bdrv_dirty_iter_new(job->bcs_bitmap);
>       while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
>           do {
>               if (yield_and_check(job)) {
> @@ -216,13 +216,13 @@ static int coroutine_fn backup_loop(BackupBlockJob *job)
>       return ret;
>   }
>   
> -static void backup_init_copy_bitmap(BackupBlockJob *job)
> +static void backup_init_bcs_bitmap(BackupBlockJob *job)
>   {
>       bool ret;
>       uint64_t estimate;
>   
>       if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
> -        ret = bdrv_dirty_bitmap_merge_internal(job->bcs->copy_bitmap,
> +        ret = bdrv_dirty_bitmap_merge_internal(job->bcs_bitmap,
>                                                  job->sync_bitmap,
>                                                  NULL, true);
>           assert(ret);
> @@ -232,12 +232,12 @@ static void backup_init_copy_bitmap(BackupBlockJob *job)
>                * We can't hog the coroutine to initialize this thoroughly.
>                * Set a flag and resume work when we are able to yield safely.
>                */
> -            job->bcs->skip_unallocated = true;
> +            block_copy_set_skip_unallocated(job->bcs, true);
>           }
> -        bdrv_set_dirty_bitmap(job->bcs->copy_bitmap, 0, job->len);
> +        bdrv_set_dirty_bitmap(job->bcs_bitmap, 0, job->len);
>       }
>   
> -    estimate = bdrv_get_dirty_count(job->bcs->copy_bitmap);
> +    estimate = bdrv_get_dirty_count(job->bcs_bitmap);
>       job_progress_set_remaining(&job->common.job, estimate);
>   }
>   
> @@ -246,7 +246,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>       BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>       int ret = 0;
>   
> -    backup_init_copy_bitmap(s);
> +    backup_init_bcs_bitmap(s);
>   
>       if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
>           int64_t offset = 0;
> @@ -265,12 +265,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>   
>               offset += count;
>           }
> -        s->bcs->skip_unallocated = false;
> +        block_copy_set_skip_unallocated(s->bcs, false);
>       }
>   
>       if (s->sync_mode == MIRROR_SYNC_MODE_NONE) {
>           /*
> -         * All bits are set in copy_bitmap to allow any cluster to be copied.
> +         * All bits are set in bcs_bitmap to allow any cluster to be copied.
>            * This does not actually require them to be copied.
>            */
>           while (!job_is_cancelled(job)) {
> @@ -458,6 +458,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>       job->sync_bitmap = sync_bitmap;
>       job->bitmap_mode = bitmap_mode;
>       job->bcs = bcs;
> +    job->bcs_bitmap = block_copy_dirty_bitmap(bcs);
>       job->cluster_size = cluster_size;
>       job->len = len;
>   
> diff --git a/block/block-copy.c b/block/block-copy.c
> index aca44b13fb..7e14e86a2d 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -24,6 +24,60 @@
>   #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
>   #define BLOCK_COPY_MAX_MEM (128 * MiB)
>   
> +typedef struct BlockCopyInFlightReq {
> +    int64_t offset;
> +    int64_t bytes;
> +    QLIST_ENTRY(BlockCopyInFlightReq) list;
> +    CoQueue wait_queue; /* coroutines blocked on this request */
> +} BlockCopyInFlightReq;
> +
> +typedef struct BlockCopyState {
> +    /*
> +     * BdrvChild objects are not owned or managed by block-copy. They are
> +     * provided by block-copy user and user is responsible for appropriate
> +     * permissions on these children.
> +     */
> +    BdrvChild *source;
> +    BdrvChild *target;
> +    BdrvDirtyBitmap *copy_bitmap;
> +    int64_t cluster_size;
> +    bool use_copy_range;
> +    int64_t copy_size;
> +    uint64_t len;
> +    QLIST_HEAD(, BlockCopyInFlightReq) inflight_reqs;
> +
> +    BdrvRequestFlags write_flags;
> +
> +    /*
> +     * skip_unallocated:
> +     *
> +     * Used by sync=top jobs, which first scan the source node for unallocated
> +     * areas and clear them in the copy_bitmap.  During this process, the bitmap
> +     * is thus not fully initialized: It may still have bits set for areas that
> +     * are unallocated and should actually not be copied.
> +     *
> +     * This is indicated by skip_unallocated.
> +     *
> +     * In this case, block_copy() will query the source’s allocation status,
> +     * skip unallocated regions, clear them in the copy_bitmap, and invoke
> +     * block_copy_reset_unallocated() every time it does.
> +     */
> +    bool skip_unallocated;
> +
> +    /* progress_bytes_callback: called when some copying progress is done. */
> +    ProgressBytesCallbackFunc progress_bytes_callback;
> +
> +    /*
> +     * progress_reset_callback: called when some bytes reset from copy_bitmap
> +     * (see @skip_unallocated above). The callee is assumed to recalculate how
> +     * many bytes remain based on the dirty bit count of copy_bitmap.
> +     */
> +    ProgressResetCallbackFunc progress_reset_callback;
> +    void *progress_opaque;
> +
> +    SharedResource *mem;
> +} BlockCopyState;
> +
>   static BlockCopyInFlightReq *block_copy_find_inflight_req(BlockCopyState *s,
>                                                             int64_t offset,
>                                                             int64_t bytes)
> @@ -492,3 +546,13 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, uint64_t bytes,
>   
>       return 0;
>   }
> +
> +BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s)
> +{
> +    return s->copy_bitmap;
> +}
> +
> +void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip)
> +{
> +    s->skip_unallocated = skip;
> +}
> 

The idea of API is OK but we have got the duplicated members in the 
nested structures.

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich