Re: [Qemu-block] [PATCH 3/8] block/io: handle alignment and max_transfer for copy_range

2019-08-09 Thread Vladimir Sementsov-Ogievskiy
07.08.2019 20:28, Max Reitz wrote:
> On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
>> copy_range ignores these limitations, let's improve it. block/backup
>> code handles max_transfer for copy_range by itself, now it's not needed
>> more, drop it.
> 
> Shouldn’t this be two separate patches?

Not a problem, will do.

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/backup.c | 11 ++-
>>   block/io.c | 41 +
>>   2 files changed, 35 insertions(+), 17 deletions(-)
> 
> [...]
> 
>> diff --git a/block/io.c b/block/io.c
>> index 06305c6ea6..5abbd0fff2 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3005,6 +3005,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>>   {
>>   BdrvTrackedRequest req;
>>   int ret;
>> +uint32_t align = MAX(src->bs->bl.request_alignment,
>> + dst->bs->bl.request_alignment);
>> +uint32_t max_transfer =
>> +
>> QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
>> +  
>> dst->bs->bl.max_transfer),
>> + INT_MAX), align);
> 
> I suppose the outer QEMU_ALIGN_DOWN() may result in @max_transfer of 0
> (in theory, if one’s max_transfer is smaller than the other’s alignment).
> 
> Not likely, but should still be handled.
> 
> The rest looks good to me.
> 
> Max
> 
>>   /* TODO We can support BDRV_REQ_NO_FALLBACK here */
>>   assert(!(read_flags & BDRV_REQ_NO_FALLBACK));
> 


-- 
Best regards,
Vladimir


Re: [Qemu-block] [PATCH 3/8] block/io: handle alignment and max_transfer for copy_range

2019-08-07 Thread Max Reitz
On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> copy_range ignores these limitations, let's improve it. block/backup
> code handles max_transfer for copy_range by itself, now it's not needed
> more, drop it.

Shouldn’t this be two separate patches?

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 11 ++-
>  block/io.c | 41 +
>  2 files changed, 35 insertions(+), 17 deletions(-)

[...]

> diff --git a/block/io.c b/block/io.c
> index 06305c6ea6..5abbd0fff2 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3005,6 +3005,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>  {
>  BdrvTrackedRequest req;
>  int ret;
> +uint32_t align = MAX(src->bs->bl.request_alignment,
> + dst->bs->bl.request_alignment);
> +uint32_t max_transfer =
> +
> QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
> +  
> dst->bs->bl.max_transfer),
> + INT_MAX), align);

I suppose the outer QEMU_ALIGN_DOWN() may result in @max_transfer of 0
(in theory, if one’s max_transfer is smaller than the other’s alignment).

Not likely, but should still be handled.

The rest looks good to me.

Max

>  /* TODO We can support BDRV_REQ_NO_FALLBACK here */
>  assert(!(read_flags & BDRV_REQ_NO_FALLBACK));



signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PATCH 3/8] block/io: handle alignment and max_transfer for copy_range

2019-08-07 Thread Vladimir Sementsov-Ogievskiy
copy_range ignores these limitations, let's improve it. block/backup
code handles max_transfer for copy_range by itself, now it's not needed
more, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 11 ++-
 block/io.c | 41 +
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 3cdbe973e6..11e27c844d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -54,7 +54,6 @@ typedef struct BackupBlockJob {
 QLIST_HEAD(, CowRequest) inflight_reqs;
 
 bool use_copy_range;
-int64_t copy_range_size;
 
 BdrvRequestFlags write_flags;
 bool initializing_bitmap;
@@ -156,12 +155,11 @@ static int coroutine_fn 
backup_cow_with_offload(BackupBlockJob *job,
 int ret;
 int nr_clusters;
 BlockBackend *blk = job->common.blk;
-int nbytes;
+int nbytes = end - start;
 int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
-assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+assert(end - start < INT_MAX);
 assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-nbytes = MIN(job->copy_range_size, end - start);
 nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
 bdrv_reset_dirty_bitmap(job->copy_bitmap, start,
 job->cluster_size * nr_clusters);
@@ -756,11 +754,6 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 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));
 
 /* Required permissions are already taken with target's blk_new() */
 block_job_add_bdrv(>common, "target", target, 0, BLK_PERM_ALL,
diff --git a/block/io.c b/block/io.c
index 06305c6ea6..5abbd0fff2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3005,6 +3005,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 {
 BdrvTrackedRequest req;
 int ret;
+uint32_t align = MAX(src->bs->bl.request_alignment,
+ dst->bs->bl.request_alignment);
+uint32_t max_transfer =
+QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
+  
dst->bs->bl.max_transfer),
+ INT_MAX), align);
 
 /* TODO We can support BDRV_REQ_NO_FALLBACK here */
 assert(!(read_flags & BDRV_REQ_NO_FALLBACK));
@@ -3031,7 +3037,10 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 
 if (!src->bs->drv->bdrv_co_copy_range_from
 || !dst->bs->drv->bdrv_co_copy_range_to
-|| src->bs->encrypted || dst->bs->encrypted) {
+|| src->bs->encrypted || dst->bs->encrypted ||
+!QEMU_IS_ALIGNED(src_offset, src->bs->bl.request_alignment) ||
+!QEMU_IS_ALIGNED(dst_offset, dst->bs->bl.request_alignment) ||
+!QEMU_IS_ALIGNED(bytes, align)) {
 return -ENOTSUP;
 }
 
@@ -3046,11 +3055,22 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 wait_serialising_requests();
 }
 
-ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
-src, src_offset,
-dst, dst_offset,
-bytes,
-read_flags, write_flags);
+while (bytes) {
+int num = MIN(bytes, max_transfer);
+
+ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
+src, src_offset,
+dst, dst_offset,
+num,
+read_flags,
+write_flags);
+if (ret < 0) {
+break;
+}
+bytes -= num;
+src_offset += num;
+dst_offset += num;
+}
 
 tracked_request_end();
 bdrv_dec_in_flight(src->bs);
@@ -3060,12 +3080,17 @@ static int coroutine_fn bdrv_co_copy_range_internal(
   BDRV_TRACKED_WRITE);
 ret = bdrv_co_write_req_prepare(dst, dst_offset, bytes, ,
 write_flags);
-if (!ret) {
+while (!ret && bytes) {
+int num = MIN(bytes, max_transfer);
+
 ret = dst->bs->drv->bdrv_co_copy_range_to(dst->bs,