[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/5] block: add max_pwrite_zeroes_no_fallback to BlockLimits

From: Eric Blake
Subject: Re: [PATCH 3/5] block: add max_pwrite_zeroes_no_fallback to BlockLimits
Date: Fri, 13 Mar 2020 16:07:03 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
NBD spec is updated, so that max_block doesn't relate to

Maybe: The NBD spec was recently updated to clarify that max_block...

flag BDRV_REQ_NO_FALLBACK). To drop the restriction we need new

It feels odd to have two different pwrite_zeroes limits in the block layer, but I can live with it if other block layer gurus are also okay with it.

Default value of new max_pwrite_zeroes_no_fallback is zero and it means
no-restriction, so we are automatically done by this commit. Note that

Why not have the default value be set to the existing value of the normal pwrite_zeroes limit, rather than 0?

nbd and blkdebug are the only drivers which in the same time define
max_pwrite_zeroes limit and support BDRV_REQ_NO_FALLBACK, so we need to
update only blkdebug.


The default value for the new max_pwrite_zeroes_no_fallback is zero, meaning no restriction, which covers all drivers not touched by this commit. Note that nbd and blkdebug are the only drivers which have a max_pwrite_zeroes limit while supporting BDRV_REQ_NO_FALLBACK, so we only need to update blkdebug.

Except that I think there IS still a limit in current NBD: you can't request anything larger than 32 bits (whereas some other drivers may allow a full 63-bit request, as well as future NBD usage when we finally add 64-bit extensions to the protocol). So I think this patch is incomplete; it should be updating the nbd code to set the proper limit.

(I still need to post v2 of my patches for bdrv_co_make_zero support, which is a case where knowing if there is a 32-bit limit when using BDRV_REQ_NO_FALLBACK for fast zeroing is important).

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
  include/block/block_int.h | 8 ++++++++
  block/blkdebug.c          | 7 ++++++-
  block/io.c                | 4 +++-
  3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f9fd5e20e..c167e887c6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -618,6 +618,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 zeroized at once if flag


+     * BDRV_REQ_NO_FALLBACK specified (since it is signed, it must be < 2G, if
+     * set).

Why must it be a signed 32-bit number?  Why not let it be a 64-bit number?

Must be multiple of pwrite_zeroes_alignment. May be 0 if no
+     * inherent 32-bit limit.
+     */
+    int32_t max_pwrite_zeroes_no_fallback;
      /* 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/blkdebug.c b/block/blkdebug.c
index af44aa973f..7627fbcb3b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -692,7 +692,11 @@ static int coroutine_fn 
blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
      assert(QEMU_IS_ALIGNED(offset, align));
      assert(QEMU_IS_ALIGNED(bytes, align));
-    if (bs->bl.max_pwrite_zeroes) {
+    if ((flags & BDRV_REQ_NO_FALLBACK) &&
+        bs->bl.max_pwrite_zeroes_no_fallback)
+    {
+        assert(bytes <= bs->bl.max_pwrite_zeroes_no_fallback);
+    } else if (bs->bl.max_pwrite_zeroes) {
          assert(bytes <= bs->bl.max_pwrite_zeroes);
@@ -977,6 +981,7 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp)
      if (s->max_write_zero) {
          bs->bl.max_pwrite_zeroes = s->max_write_zero;
+        bs->bl.max_pwrite_zeroes_no_fallback = s->max_write_zero;

Ah, so you DO default it to max_pwwrite_zeroes instead of to 0; the commit message does not quite match the code.

      if (s->opt_discard) {
          bs->bl.pdiscard_alignment = s->opt_discard;
diff --git a/block/io.c b/block/io.c
index 7e4cb74cf4..75fd5600c2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1752,7 +1752,9 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
      int head = 0;
      int tail = 0;
- int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
+    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);

I'd still like to get rid of this INT_MAX clamping. If we can blank the entire image in one call, even when it is larger than 4G, then it is worth making that exposed to the user. (Even in NBD, we might decide to add an extension that allows NBD_CMD_WRITE_ZEROES with a new flag and with offset/length == 0/0, as an official way to make the entire image zero, whereas it is now currently unspecified to pass a length of 0).

      int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
      int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]