[PATCH v2 0/6] nbd: reduce max_block restrictions

[PATCH v2 0/6] nbd: reduce max_block restrictions

[Vladimir Sementsov-Ogievskiy]

Recent changes in NBD protocol allowed to use some commands without
max_block restriction. Let's drop the restrictions.

NBD change is here:
https://github.com/NetworkBlockDevice/nbd/commit/9f30fedb8699f151e7ef4ccc07e624330be3316b#diff-762fb7c670348da69cc9050ef58fe3ae

v2:

01: add Eric's r-b
02: keep INT_MAX limit for discard
03: s/max_pwrite_zeroes_no_fallback/max_pwrite_zeroes_fast/
      (a bit shorter, so that if condition fit in one line)
    default is changed to be max_pwrite_zeroes,
      so blkdebug is unchanged and we need one more patch: 04
    refactor max_write_zeroes calculation in bdrv_co_do_pwrite_zeroes,
      prepare to last patch
04: new, actual change for nbd driver
05: reword commit message (fix -> refactoring) as I don't see bugs here. Keep an r-b
06: rewording, grammar [Eric]
    rebase on 03 changes

Vladimir Sementsov-Ogievskiy (6):
  block/nbd-client: drop max_block restriction from block_status
  block/nbd-client: drop max_block restriction from discard
  block: add max_pwrite_zeroes_fast to BlockLimits
  block/nbd: define new max_write_zero_fast limit
  block/io: refactor bdrv_co_do_pwrite_zeroes head calculation
  block/io: auto-no-fallback for write-zeroes

 include/block/block_int.h |  8 ++++++++
 block/io.c                | 39 +++++++++++++++++++++++++++++++++------
 block/nbd.c               |  7 +++----
 3 files changed, 44 insertions(+), 10 deletions(-)

-- 
2.21.0

[PATCH v2 1/6] block/nbd-client: drop max_block restriction from block_status

[Vladimir Sementsov-Ogievskiy]

NBD spec is updated, so that max_block doesn't relate to
NBD_CMD_BLOCK_STATUS. So, drop the restriction.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 2160859f64..d4d518a780 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1320,9 +1320,7 @@ static int coroutine_fn nbd_client_co_block_status(
     NBDRequest request = {
         .type = NBD_CMD_BLOCK_STATUS,
         .from = offset,
-        .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX,
-                                                bs->bl.request_alignment),
-                                s->info.max_block),
+        .len = MIN(QEMU_ALIGN_DOWN(INT_MAX, bs->bl.request_alignment),
                    MIN(bytes, s->info.size - offset)),
         .flags = NBD_CMD_FLAG_REQ_ONE,
     };
-- 
2.21.0

[PATCH v2 2/6] block/nbd-client: drop max_block restriction from discard

[Vladimir Sementsov-Ogievskiy]

NBD spec is updated, so that max_block doesn't relate to
NBD_CMD_TRIM. So, drop the restriction.

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

diff --git a/block/nbd.c b/block/nbd.c
index d4d518a780..4ac23c8f62 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1955,7 +1955,7 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 
     bs->bl.request_alignment = min;
-    bs->bl.max_pdiscard = max;
+    bs->bl.max_pdiscard = QEMU_ALIGN_DOWN(INT_MAX, min);
     bs->bl.max_pwrite_zeroes = max;
     bs->bl.max_transfer = max;
 
-- 
2.21.0

Re: [PATCH v2 2/6] block/nbd-client: drop max_block restriction from discard

[Eric Blake]

On 4/1/20 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> NBD spec is updated, so that max_block doesn't relate to
> NBD_CMD_TRIM. So, drop the restriction.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/nbd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

I might tweak the commit message of 1/6 and here to call out the NBD 
spec commit id (nbd.git 9f30fedb), but that doesn't change the patch proper.

> 
> diff --git a/block/nbd.c b/block/nbd.c
> index d4d518a780..4ac23c8f62 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1955,7 +1955,7 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
>       }
>   
>       bs->bl.request_alignment = min;
> -    bs->bl.max_pdiscard = max;
> +    bs->bl.max_pdiscard = QEMU_ALIGN_DOWN(INT_MAX, min);
>       bs->bl.max_pwrite_zeroes = max;
>       bs->bl.max_transfer = max;
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/6] block/nbd-client: drop max_block restriction from discard

[Eric Blake]

On 4/21/20 4:56 PM, Eric Blake wrote:
> On 4/1/20 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
>> NBD spec is updated, so that max_block doesn't relate to
>> NBD_CMD_TRIM. So, drop the restriction.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I might tweak the commit message of 1/6 and here to call out the NBD 
> spec commit id (nbd.git 9f30fedb), but that doesn't change the patch 
> proper.

I'm queuing 1 and 2 through my NBD tree now; the rest involve more of 
the block layer and go in tandem with your other work on cleaning up 
64-bit operations throughout, and I still need to give that a better review.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/6] block/nbd-client: drop max_block restriction from discard

[Vladimir Sementsov-Ogievskiy]

