[Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are two small fixes.

01 is not a degradation at all, so it's OK for 4.2
02 is degradation of 3.0, so it's possibly OK for 4.2 too,
   but it seems to be real bug and fix is very simple, so,
   may be 4.1 is better

Or you may take the whole series to 4.1 if you want.

Vladimir Sementsov-Ogievskiy (3):
  block/backup: deal with zero detection
  block/backup: disable copy_range for compressed backup
  block/backup: refactor write_flags

 block/backup.c | 31 ++++++++++++++-----------------
 blockdev.c     |  8 ++++----
 2 files changed, 18 insertions(+), 21 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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);
     }
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[John Snow]

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 <vsementsov@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

Re: [Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[John Snow]

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 <vsementsov@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!

Re: [Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3850 bytes --]

On 30.07.19 20: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 <vsementsov@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.

I don’t quite know why, because this is just a block job specific
question and doesn’t have much to do with the rest of the block layer,
but OK. :-)

drive-backup always set BDRV_REQ_MAY_UNMAP, as you can see.  Maybe that
didn’t do anything because the target wasn’t opened with discard=unmap.
 But to me, it’s clear that the intention was to indeed unmap the areas
in the target (it isn’t like the user had a choice of opening the target
with discard=unmap or not).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block/backup: deal with zero detection

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 552 bytes --]

On 30.07.19 18:32, 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 <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 15 ++++++---------
>  blockdev.c     |  8 ++++----
>  2 files changed, 10 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 2/3] block/backup: disable copy_range for compressed backup

[Vladimir Sementsov-Ogievskiy]

Enabled by default copy_range ignores compress option. It's definitely
unexpected for user.

It's broken since introduction of copy_range usage in backup in
9ded4a011496.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index f4aaf08df3..c5f941101a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -645,7 +645,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->cluster_size = cluster_size;
     job->copy_bitmap = copy_bitmap;
     copy_bitmap = NULL;
-    job->use_copy_range = true;
+    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,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/3] block/backup: disable copy_range for compressed backup

[John Snow]

On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> Enabled by default copy_range ignores compress option. It's definitely
> unexpected for user.
> 
> It's broken since introduction of copy_range usage in backup in
> 9ded4a011496.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index f4aaf08df3..c5f941101a 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -645,7 +645,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>      job->cluster_size = cluster_size;
>      job->copy_bitmap = copy_bitmap;
>      copy_bitmap = NULL;
> -    job->use_copy_range = true;
> +    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,
> 

Agree, these aren't compatible options. Is this worth a note in
docs/interop/live-block-operations.rst?

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] block/backup: disable copy_range for compressed backup

[Vladimir Sementsov-Ogievskiy]

30.07.2019 21:22, John Snow wrote:
> 
> 
> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Enabled by default copy_range ignores compress option. It's definitely
>> unexpected for user.
>>
>> It's broken since introduction of copy_range usage in backup in
>> 9ded4a011496.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/backup.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index f4aaf08df3..c5f941101a 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -645,7 +645,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>       job->cluster_size = cluster_size;
>>       job->copy_bitmap = copy_bitmap;
>>       copy_bitmap = NULL;
>> -    job->use_copy_range = true;
>> +    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,
>>
> 
> Agree, these aren't compatible options. Is this worth a note in
> docs/interop/live-block-operations.rst?

Use_copy_range is not an user-visible option and it's not documented as I can see,
so I think we don't need.

> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] block/backup: disable copy_range for compressed backup

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 451 bytes --]

On 30.07.19 18:32, Vladimir Sementsov-Ogievskiy wrote:
> Enabled by default copy_range ignores compress option. It's definitely
> unexpected for user.
> 
> It's broken since introduction of copy_range usage in backup in
> 9ded4a011496.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/3] block/backup: disable copy_range for compressed backup

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

On 30.07.19 18:32, Vladimir Sementsov-Ogievskiy wrote:
> Enabled by default copy_range ignores compress option. It's definitely
> unexpected for user.
> 
> It's broken since introduction of copy_range usage in backup in
> 9ded4a011496.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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;
-- 
2.18.0

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[John Snow]

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 <vsementsov@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?

I had just assumed it wouldn't work quite right because of the nature of
copy offloading, but I don't actually know what it does do.

This seems like a pretty minor cleanup, but why not. I like dropping
extra fields when I can:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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?


> 
> I had just assumed it wouldn't work quite right because of the nature of
> copy offloading, but I don't actually know what it does do.
> 
> This seems like a pretty minor cleanup, but why not. I like dropping
> extra fields when I can:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5098 bytes --]

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 <vsementsov@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5836 bytes --]

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 <vsementsov@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

01.08.2019 14:37, Max Reitz wrote:
> 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 <vsementsov@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.
> 

Hmm, but seems that we do exactly this, and this is wrong. But this is separate thing..


