[Qemu-devel] [RFC PATCH 0/2] Enhance block status when crossing EOF

[Qemu-devel] [RFC PATCH 0/2] Enhance block status when crossing EOF

[Eric Blake]

As mentioned in my 'qcow2 zero-cluster tweaks' cover letter
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00722.html
I hit a case where we were not fully optimizing a write-zeroes
operation when we have a backing file that is shorter than the
active layer, and where the request crosses that boundary.  I don't
know that the situation will be all that common (most qcow2 files
happen to be cluster-aligned in size; and most backing chains use
the same length at each layer of the chain), but for RFC purposes,
I wanted to explore how easy it would be to optimize the case
anyways, and to see if we can think of any other cases where
knowing that a block status crossed an end-of-file boundary can
be exploited for less work.

Thus, this is a followup to that series, but I'm also okay if we
think it is too much maintenance compared to the potential gains,
and decide to drop it after all.

Another potential use that I thought of: we may someday want to
track BDS length by bytes.  Right now, bdrv_getlength() returns a
value in bytes, but which is sector-aligned, by adding padding;
but raw-format.c is able to see the transition between data up to
EOF followed by a hole after EOF.  Because of that, my series to
convert bdrv_get_block_status() into a byte-based interface has to
fudge things - any time we see a mid-sector hole being reported,
we know it is because we hit EOF, and can round the result up to
the next sector boundary.  Having the BDRV_BLOCK_EOF flag set in
that case may alert clients that the rounding took place, or help
us when we quit doing the rounding and actually start tracking BDS
lengths by bytes even where it is not sector-aligned.

Eric Blake (2):
  block: Add BDRV_BLOCK_EOF to bdrv_get_block_status()
  block: Exploit BDRV_BLOCK_EOF for larger zero blocks

 include/block/block.h      |  2 ++
 block/io.c                 | 42 +++++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/154     |  4 ----
 tests/qemu-iotests/154.out | 12 ++++++------
 4 files changed, 41 insertions(+), 19 deletions(-)

-- 
2.9.3

[Qemu-devel] [RFC PATCH 1/2] block: Add BDRV_BLOCK_EOF to bdrv_get_block_status()

[Eric Blake]

Just as the block layer already sets BDRV_BLOCK_ALLOCATED as a
shortcut for subsequent operations, there are also some optimizations
that are made easier if we can quickly tell that *pnum will advance
us to the end of a file, via a new BDRV_BLOCK_EOF which gets set
by the block layer.

