1. Patchew
  2. QEMU
  3. [PATCH 0/5] nbd: reduce max_block restrictions
  4. View patch

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

Vladimir Sementsov-Ogievskiy posted 5 patches 5 years, 8 months ago
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>, Eric Blake <eblake@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
[PATCH 5/5] block/io: auto-no-fallback for write-zeroes
Posted by Vladimir Sementsov-Ogievskiy 5 years, 8 months ago 
NBD driver may has max_pwrite_zeroes but doesn't has
max_pwrite_zeroes_no_fallback limit. This means, that (when
BDRV_REQ_NO_FALLBACK is supported) it is beneficial to try send request
with BDRV_REQ_NO_FALLBACK instead of splitting the request accordingly
to max_pwrite_zeroes.

If failed, fallback to old behavior.

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

diff --git a/block/io.c b/block/io.c
index c64566b4cf..48d71b0883 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1752,17 +1752,28 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int head = 0;
     int tail = 0;
 
-    int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
-                                        bs->bl.max_pwrite_zeroes_no_fallback :
-                                        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);
+    bool auto_no_fallback;
 
     if (!drv) {
         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_no_fallback ||
+         bs->bl.max_pwrite_zeroes_no_fallback == 0))
+    {
+        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;
     }
@@ -1770,7 +1781,11 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     assert(alignment % bs->bl.request_alignment == 0);
     head = offset % alignment;
     tail = (offset + bytes) % alignment;
-    max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
+    max_write_zeroes =
+        QEMU_ALIGN_DOWN(MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
+                                     bs->bl.max_pwrite_zeroes_no_fallback :
+                                     bs->bl.max_pwrite_zeroes, INT_MAX),
+                        alignment);
     assert(max_write_zeroes >= bs->bl.request_alignment);
 
     while (bytes > 0 && !ret) {
@@ -1801,6 +1816,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 5/5] block/io: auto-no-fallback for write-zeroes
Posted by Eric Blake 5 years, 8 months ago 
On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> NBD driver may has max_pwrite_zeroes but doesn't has
> max_pwrite_zeroes_no_fallback limit. This means, that (when
> BDRV_REQ_NO_FALLBACK is supported) it is beneficial to try send request
> with BDRV_REQ_NO_FALLBACK instead of splitting the request accordingly
> to max_pwrite_zeroes.
> 
> If failed, fallback to old behavior.

Grammar:

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 | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index c64566b4cf..48d71b0883 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1752,17 +1752,28 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>       int head = 0;
>       int tail = 0;
>   
> -    int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
> -                                        bs->bl.max_pwrite_zeroes_no_fallback :
> -                                        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);
> +    bool auto_no_fallback;
>   
>       if (!drv) {
>           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_no_fallback ||
> +         bs->bl.max_pwrite_zeroes_no_fallback == 0))

Why are we letting max_pwrite_zeroes_no_fallback ever be 0?  It might be 
more convenient if it is always guaranteed to be >= max_pwrite_zeroes by 
the block layer.

> +    {
> +        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;
>       }
> @@ -1770,7 +1781,11 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>       assert(alignment % bs->bl.request_alignment == 0);
>       head = offset % alignment;
>       tail = (offset + bytes) % alignment;
> -    max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
> +    max_write_zeroes =
> +        QEMU_ALIGN_DOWN(MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
> +                                     bs->bl.max_pwrite_zeroes_no_fallback :
> +                                     bs->bl.max_pwrite_zeroes, INT_MAX),
> +                        alignment);
>       assert(max_write_zeroes >= bs->bl.request_alignment);
>   
>       while (bytes > 0 && !ret) {
> @@ -1801,6 +1816,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;
> 

Otherwise makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Re: [PATCH 5/5] block/io: auto-no-fallback for write-zeroes
Posted by Vladimir Sementsov-Ogievskiy 5 years, 7 months ago 
14.03.2020 0:56, Eric Blake wrote:
> On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>> NBD driver may has max_pwrite_zeroes but doesn't has
>> max_pwrite_zeroes_no_fallback limit. This means, that (when
>> BDRV_REQ_NO_FALLBACK is supported) it is beneficial to try send request
>> with BDRV_REQ_NO_FALLBACK instead of splitting the request accordingly
>> to max_pwrite_zeroes.
>>
>> If failed, fallback to old behavior.
> 
> Grammar:
> 
> 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 | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index c64566b4cf..48d71b0883 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1752,17 +1752,28 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>       int head = 0;
>>       int tail = 0;
>> -    int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
>> -                                        bs->bl.max_pwrite_zeroes_no_fallback :
>> -                                        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);
>> +    bool auto_no_fallback;
>>       if (!drv) {
>>           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_no_fallback ||
>> +         bs->bl.max_pwrite_zeroes_no_fallback == 0))
> 
> Why are we letting max_pwrite_zeroes_no_fallback ever be 0?  It might be more convenient if it is always guaranteed to be >= max_pwrite_zeroes by the block layer.


All other limits may be 0 too, which means some default.. So, if we want to set the default for max_pwrite_zeroes_no_fallback explicitly, I think we should do it for all other limits too. And it should be separate thing..

> 
>> +    {
>> +        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;
>>       }
>> @@ -1770,7 +1781,11 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>       assert(alignment % bs->bl.request_alignment == 0);
>>       head = offset % alignment;
>>       tail = (offset + bytes) % alignment;
>> -    max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
>> +    max_write_zeroes =
>> +        QEMU_ALIGN_DOWN(MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
>> +                                     bs->bl.max_pwrite_zeroes_no_fallback :
>> +                                     bs->bl.max_pwrite_zeroes, INT_MAX),
>> +                        alignment);
>>       assert(max_write_zeroes >= bs->bl.request_alignment);
>>       while (bytes > 0 && !ret) {
>> @@ -1801,6 +1816,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;
>>
> 
> Otherwise makes sense.
> 


-- 
Best regards,
Vladimir

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.