On 31.07.19 18:01, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:28, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> write flags are constant, let's store it in BackupBlockJob instead of
>>> recalculating. It also makes two boolean fields to be unused, so,
>>> drop them.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>> ---
>>> block/backup.c | 24 ++++++++++++------------
>>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index c5f941101a..4651649e9d 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -47,7 +47,6 @@ typedef struct BackupBlockJob {
>>> uint64_t len;
>>> uint64_t bytes_read;
>>> int64_t cluster_size;
>>> - bool compress;
>>> NotifierWithReturn before_write;
>>> QLIST_HEAD(, CowRequest) inflight_reqs;
>>>
>>> @@ -55,7 +54,7 @@ typedef struct BackupBlockJob {
>>> bool use_copy_range;
>>> int64_t copy_range_size;
>>>
>>> - bool serialize_target_writes;
>>> + BdrvRequestFlags write_flags;
>>> } BackupBlockJob;
>>>
>>> static const BlockJobDriver backup_job_driver;
>>> @@ -110,10 +109,6 @@ static int coroutine_fn
>>> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>> BlockBackend *blk = job->common.blk;
>>> int nbytes;
>>> int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>>> - int write_flags =
>>> - (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) |
>>> - (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
>>> -
>>>
>>> assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>> hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
>>> @@ -132,7 +127,7 @@ static int coroutine_fn
>>> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>> }
>>>
>>> ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
>>> - write_flags);
>>> + job->write_flags);
>>> if (ret < 0) {
>>> trace_backup_do_cow_write_fail(job, start, ret);
>>> if (error_is_read) {
>>> @@ -160,7 +155,6 @@ static int coroutine_fn
>>> backup_cow_with_offload(BackupBlockJob *job,
>>> BlockBackend *blk = job->common.blk;
>>> int nbytes;
>>> int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>>> - int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING
>>> : 0;
>>>
>>> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>>> assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>> @@ -168,7 +162,7 @@ static int coroutine_fn
>>> backup_cow_with_offload(BackupBlockJob *job,
>>> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>>> hbitmap_reset(job->copy_bitmap, start, job->cluster_size *
>>> nr_clusters);
>>> ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
>>> - read_flags, write_flags);
>>> + read_flags, job->write_flags);
>>> if (ret < 0) {
>>> trace_backup_do_cow_copy_range_fail(job, start, ret);
>>> hbitmap_set(job->copy_bitmap, start, job->cluster_size *
>>> nr_clusters);
>>> @@ -638,10 +632,16 @@ BlockJob *backup_job_create(const char *job_id,
>>> BlockDriverState *bs,
>>> job->sync_mode = sync_mode;
>>> job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
>>> sync_bitmap : NULL;
>>> - job->compress = compress;
>>>
>>> - /* Detect image-fleecing (and similar) schemes */
>>> - job->serialize_target_writes = bdrv_chain_contains(target, 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->cluster_size = cluster_size;
>>> job->copy_bitmap = copy_bitmap;
>>> copy_bitmap = NULL;
>>>
>>
>> What happens if you did pass BDRV_REQ_WRITE_COMPRESSED to
>> blk_co_copy_range? Is that rejected somewhere in the stack?
>
>
> Hmm, I'm afraid that it will be silently ignored.
>
> And I have one question related to copy offload too.
>
> Do we really need to handle max_transfer in backup code for copy offload?
> Is max_transfer related to it really?
>
> Anyway, bl.max_transfer should be handled in generic copy_range code in
> block/io.c
> (if it should at all), I'm going to fix it, but may be, I can just drop this
> limitation
> from backup?On a quick glance, it doesn’t look like a limitation to me but actually like the opposite. backup_cow_with_bounce_buffer() only copies up to cluster_size, whereas backup_cow_with_offload() will copy up to the maximum transfer size permitted by both source and target for copy offloading. Max
signature.asc
Description: OpenPGP digital signature