This just plumbs up the new bit; subsequent patches will make use
of it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h |  2 ++
 block/io.c            | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index c8bec7d..6aff894 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -128,6 +128,7 @@ typedef struct HDGeometry {
  * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
  * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
  *                       layer (short for DATA || ZERO), set by block layer
+ * BDRV_BLOCK_EOF: the returned pnum covers through end of file for this layer
  *
  * Internal flag:
  * BDRV_BLOCK_RAW: used internally to indicate that the request was
@@ -156,6 +157,7 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_OFFSET_VALID 0x04
 #define BDRV_BLOCK_RAW          0x08
 #define BDRV_BLOCK_ALLOCATED    0x10
+#define BDRV_BLOCK_EOF          0x20
 #define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK

 typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
diff --git a/block/io.c b/block/io.c
index fdd7485..a7bc8bd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1727,15 +1727,16 @@ typedef struct BdrvCoGetBlockStatusData {
  * Drivers not implementing the functionality are assumed to not support
  * backing files, hence all their sectors are reported as allocated.
  *
- * If 'sector_num' is beyond the end of the disk image the return value is 0
- * and 'pnum' is set to 0.
+ * If 'sector_num' is beyond the end of the disk image the return value is
+ * BDRV_BLOCK_EOF and 'pnum' is set to 0.
  *
  * 'pnum' is set to the number of sectors (including and immediately following
  * the specified sector) that are known to be in the same
  * allocated/unallocated state.
  *
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
- * beyond the end of the disk image it will be clamped.
+ * beyond the end of the disk image it will be clamped; if 'pnum' is set to
+ * the end of the image, then the returned value will include BDRV_BLOCK_EOF.
  *
  * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
  * points to the BDS which the sector range is allocated in.
@@ -1756,7 +1757,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,

     if (sector_num >= total_sectors) {
         *pnum = 0;
-        return 0;
+        return BDRV_BLOCK_EOF;
     }

     n = total_sectors - sector_num;
@@ -1767,6 +1768,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
         ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
+        if (sector_num + nb_sectors == total_sectors) {
+            ret |= BDRV_BLOCK_EOF;
+        }
         if (bs->drv->protocol_name) {
             ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
         }
@@ -1830,6 +1834,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,

 out:
     bdrv_dec_in_flight(bs);
+    if (ret >= 0 && sector_num + *pnum == total_sectors) {
+        ret |= BDRV_BLOCK_EOF;
+    }
     return ret;
 }

-- 
2.9.3

[Qemu-devel] [RFC PATCH 2/2] block: Exploit BDRV_BLOCK_EOF for larger zero blocks

[Eric Blake]

When we have a BSD with unallocated clusters, but asking the status
of its underlying bs->file or backing layer encounters an end-of-file
condition, we know that the rest of the unallocated area will read as
zeroes.  However, pre-patch, this required two separate calls to
bdrv_get_block_status(), as the first call stops at the point where
the underlying file ends.  Thanks to BDRV_BLOCK_EOF, we can now widen
the results of the primary status if the secondary status already
includes BDRV_BLOCK_ZERO.

In turn, this fixes a TODO mentioned in iotest 154, where we can now
see that all sectors in a partial cluster at the end of a file read
as zero when coupling the shorter backing file's status along with our
knowledge that the remaining sectors came from an unallocated cluster.

Also, note that the loop in bdrv_co_get_block_status_above() had an
inefficent exit: in cases where the active layer sets BDRV_BLOCK_ZERO
but does NOT set BDRV_BLOCK_ALLOCATED (namely, where we know we read
zeroes merely because our unallocated clusters lie beyond the backing
file's shorter length), we still ended up probing the backing layer
even though we already had a good answer.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c                 | 27 ++++++++++++++++++++++-----
 tests/qemu-iotests/154     |  4 ----
 tests/qemu-iotests/154.out | 12 ++++++------
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/block/io.c b/block/io.c
index a7bc8bd..0ec9c75 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1819,10 +1819,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
             /* Ignore errors.  This is just providing extra information, it
              * is useful but not necessary.
              */
-            if (!file_pnum) {
-                /* !file_pnum indicates an offset at or beyond the EOF; it is
-                 * perfectly valid for the format block driver to point to such
-                 * offsets, so catch it and mark everything as zero */
+            if (ret2 & BDRV_BLOCK_EOF &&
+                (!file_pnum || ret2 & BDRV_BLOCK_ZERO)) {
+                /*
+                 * It is valid for the format block driver to read
+                 * beyond the end of the underlying file's current
+                 * size; such areas read as zero.
+                 */
                 ret |= BDRV_BLOCK_ZERO;
             } else {
                 /* Limit request to the range reported by the protocol driver */
@@ -1849,16 +1852,30 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
 {
     BlockDriverState *p;
     int64_t ret = 0;
+    bool first = true;

     assert(bs != base);
     for (p = bs; p != base; p = backing_bs(p)) {
         ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
-        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
+        if (ret < 0) {
+            break;
+        }
+        if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
+            /*
+             * Reading beyond the end of the file continues to read
+             * zeroes, but we can only widen the result to the
+             * unallocated length we learned from an earlier
+             * iteration.
+             */
+            *pnum = nb_sectors;
+        }
+        if (ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_DATA)) {
             break;
         }
         /* [sector_num, pnum] unallocated on this layer, which could be only
          * the first part of [sector_num, nb_sectors].  */
         nb_sectors = MIN(nb_sectors, *pnum);
+        first = false;
     }
     return ret;
 }
diff --git a/tests/qemu-iotests/154 b/tests/qemu-iotests/154
index 687b8f3..d4dae83 100755
--- a/tests/qemu-iotests/154
+++ b/tests/qemu-iotests/154
@@ -334,8 +334,6 @@ $QEMU_IO -c "alloc $size 2048" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map

 # Repeat with backing file holding unallocated cluster.
-# TODO: Note that this forces an allocation, because we aren't yet able to
-# quickly detect that reads beyond EOF of the backing file are always zero
 CLUSTER_SIZE=2048 TEST_IMG="$TEST_IMG.base" _make_test_img $((size + 1024))

 # Write at the front: sector-wise, the request is:
@@ -371,8 +369,6 @@ $QEMU_IO -c "alloc $size 2048" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map

 # Repeat with backing file holding zero'd cluster
-# TODO: Note that this forces an allocation, because we aren't yet able to
-# quickly detect that reads beyond EOF of the backing file are always zero
 $QEMU_IO -c "write -z $size 512" "$TEST_IMG.base" | _filter_qemu_io

 # Write at the front: sector-wise, the request is:
diff --git a/tests/qemu-iotests/154.out b/tests/qemu-iotests/154.out
index b86f074..9e7abf2 100644
--- a/tests/qemu-iotests/154.out
+++ b/tests/qemu-iotests/154.out
@@ -310,19 +310,19 @@ wrote 512/512 bytes at offset 134217728
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 512/512 bytes at offset 134219264
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 1024/1024 bytes at offset 134218240
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 2048/2048 bytes at offset 134217728
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -336,19 +336,19 @@ wrote 512/512 bytes at offset 134217728
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 512/512 bytes at offset 134219264
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 1024/1024 bytes at offset 134218240
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 2048/2048 bytes at offset 134217728
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.9.3

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 2/2] block: Exploit BDRV_BLOCK_EOF for larger zero blocks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 142 bytes --]

