[PATCH v4 0/4] nbd: reduce max_block restrictions

[PATCH v4 0/4] 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

v4: fix auto_no_fallback variable usage in 04:
    - initialize it
    - unset it on error
    Hope, it will fix bug reported by patchew

Vladimir Sementsov-Ogievskiy (4):
  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                | 40 +++++++++++++++++++++++++++++++++------
 block/nbd.c               |  1 +
 3 files changed, 43 insertions(+), 6 deletions(-)

-- 
2.21.0

[PATCH v4 1/4] 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 791de6a59c..277e32fe31 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -626,6 +626,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 df8f2a98d4..0af62a53fd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1774,12 +1774,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;
     }
@@ -1788,12 +1789,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 v4 2/4] 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 v4 3/4] 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 0af62a53fd..3fae97da2d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1813,7 +1813,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.  */
@@ -1872,6 +1871,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 v4 4/4] 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/io.c b/block/io.c
index 3fae97da2d..ad219cb220 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1778,6 +1778,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 = false;
 
     assert(alignment % bs->bl.request_alignment == 0);
 
@@ -1785,6 +1786,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;
     }
@@ -1829,6 +1840,14 @@ 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) {
+                auto_no_fallback = false;
+                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 v4 1/4] block: add max_pwrite_zeroes_fast to BlockLimits

[Eric Blake]

On 6/11/20 11:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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(-)

Hmm, this is an optimization, rather than a correctness issue.  I'm 
sorry I didn't review it sooner, but at this point, I think it is better 
as 5.2 material.

> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 791de6a59c..277e32fe31 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -626,6 +626,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;

Nice that this is 64-bit off the bat (I know you have another series 
about converting more stuff to 64-bit).

> +
>       /* 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 df8f2a98d4..0af62a53fd 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1774,12 +1774,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;

32-bit...

>       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);

Would this look any better using the QEMU_IS_ALIGNED macro?

> +
>       if (!drv) {
>           return -ENOMEDIUM;
>       }
> @@ -1788,12 +1789,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;

...but you try to assign something that may be 64-bit into it.  Risk of 
overflow.  Maybe we should get your 64-bit cleanup series in first.

> +    } 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;
>   
> 

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

Re: [PATCH v4 2/4] block/nbd: define new max_write_zero_fast limit

[Eric Blake]

On 6/11/20 11:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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;

Do we even need max_pwrite_zeroes_fast?  Doesn't qemu behave correctly 
if we just blindly assign max_pdiscard and max_pwrite_zeroes to the same 
value near 2G?

>       bs->bl.max_transfer = max;
>   
> 

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

Re: [PATCH v4 2/4] block/nbd: define new max_write_zero_fast limit

[Vladimir Sementsov-Ogievskiy]

23.07.2020 23:32, Eric Blake wrote:
> On 6/11/20 11:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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;
> 
> Do we even need max_pwrite_zeroes_fast?  Doesn't qemu behave correctly if we just blindly assign max_pdiscard and max_pwrite_zeroes to the same value near 2G?
> 

Without BDRV_REQ_NO_FALLBACK, max_block is an actual limit for WRITE_ZERO.. So, in my understanding, it doesn't


-- 
Best regards,
Vladimir