About copy_range, I just don't sure that max_transfer is a true restriction for copy_range.
For example, for file_posix max_transfer comes from some specific ioctl or from sysfs.. Is
it appropriate as limitation for copy_file_range?

Also, Max, could you please take a look at "[PATCH v3] blockjob: drain all job nodes in block_job_drain"
thread? Maybe, what John questions is obvious for you.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 9009 bytes --]

On 01.08.19 14:02, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 14:37, Max Reitz wrote:
>> 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 <vsementsov@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.
>>
> 
> Hmm, but seems that we do exactly this, and this is wrong. But this is separate thing..

You’re right.  It’s totally broken.  Nice.

The following gets me a nice content mismatch:

$ ./qemu-img create -f qcow2 src.qcow2 2M
$ ./qemu-io -c 'write -P 42 0 2M' src.qcow2
$ cp src.qcow2 ref.qcow2

{"execute":"qmp_capabilities"}
{"return": {}}
{"execute":"blockdev-add","arguments":
 {"node-name":"src","driver":"qcow2",
  "file":{"driver":"file","filename":"src.qcow2"}}}
{"return": {}}
{"execute":"drive-backup","arguments":
 {"device":"src","job-id":"backup","target":"tgt.qcow2",
  "format":"qcow2","sync":"full","speed":1048576}}
{"timestamp": {"seconds": 1564661742, "microseconds": 268384},
 "event": "JOB_STATUS_CHANGE",
  "data": {"status": "created", "id": "backup"}}
{"timestamp": {"seconds": 1564661742, "microseconds": 268436},
 "event": "JOB_STATUS_CHANGE",
  "data": {"status": "running", "id": "backup"}}
{"return": {}}
{"execute":"human-monitor-command",
 "arguments":{"command-line":
              "qemu-io src \"write -P 23 1114112 65536\""}}
{"return": ""}
{"execute":"human-monitor-command",
 "arguments":{"command-line":
              "qemu-io src \"write -P 66 1048576 1M\""}}
{"return": ""}
{"timestamp": {"seconds": 1564661744, "microseconds": 278362}.
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "waiting", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278534},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "pending", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278778},
 "event": "BLOCK_JOB_COMPLETED",
 "data": {"device": "backup", "len": 2097152, "offset": 2162688,
          "speed": 1048576, "type": "backup"}}
{"execute":"quit"}
{"timestamp": {"seconds": 1564661744, "microseconds": 278884},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "concluded", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278946},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "null", "id": "backup"}}
{"return": {}}

$ ./qemu-img compare src.qcow2
Content mismatch at offset 1114112!


Aww maaan.  Setting copy_range to false “fixes” it.  I guess this’ll
need to be fixed for 4.1. :-/

> About copy_range, I just don't sure that max_transfer is a true restriction for copy_range.
> For example, for file_posix max_transfer comes from some specific ioctl or from sysfs.. Is
> it appropriate as limitation for copy_file_range?

Who knows, but it’s probably the best approximation we have.

> Also, Max, could you please take a look at "[PATCH v3] blockjob: drain all job nodes in block_job_drain"
> thread? Maybe, what John questions is obvious for you.

Perhaps after fixing backup. :-/

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Vladimir Sementsov-Ogievskiy]

01.08.2019 15:21, Max Reitz wrote:
> On 01.08.19 14:02, Vladimir Sementsov-Ogievskiy wrote:
>> 01.08.2019 14:37, Max Reitz wrote:
>>> 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 <vsementsov@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.
>>>
>>
>> Hmm, but seems that we do exactly this, and this is wrong. But this is separate thing..
> 
> You’re right.  It’s totally broken.  Nice.
> 
> The following gets me a nice content mismatch:
> 
> $ ./qemu-img create -f qcow2 src.qcow2 2M
> $ ./qemu-io -c 'write -P 42 0 2M' src.qcow2
> $ cp src.qcow2 ref.qcow2
> 
> {"execute":"qmp_capabilities"}
> {"return": {}}
> {"execute":"blockdev-add","arguments":
>   {"node-name":"src","driver":"qcow2",
>    "file":{"driver":"file","filename":"src.qcow2"}}}
> {"return": {}}
> {"execute":"drive-backup","arguments":
>   {"device":"src","job-id":"backup","target":"tgt.qcow2",
>    "format":"qcow2","sync":"full","speed":1048576}}
> {"timestamp": {"seconds": 1564661742, "microseconds": 268384},
>   "event": "JOB_STATUS_CHANGE",
>    "data": {"status": "created", "id": "backup"}}
> {"timestamp": {"seconds": 1564661742, "microseconds": 268436},
>   "event": "JOB_STATUS_CHANGE",
>    "data": {"status": "running", "id": "backup"}}
> {"return": {}}
> {"execute":"human-monitor-command",
>   "arguments":{"command-line":
>                "qemu-io src \"write -P 23 1114112 65536\""}}
> {"return": ""}
> {"execute":"human-monitor-command",
>   "arguments":{"command-line":
>                "qemu-io src \"write -P 66 1048576 1M\""}}
> {"return": ""}
> {"timestamp": {"seconds": 1564661744, "microseconds": 278362}.
>   "event": "JOB_STATUS_CHANGE",
>   "data": {"status": "waiting", "id": "backup"}}
> {"timestamp": {"seconds": 1564661744, "microseconds": 278534},
>   "event": "JOB_STATUS_CHANGE",
>   "data": {"status": "pending", "id": "backup"}}
> {"timestamp": {"seconds": 1564661744, "microseconds": 278778},
>   "event": "BLOCK_JOB_COMPLETED",
>   "data": {"device": "backup", "len": 2097152, "offset": 2162688,
>            "speed": 1048576, "type": "backup"}}
> {"execute":"quit"}
> {"timestamp": {"seconds": 1564661744, "microseconds": 278884},
>   "event": "JOB_STATUS_CHANGE",
>   "data": {"status": "concluded", "id": "backup"}}
> {"timestamp": {"seconds": 1564661744, "microseconds": 278946},
>   "event": "JOB_STATUS_CHANGE",
>   "data": {"status": "null", "id": "backup"}}
> {"return": {}}
> 
> $ ./qemu-img compare src.qcow2
> Content mismatch at offset 1114112!
> 
> 
> Aww maaan.  Setting copy_range to false “fixes” it.  I guess this’ll
> need to be fixed for 4.1. :-/