On Thu, May 04, 2017 at 09:15:00PM -0500, Eric Blake wrote:
> When we have a BSD with unallocated clusters, but asking the status

s/BSD/BDS/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/2] Enhance block status when crossing EOF

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Thu, May 04, 2017 at 09:14:58PM -0500, Eric Blake wrote:
> As mentioned in my 'qcow2 zero-cluster tweaks' cover letter
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00722.html
> I hit a case where we were not fully optimizing a write-zeroes
> operation when we have a backing file that is shorter than the
> active layer, and where the request crosses that boundary.  I don't
> know that the situation will be all that common (most qcow2 files
> happen to be cluster-aligned in size; and most backing chains use
> the same length at each layer of the chain), but for RFC purposes,
> I wanted to explore how easy it would be to optimize the case
> anyways, and to see if we can think of any other cases where
> knowing that a block status crossed an end-of-file boundary can
> be exploited for less work.
> 
> Thus, this is a followup to that series, but I'm also okay if we
> think it is too much maintenance compared to the potential gains,
> and decide to drop it after all.
> 
> Another potential use that I thought of: we may someday want to
> track BDS length by bytes.  Right now, bdrv_getlength() returns a
> value in bytes, but which is sector-aligned, by adding padding;
> but raw-format.c is able to see the transition between data up to
> EOF followed by a hole after EOF.  Because of that, my series to
> convert bdrv_get_block_status() into a byte-based interface has to
> fudge things - any time we see a mid-sector hole being reported,
> we know it is because we hit EOF, and can round the result up to
> the next sector boundary.  Having the BDRV_BLOCK_EOF flag set in
> that case may alert clients that the rounding took place, or help
> us when we quit doing the rounding and actually start tracking BDS
> lengths by bytes even where it is not sector-aligned.
> 
> Eric Blake (2):
>   block: Add BDRV_BLOCK_EOF to bdrv_get_block_status()
>   block: Exploit BDRV_BLOCK_EOF for larger zero blocks
> 
>  include/block/block.h      |  2 ++
>  block/io.c                 | 42 +++++++++++++++++++++++++++++++++---------
>  tests/qemu-iotests/154     |  4 ----
>  tests/qemu-iotests/154.out | 12 ++++++------
>  4 files changed, 41 insertions(+), 19 deletions(-)
> 
> -- 
> 2.9.3
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/2] Enhance block status when crossing EOF

[Fam Zheng]

On Thu, 05/04 21:14, Eric Blake wrote:
> Thus, this is a followup to that series, but I'm also okay if we
> think it is too much maintenance compared to the potential gains,
> and decide to drop it after all.

The comments are good enough and I like how this makes the function interface a
bit more powerful. Fixed the typo as pointed out by Stefan and applied. Thanks!

Fam