On 01.08.19 13:32, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 14:28, Max Reitz wrote:
>> 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.
>>
>
> I mean, why not to just copy the whole chunk comes in notifier and don't care
> about
> max_transfer? Backup loop copies cluster by cluster anyway, so only notifier
> may copy
> larger chunk.One thing that comes to mind is the hbitmap_get() check in backup_do_cow(). You don’t want to copy everything just because the first cluster needs to be copied. Max
signature.asc
Description: OpenPGP digital signature