Oops yes. Writing "this is a separate thing..", I thought that it's not very bad to
copy more than needed, but actually we must not copy clusters not-dirty in copy_bitmap,
as they may already be rewritten by the guest.

> 
>> About copy_range, I just don't sure that max_transfer is a true restriction for copy_range.
>> For example, for file_posix max_transfer comes from some specific ioctl or from sysfs.. Is
>> it appropriate as limitation for copy_file_range?
> 
> Who knows, but it’s probably the best approximation we have.

Ok. Than I'll try to move handling of max_transfer for copy_range into block/io, like it's done for
write.

> 
>> Also, Max, could you please take a look at "[PATCH v3] blockjob: drain all job nodes in block_job_drain"
>> thread? Maybe, what John questions is obvious for you.
> 
> Perhaps after fixing backup. :-/
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 448 bytes --]

On 30.07.19 18:32, 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 <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[John Snow]

On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here are two small fixes.
> 
> 01 is not a degradation at all, so it's OK for 4.2
> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>    but it seems to be real bug and fix is very simple, so,
>    may be 4.1 is better
> 
> Or you may take the whole series to 4.1 if you want.
> 

I think (1) and (2) can go in for stable after review, but they're not
crucial for 4.1 especially at this late of a stage. Should be cataclysms
only right now.

--js

Re: [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[Vladimir Sementsov-Ogievskiy]

30.07.2019 21:41, John Snow wrote:
> 
> 
> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here are two small fixes.
>>
>> 01 is not a degradation at all, so it's OK for 4.2
>> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>>     but it seems to be real bug and fix is very simple, so,
>>     may be 4.1 is better
>>
>> Or you may take the whole series to 4.1 if you want.
>>
> 
> I think (1) and (2) can go in for stable after review, but they're not
> crucial for 4.1 especially at this late of a stage. Should be cataclysms
> only right now.
> 
> --js
> 

I can rebase it than on your bitmaps branch. Or, if we want it for stable, maybe,
I shouldn't?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[John Snow]

On 7/31/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:41, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here are two small fixes.
>>>
>>> 01 is not a degradation at all, so it's OK for 4.2
>>> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>>>     but it seems to be real bug and fix is very simple, so,
>>>     may be 4.1 is better
>>>
>>> Or you may take the whole series to 4.1 if you want.
>>>
>>
>> I think (1) and (2) can go in for stable after review, but they're not
>> crucial for 4.1 especially at this late of a stage. Should be cataclysms
>> only right now.
>>
>> --js
>>
> 
> I can rebase it than on your bitmaps branch. Or, if we want it for stable, maybe,
> I shouldn't?
> 

Good point. Keep it based on main and I'll slip it in at the beginning
of the staging queue.

--js

Re: [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[John Snow]

On 7/31/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:41, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here are two small fixes.
>>>
>>> 01 is not a degradation at all, so it's OK for 4.2
>>> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>>>     but it seems to be real bug and fix is very simple, so,
>>>     may be 4.1 is better
>>>
>>> Or you may take the whole series to 4.1 if you want.
>>>
>>
>> I think (1) and (2) can go in for stable after review, but they're not
>> crucial for 4.1 especially at this late of a stage. Should be cataclysms
>> only right now.

You found a cataclysm :(

>>
>> --js
>>
> 
> I can rebase it than on your bitmaps branch. Or, if we want it for stable, maybe,
> I shouldn't?
> 

I rebased these two patches (1 and 3) on top of bitmaps, on top of rc4.

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js