04.05.2020 22:59, Eric Blake wrote:
> On 4/21/20 4:56 PM, Eric Blake wrote:
>> On 4/1/20 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> NBD spec is updated, so that max_block doesn't relate to
>>> NBD_CMD_TRIM. So, drop the restriction.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/nbd.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> I might tweak the commit message of 1/6 and here to call out the NBD spec commit id (nbd.git 9f30fedb), but that doesn't change the patch proper.
> 
> I'm queuing 1 and 2 through my NBD tree now; the rest involve more of the block layer and go in tandem with your other work on cleaning up 64-bit operations throughout, and I still need to give that a better review.
> 

Thank you!

-- 
Best regards,
Vladimir

[PATCH v2 3/6] block: add max_pwrite_zeroes_fast to BlockLimits

[Vladimir Sementsov-Ogievskiy]

The NBD spec was recently updated to clarify that max_block doesn't
relate to NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which
mirrors Qemu flag BDRV_REQ_NO_FALLBACK). To drop the restriction we
need new max_pwrite_zeroes_fast.

Default value of new max_pwrite_zeroes_fast is zero and it means
use max_pwrite_zeroes. So this commit semantically changes nothing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  8 ++++++++
 block/io.c                | 17 ++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4c3587ea19..ea1018d598 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -624,6 +624,14 @@ typedef struct BlockLimits {
      * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
     int32_t max_pwrite_zeroes;
 
+    /*
+     * Maximum number of bytes that can zeroed at once if flag
+     * BDRV_REQ_NO_FALLBACK specified. Must be multiple of
+     * pwrite_zeroes_alignment.
+     * If 0, max_pwrite_zeroes is used for no-fallback case.
+     */
+    int64_t max_pwrite_zeroes_fast;
+
     /* Optimal alignment for write zeroes requests in bytes. A power
      * of 2 is best but not mandatory.  Must be a multiple of
      * bl.request_alignment, and must be less than max_pwrite_zeroes
diff --git a/block/io.c b/block/io.c
index aba67f66b9..07270524a9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1751,12 +1751,13 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     bool need_flush = false;
     int head = 0;
     int tail = 0;
-
-    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
+    int max_write_zeroes;
     int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
                         bs->bl.request_alignment);
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
 
+    assert(alignment % bs->bl.request_alignment == 0);
+
     if (!drv) {
         return -ENOMEDIUM;
     }
@@ -1765,12 +1766,18 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
-    assert(alignment % bs->bl.request_alignment == 0);
-    head = offset % alignment;
-    tail = (offset + bytes) % alignment;
+    if ((flags & BDRV_REQ_NO_FALLBACK) && bs->bl.max_pwrite_zeroes_fast) {
+        max_write_zeroes = bs->bl.max_pwrite_zeroes_fast;
+    } else {
+        max_write_zeroes = bs->bl.max_pwrite_zeroes;
+    }
+    max_write_zeroes = MIN_NON_ZERO(max_write_zeroes, INT_MAX);
     max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
     assert(max_write_zeroes >= bs->bl.request_alignment);
 
+    head = offset % alignment;
+    tail = (offset + bytes) % alignment;
+
     while (bytes > 0 && !ret) {
         int num = bytes;
 
-- 
2.21.0

[PATCH v2 4/6] block/nbd: define new max_write_zero_fast limit

[Vladimir Sementsov-Ogievskiy]

The NBD spec was recently updated to clarify that max_block doesn't
relate to NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which
mirrors Qemu flag BDRV_REQ_NO_FALLBACK).

bs->bl.max_write_zero_fast is zero by default which means using
max_pwrite_zeroes. Update nbd driver to allow larger requests with
BDRV_REQ_NO_FALLBACK.

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

diff --git a/block/nbd.c b/block/nbd.c
index 4ac23c8f62..b0584cf68d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1956,6 +1956,7 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
 
     bs->bl.request_alignment = min;
     bs->bl.max_pdiscard = QEMU_ALIGN_DOWN(INT_MAX, min);
+    bs->bl.max_pwrite_zeroes_fast = bs->bl.max_pdiscard;
     bs->bl.max_pwrite_zeroes = max;
     bs->bl.max_transfer = max;
 
-- 
2.21.0

[PATCH v2 5/6] block/io: refactor bdrv_co_do_pwrite_zeroes head calculation

[Vladimir Sementsov-Ogievskiy]

It's wrong to update head using num in this place, as num may be
reduced during the iteration (seems it doesn't, but it's not obvious),
and we'll have wrong head value on next iteration.

Instead update head at iteration end.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 07270524a9..f8335e7212 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1790,7 +1790,6 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
              * convenience, limit this request to max_transfer even if
              * we don't need to fall back to writes.  */
             num = MIN(MIN(bytes, max_transfer), alignment - head);
-            head = (head + num) % alignment;
             assert(num < max_write_zeroes);
         } else if (tail && num > alignment) {
             /* Shorten the request to the last aligned sector.  */
@@ -1849,6 +1848,9 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
 
         offset += num;
         bytes -= num;
+        if (head) {
+            head = offset % alignment;
+        }
     }
 
 fail:
