On 7/31/19 6:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:40, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> We have detect_zeroes option, so at least for blockdev-backup user
>>> should define it if zero-detection is needed. For drive-backup leave
>>> detection enabled by default but do it through existing option instead
>>> of open-coding.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>> ---
>>> block/backup.c | 15 ++++++---------
>>> blockdev.c | 8 ++++----
>>> 2 files changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 715e1d3be8..f4aaf08df3 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -110,7 +110,10 @@ 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;
>>> + 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);
>>> @@ -128,14 +131,8 @@ static int coroutine_fn
>>> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>> goto fail;
>>> }
>>>
>>> - if (buffer_is_zero(*bounce_buffer, nbytes)) {
>>> - ret = blk_co_pwrite_zeroes(job->target, start,
>>> - nbytes, write_flags |
>>> BDRV_REQ_MAY_UNMAP);
>>> - } else {
>>> - ret = blk_co_pwrite(job->target, start,
>>> - nbytes, *bounce_buffer, write_flags |
>>> - (job->compress ? BDRV_REQ_WRITE_COMPRESSED :
>>> 0));
>>> - }
>>> + ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
>>> + write_flags);
>>> if (ret < 0) {
>>> trace_backup_do_cow_write_fail(job, start, ret);
>>> if (error_is_read) {
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 4d141e9a1f..a94d754504 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3434,7 +3434,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup,
>>> JobTxn *txn,
>>> BlockJob *job = NULL;
>>> BdrvDirtyBitmap *bmap = NULL;
>>> AioContext *aio_context;
>>> - QDict *options = NULL;
>>> + QDict *options;
>>> Error *local_err = NULL;
>>> int flags, job_flags = JOB_DEFAULT;
>>> int64_t size;
>>> @@ -3529,10 +3529,10 @@ static BlockJob *do_drive_backup(DriveBackup
>>> *backup, JobTxn *txn,
>>> goto out;
>>> }
>>>
>>> + options = qdict_new();
>>> + qdict_put_str(options, "discard", "unmap");
>>> + qdict_put_str(options, "detect-zeroes", "unmap");
>>> if (backup->format) {
>>> - if (!options) {
>>> - options = qdict_new();
>>> - }
>>> qdict_put_str(options, "driver", backup->format);
>>> }
>>>
>>>
>>
>> I'm less sure of this one personally. Is it right to always try to set
>> unmap on the target?
>>
>> I like the idea of removing special cases and handling things more
>> centrally though, but I'll want Max (or Kevin) to take a peek.
>>
>> --js
>>
>
>
> If nobody minds I'd agree with you to drop zero detecting from both backups.
>
I'm not sure it's WRONG either!