* [Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes
@ 2019-09-17 16:07 Vladimir Sementsov-Ogievskiy
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range Vladimir Sementsov-Ogievskiy
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-17 16:07 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, qemu-stable, mreitz, den, jsnow
Hi all!
Here are two small fixes. They fixes old commit, so qemu-stable is in
CC, but actually, I don't think they are critical.
01: is new
02: is just copied from
[PATCH v11 01/14] block/backup: fix backup_cow_with_offload for last cluster
(I only add Fixes: to commit-message)
and this is why I called this v12, to not interfere with previous
emails
I'd prefer this to go through Max's block branch, as Max is reviewing my
backup-top series, which will refer to these patches and seems simpler
to queue them all together.
Vladimir Sementsov-Ogievskiy (2):
block/backup: fix max_transfer handling for copy_range
block/backup: fix backup_cow_with_offload for last cluster
block/backup.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
2019-09-17 16:07 [Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes Vladimir Sementsov-Ogievskiy
@ 2019-09-17 16:07 ` Vladimir Sementsov-Ogievskiy
2019-09-18 19:57 ` John Snow
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-17 16:07 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, qemu-stable, mreitz, den, jsnow
Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
are trying to find aligned size which satisfy both source and target.
Also, don't ignore too small max_transfer. In this case seems safer to
disable copy_range.
Fixes: 9ded4a0114968e
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/backup.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..d8fdbfadfe 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -741,12 +741,16 @@ 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 = !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,
- QEMU_ALIGN_UP(job->copy_range_size,
- job->cluster_size));
+ job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
+ job->cluster_size);
+ /*
+ * Compression is not supported for copy_range. Also, we don't want to
+ * handle small max_transfer for copy_range (which currently don't
+ * handle max_transfer at all).
+ */
+ job->use_copy_range = !compress && job->copy_range_size > 0;
/* Required permissions are already taken with target's blk_new() */
block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster
2019-09-17 16:07 [Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes Vladimir Sementsov-Ogievskiy
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range Vladimir Sementsov-Ogievskiy
@ 2019-09-17 16:07 ` Vladimir Sementsov-Ogievskiy
2019-09-18 20:14 ` John Snow
1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-17 16:07 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, qemu-stable, mreitz, den, jsnow
We shouldn't try to copy bytes beyond EOF. Fix it.
Fixes: 9ded4a0114968e
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/backup.c b/block/backup.c
index d8fdbfadfe..89f7f89200 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
assert(QEMU_IS_ALIGNED(start, job->cluster_size));
- nbytes = MIN(job->copy_range_size, end - start);
+ nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
job->cluster_size * nr_clusters);
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range Vladimir Sementsov-Ogievskiy
@ 2019-09-18 19:57 ` John Snow
2019-09-19 6:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2019-09-18 19:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, den, mreitz, qemu-devel, qemu-stable
On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
> are trying to find aligned size which satisfy both source and target.
> Also, don't ignore too small max_transfer. In this case seems safer to
> disable copy_range.
>
> Fixes: 9ded4a0114968e
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/backup.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6..d8fdbfadfe 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -741,12 +741,16 @@ 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 = !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,
> - QEMU_ALIGN_UP(job->copy_range_size,
> - job->cluster_size));
> + job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
> + job->cluster_size);
> + /*
> + * Compression is not supported for copy_range. Also, we don't want to
> + * handle small max_transfer for copy_range (which currently don't
> + * handle max_transfer at all).
> + */
> + job->use_copy_range = !compress && job->copy_range_size > 0;
>
> /* Required permissions are already taken with target's blk_new() */
> block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>
I'm clear on the alignment fix, I'm not clear on the comment about
max_transfer and how it relates to copy_range_size being non-zero.
"small max transfer" -- what happens when it's zero? we're apparently OK
with a single cluster, but when it's zero, what happens?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster Vladimir Sementsov-Ogievskiy
@ 2019-09-18 20:14 ` John Snow
2019-09-19 7:02 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2019-09-18 20:14 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, den, mreitz, qemu-devel, qemu-stable
On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
> We shouldn't try to copy bytes beyond EOF. Fix it.
>
> Fixes: 9ded4a0114968e
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> block/backup.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index d8fdbfadfe..89f7f89200 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>
> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
> assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> - nbytes = MIN(job->copy_range_size, end - start);
> + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
I'm a little confused. I think the patch as written is correct, but I
don't know what problem it solves.
If we're going to allow the caller to pass in an end that's beyond EOF,
does that mean we are *requiring* the caller to pass in a value that's
aligned?
We should probably assert what kind of a value we're accepted here,
right? We do for start, but should we for 'end' as well?
Then ...
> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
Don't we just round this right back up immediately anyway? Does that
mean we have callers that are passing in an 'end' that's more than 1
job-cluster beyond EOF? That seems like something that should be fixed
in the caller, surely?
> bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
> job->cluster_size * nr_clusters);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
2019-09-18 19:57 ` John Snow
@ 2019-09-19 6:50 ` Vladimir Sementsov-Ogievskiy
2019-09-20 1:13 ` John Snow
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-19 6:50 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, Denis Lunev, mreitz, qemu-devel, qemu-stable
18.09.2019 22:57, John Snow wrote:
>
>
> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>> are trying to find aligned size which satisfy both source and target.
>> Also, don't ignore too small max_transfer. In this case seems safer to
>> disable copy_range.
>>
>> Fixes: 9ded4a0114968e
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/backup.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index 763f0d7ff6..d8fdbfadfe 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -741,12 +741,16 @@ 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 = !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,
>> - QEMU_ALIGN_UP(job->copy_range_size,
>> - job->cluster_size));
>> + job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>> + job->cluster_size);
>> + /*
>> + * Compression is not supported for copy_range. Also, we don't want to
>> + * handle small max_transfer for copy_range (which currently don't
>> + * handle max_transfer at all).
>> + */
>> + job->use_copy_range = !compress && job->copy_range_size > 0;
>> /* Required permissions are already taken with target's blk_new() */
>> block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>
>
> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
>
> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?
if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster
2019-09-18 20:14 ` John Snow
@ 2019-09-19 7:02 ` Vladimir Sementsov-Ogievskiy
2019-09-20 0:55 ` John Snow
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-19 7:02 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, Denis Lunev, mreitz, qemu-devel, qemu-stable
18.09.2019 23:14, John Snow wrote:
>
>
> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>> We shouldn't try to copy bytes beyond EOF. Fix it.
>>
>> Fixes: 9ded4a0114968e
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/backup.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index d8fdbfadfe..89f7f89200 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>> assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>> - nbytes = MIN(job->copy_range_size, end - start);
>> + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
>
> I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves.
last cluster may exceed EOF. And backup_do_cow (who calls backup_cow_with_offload) rounds all to clusters.
It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing.
>
> If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned?
Actually yes, as we are resetting dirty bitmap.
>
> We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well?
Yes assertion may be added.
>
> Then ...
>
>> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>
> Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely?
nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers.
nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF.
Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes.
Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix.
>
>> bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
>> job->cluster_size * nr_clusters);
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster
2019-09-19 7:02 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-20 0:55 ` John Snow
0 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2019-09-20 0:55 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, Denis Lunev, mreitz, qemu-devel, qemu-stable
On 9/19/19 3:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.09.2019 23:14, John Snow wrote:
>>
>>
>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> We shouldn't try to copy bytes beyond EOF. Fix it.
>>>
>>> Fixes: 9ded4a0114968e
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> block/backup.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index d8fdbfadfe..89f7f89200 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
>>> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>>> assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>> - nbytes = MIN(job->copy_range_size, end - start);
>>> + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start);
>>
>> I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves.
>
> last cluster may exceed EOF. And backup_do_cow (who calls backup_cow_with_offload) rounds all to clusters.
> It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing.
>
>>
>> If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned?
>
> Actually yes, as we are resetting dirty bitmap.
>
>>
>> We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well?
>
> Yes assertion may be added.
>
>>
>> Then ...
>>
>>> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>>
>> Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely?
>
> nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers.
> nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF.
>
Ah, right, right ... I *was* confused. We don't use nr_clusters for the
IO itself, just the bitmap. So we effectively re-calculate aligned and
unaligned values for use in different places.
> Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes.
>
> Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix.
>
>>
>>> bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
>>> job->cluster_size * nr_clusters);
>>>
>>
>
>
We should make the interface here a little more clear I think, but what
you wrote is correct.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
2019-09-19 6:50 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-20 1:13 ` John Snow
2019-09-20 7:52 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2019-09-20 1:13 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, Denis Lunev, mreitz, qemu-devel, qemu-stable
On 9/19/19 2:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.09.2019 22:57, John Snow wrote:
>>
>>
>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>>> are trying to find aligned size which satisfy both source and target.
>>> Also, don't ignore too small max_transfer. In this case seems safer to
>>> disable copy_range.
>>>
>>> Fixes: 9ded4a0114968e
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> block/backup.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6..d8fdbfadfe 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -741,12 +741,16 @@ 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 = !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,
>>> - QEMU_ALIGN_UP(job->copy_range_size,
>>> - job->cluster_size));
>>> + job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>>> + job->cluster_size);
>>> + /*
>>> + * Compression is not supported for copy_range. Also, we don't want to
>>> + * handle small max_transfer for copy_range (which currently don't
>>> + * handle max_transfer at all).
>>> + */
>>> + job->use_copy_range = !compress && job->copy_range_size > 0;
>>> /* Required permissions are already taken with target's blk_new() */
>>> block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>>
>>
>> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
>>
>> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?
>
> if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
> Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).
>
oh, I'm ... sorry, I just didn't quite understand the comment.
You're just making sure copy_range after all of our checks is non-zero,
plain and simple. If max_transfer was *smaller than a job cluster*, we
might end up with a copy_range size that's zero, which is obviously...
not useful.
So, I might phrase "Also, we don't want to..." as:
"copy_range does not respect max_transfer, so we factor that in here. If
it's smaller than the job->cluster_size, we are unable to use copy_range."
Just a suggestion, though, so:
Reviewed-by: John Snow <jsnow@redhat.com>
(SHOULD copy_range respect max_transfer? I guess it would be quite
different -- it would only count things it had to fall back and actually
*transfer*, right? I suppose that because it can have that fallback we
need to accommodate it here in backup.c, hence this workaround clamp.)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
2019-09-20 1:13 ` John Snow
@ 2019-09-20 7:52 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-20 7:52 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, Denis Lunev, mreitz, qemu-devel, qemu-stable
20.09.2019 4:13, John Snow wrote:
>
>
> On 9/19/19 2:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 18.09.2019 22:57, John Snow wrote:
>>>
>>>
>>> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we
>>>> are trying to find aligned size which satisfy both source and target.
>>>> Also, don't ignore too small max_transfer. In this case seems safer to
>>>> disable copy_range.
>>>>
>>>> Fixes: 9ded4a0114968e
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>> block/backup.c | 12 ++++++++----
>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/block/backup.c b/block/backup.c
>>>> index 763f0d7ff6..d8fdbfadfe 100644
>>>> --- a/block/backup.c
>>>> +++ b/block/backup.c
>>>> @@ -741,12 +741,16 @@ 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 = !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,
>>>> - QEMU_ALIGN_UP(job->copy_range_size,
>>>> - job->cluster_size));
>>>> + job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size,
>>>> + job->cluster_size);
>>>> + /*
>>>> + * Compression is not supported for copy_range. Also, we don't want to
>>>> + * handle small max_transfer for copy_range (which currently don't
>>>> + * handle max_transfer at all).
>>>> + */
>>>> + job->use_copy_range = !compress && job->copy_range_size > 0;
>>>> /* Required permissions are already taken with target's blk_new() */
>>>> block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>>>>
>>>
>>> I'm clear on the alignment fix, I'm not clear on the comment about max_transfer and how it relates to copy_range_size being non-zero.
>>>
>>> "small max transfer" -- what happens when it's zero? we're apparently OK with a single cluster, but when it's zero, what happens?
>>
>> if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case.
>> Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it).
>>
>
> oh, I'm ... sorry, I just didn't quite understand the comment.
>
> You're just making sure copy_range after all of our checks is non-zero,
> plain and simple. If max_transfer was *smaller than a job cluster*, we
> might end up with a copy_range size that's zero, which is obviously...
> not useful.
>
> So, I might phrase "Also, we don't want to..." as:
>
> "copy_range does not respect max_transfer, so we factor that in here. If
> it's smaller than the job->cluster_size, we are unable to use copy_range."
We actually able to: just using a loop and calling copy_range several times. May be just:
copy_range does not respect max_transfer, so we factor that in here. If
it's smaller than the job->cluster_size, we do not use copy_range.
>
> Just a suggestion, though, so:
>
> Reviewed-by: John Snow <jsnow@redhat.com>
>
>
> (SHOULD copy_range respect max_transfer? I guess it would be quite
> different -- it would only count things it had to fall back and actually
> *transfer*, right? I suppose that because it can have that fallback we
> need to accommodate it here in backup.c, hence this workaround clamp.)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-20 7:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-17 16:07 [Qemu-devel] [PATCH v12 0/2] backup: copy_range fixes Vladimir Sementsov-Ogievskiy
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range Vladimir Sementsov-Ogievskiy
2019-09-18 19:57 ` John Snow
2019-09-19 6:50 ` Vladimir Sementsov-Ogievskiy
2019-09-20 1:13 ` John Snow
2019-09-20 7:52 ` Vladimir Sementsov-Ogievskiy
2019-09-17 16:07 ` [Qemu-devel] [PATCH v12 2/2] block/backup: fix backup_cow_with_offload for last cluster Vladimir Sementsov-Ogievskiy
2019-09-18 20:14 ` John Snow
2019-09-19 7:02 ` Vladimir Sementsov-Ogievskiy
2019-09-20 0:55 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.