-- 
2.21.0

[PATCH v2 6/6] block/io: auto-no-fallback for write-zeroes

[Vladimir Sementsov-Ogievskiy]

When BDRV_REQ_NO_FALLBACK is supported, the NBD driver supports a
larger request size.  Add code to try large zero requests with a
NO_FALLBACK request prior to having to split a request into chunks
according to max_pwrite_zeroes.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block/io.c b/block/io.c
index f8335e7212..425314a221 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1755,6 +1755,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
                         bs->bl.request_alignment);
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
+    bool auto_no_fallback;
 
     assert(alignment % bs->bl.request_alignment == 0);
 
@@ -1762,6 +1763,16 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
         return -ENOMEDIUM;
     }
 
+    if (!(flags & BDRV_REQ_NO_FALLBACK) &&
+        (bs->supported_zero_flags & BDRV_REQ_NO_FALLBACK) &&
+        bs->bl.max_pwrite_zeroes && bs->bl.max_pwrite_zeroes < bytes &&
+        bs->bl.max_pwrite_zeroes < bs->bl.max_pwrite_zeroes_fast)
+    {
+        assert(drv->bdrv_co_pwrite_zeroes);
+        flags |= BDRV_REQ_NO_FALLBACK;
+        auto_no_fallback = true;
+    }
+
     if ((flags & ~bs->supported_zero_flags) & BDRV_REQ_NO_FALLBACK) {
         return -ENOTSUP;
     }
@@ -1806,6 +1817,13 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
         if (drv->bdrv_co_pwrite_zeroes) {
             ret = drv->bdrv_co_pwrite_zeroes(bs, offset, num,
                                              flags & bs->supported_zero_flags);
+            if (ret == -ENOTSUP && auto_no_fallback) {
+                flags &= ~BDRV_REQ_NO_FALLBACK;
+                max_write_zeroes =
+                    QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pwrite_zeroes,
+                                                 INT_MAX), alignment);
+                continue;
+            }
             if (ret != -ENOTSUP && (flags & BDRV_REQ_FUA) &&
                 !(bs->supported_zero_flags & BDRV_REQ_FUA)) {
                 need_flush = true;
-- 
2.21.0

Re: [PATCH v2 0/6] nbd: reduce max_block restrictions

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200401150112.9557-1-vsementsov@virtuozzo.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

--- /tmp/qemu-test/src/tests/qemu-iotests/251.out       2020-04-01 15:01:19.000000000 +0000
+++ /tmp/qemu-test/build/tests/qemu-iotests/251.out.bad 2020-04-01 16:49:36.542097534 +0000
@@ -18,26 +18,16 @@
 qemu-img: warning: error while reading offset read_fail_offset_8: Input/output error
 qemu-img: warning: error while reading offset read_fail_offset_9: Input/output error
 
-wrote 512/512 bytes at offset read_fail_offset_0
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
---
Not run: 259
Failures: 033 034 154 177 251
Failed 5 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-aarch64: tests/qtest/test-hmp
  TEST    check-qtest-aarch64: tests/qtest/qos-test
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=067a8c1d1d00403ba8447315095d6388', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-7rgy2yf_/src/docker-src.2020-04-01-12.37.18.28490:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=067a8c1d1d00403ba8447315095d6388
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-7rgy2yf_/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m49.328s
user    0m7.899s


The full log is available at
http://patchew.org/logs/20200401150112.9557-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/6] nbd: reduce max_block restrictions

[Vladimir Sementsov-Ogievskiy]

01.04.2020 19:52, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200401150112.9557-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
> --- /tmp/qemu-test/src/tests/qemu-iotests/251.out       2020-04-01 15:01:19.000000000 +0000
> +++ /tmp/qemu-test/build/tests/qemu-iotests/251.out.bad 2020-04-01 16:49:36.542097534 +0000
> @@ -18,26 +18,16 @@
>   qemu-img: warning: error while reading offset read_fail_offset_8: Input/output error
>   qemu-img: warning: error while reading offset read_fail_offset_9: Input/output error
>   
> -wrote 512/512 bytes at offset read_fail_offset_0
> -512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> ---
> Not run: 259
> Failures: 033 034 154 177 251

Strange, all five passed for me.

Several writes just failed as not supported.. Strange.


> Failed 5 of 116 iotests
> make: *** [check-tests/check-block.sh] Error 1
> make: *** Waiting for unfinished jobs....
>    TEST    check-qtest-aarch64: tests/qtest/test-hmp
>    TEST    check-qtest-aarch64: tests/qtest/qos-test
> ---
>      raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=067a8c1d1d00403ba8447315095d6388', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-7rgy2yf_/src/docker-src.2020-04-01-12.37.18.28490:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
> filter=--filter=label=com.qemu.instance.uuid=067a8c1d1d00403ba8447315095d6388
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-7rgy2yf_/src'
> make: *** [docker-run-test-quick@centos7] Error 2
> 
> real    14m49.328s
> user    0m7.899s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200401150112.9557-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 


-- 
Best regards,
Vladimir