[Qemu-devel] [PATCH v4 0/3] raw-posix: Fix raw_co_get_block_status()

[Qemu-devel] [PATCH v4 0/3] raw-posix: Fix raw_co_get_block_status()

[Max Reitz]

raw_co_get_block_status() should return 0 and set *pnum to 0 after the
EOF; currently it does this merely by accident, so implement it
directly. Also, nb_sectors should be clamped against the image end.

While doing that, centralize the generation of
raw_co_get_block_status()'s return value along the way.


v4:
- Patch 1: Use DIV_ROUND_UP() for the range until the image end instead
  of truncating integer division [Eric]
- Patch 3:
  - Use qemu-img resize -f raw instrad of truncate [Eric]
  - Use qemu with HMP qemu-io instead of qemu-io itself; drop the
    qemu-img test [Eric]


git-backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[0002] [FC] 'raw-posix: Fix raw_co_get_block_status() after EOF'
002/3:[----] [-C] 'raw-posix: raw_co_get_block_status() return value'
003/3:[0021] [FC] 'iotests: Add test for external image truncation'

Max Reitz (3):
  raw-posix: Fix raw_co_get_block_status() after EOF
  raw-posix: raw_co_get_block_status() return value
  iotests: Add test for external image truncation

 block/raw-posix.c          | 42 ++++++++++++++++++++++++------------------
 tests/qemu-iotests/102     | 21 +++++++++++++++++++--
 tests/qemu-iotests/102.out | 11 +++++++++++
 3 files changed, 54 insertions(+), 20 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/3] raw-posix: Fix raw_co_get_block_status() after EOF

[Max Reitz]

As its comment states, raw_co_get_block_status() should unconditionally
return 0 and set *pnum to 0 for after EOF.

An assertion after lseek(..., SEEK_HOLE) tried to catch this case by
asserting that errno != -ENXIO (which would indicate a position after
the EOF); but it should be errno != ENXIO instead. Regardless of that,
there should be no such assertion at all. If bdrv_getlength() returned
an outdated value and the image has been resized outside of qemu,
lseek() will return with errno == ENXIO. Just return that value as an
error then.

Setting *pnum to 0 and returning 0 should not be done here, as in that
case we should update the device length as well. So, from qemu's
perspective, the file has not been resized; it's just that there was an
error querying sectors beyond a certain point (the actual file size).

Additionally, nb_sectors should be clamped against the image end. This
was probably not an issue if FIEMAP or SEEK_HOLE/SEEK_DATA worked, but
the fallback did not take this case into account.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/raw-posix.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 475cf74..a86b784 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1535,10 +1535,6 @@ static int64_t try_seek_hole(BlockDriverState *bs, off_t start, off_t *data,
 
     *hole = lseek(s->fd, start, SEEK_HOLE);
     if (*hole == -1) {
-        /* -ENXIO indicates that sector_num was past the end of the file.
-         * There is a virtual hole there.  */
-        assert(errno != -ENXIO);
-
         return -errno;
     }
 
@@ -1578,6 +1574,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                                     int nb_sectors, int *pnum)
 {
     off_t start, data = 0, hole = 0;
+    int64_t total_size;
     int64_t ret;
 
     ret = fd_open(bs);
@@ -1586,6 +1583,15 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     }
 
     start = sector_num * BDRV_SECTOR_SIZE;
+    total_size = bdrv_getlength(bs);
+    if (total_size < 0) {
+        return total_size;
+    } else if (start >= total_size) {
+        *pnum = 0;
+        return 0;
+    } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
+        nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
+    }
 
     ret = try_seek_hole(bs, start, &data, &hole, pnum);
     if (ret < 0) {
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/3] raw-posix: raw_co_get_block_status() return value

[Max Reitz]

Instead of generating the full return value thrice in try_fiemap(),
try_seek_hole() and as a fall-back in raw_co_get_block_status() itself,
generate the value only in raw_co_get_block_status().

While at it, also remove the pnum parameter from try_fiemap() and
try_seek_hole().

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/raw-posix.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a86b784..e100ae2 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1481,12 +1481,12 @@ out:
     return result;
 }
 
-static int64_t try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
-                          off_t *hole, int nb_sectors, int *pnum)
+static int try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
+                      off_t *hole, int nb_sectors)
 {
 #ifdef CONFIG_FIEMAP
     BDRVRawState *s = bs->opaque;
-    int64_t ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+    int ret = 0;
     struct {
         struct fiemap fm;
         struct fiemap_extent fe;
@@ -1527,8 +1527,8 @@ static int64_t try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
 #endif
 }
 
-static int64_t try_seek_hole(BlockDriverState *bs, off_t start, off_t *data,
-                             off_t *hole, int *pnum)
+static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data,
+                         off_t *hole)
 {
 #if defined SEEK_HOLE && defined SEEK_DATA
     BDRVRawState *s = bs->opaque;
@@ -1548,7 +1548,7 @@ static int64_t try_seek_hole(BlockDriverState *bs, off_t start, off_t *data,
         }
     }
 
-    return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+    return 0;
 #else
     return -ENOTSUP;
 #endif
@@ -1575,7 +1575,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 {
     off_t start, data = 0, hole = 0;
     int64_t total_size;
-    int64_t ret;
+    int ret;
 
     ret = fd_open(bs);
     if (ret < 0) {
@@ -1593,28 +1593,28 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
         nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
     }
 
-    ret = try_seek_hole(bs, start, &data, &hole, pnum);
+    ret = try_seek_hole(bs, start, &data, &hole);
     if (ret < 0) {
-        ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
+        ret = try_fiemap(bs, start, &data, &hole, nb_sectors);
         if (ret < 0) {
             /* Assume everything is allocated. */
             data = 0;
             hole = start + nb_sectors * BDRV_SECTOR_SIZE;
-            ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+            ret = 0;
         }
     }
 
+    assert(ret >= 0);
+
     if (data <= start) {
         /* On a data extent, compute sectors to the end of the extent.  */
         *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE);
+        return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
     } else {
         /* On a hole, compute sectors to the beginning of the next extent.  */
         *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
-        ret &= ~BDRV_BLOCK_DATA;
-        ret |= BDRV_BLOCK_ZERO;
+        return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start;
     }
-
-    return ret;
 }
 
 static coroutine_fn BlockAIOCB *raw_aio_discard(BlockDriverState *bs,
-- 
1.9.3

[Qemu-devel] [PATCH v4 3/3] iotests: Add test for external image truncation

[Max Reitz]

It should not be happening, but it is possible to truncate an image
outside of qemu while qemu is running (or any of the qemu tools using
the block layer. raw_co_get_block_status() should not break then.

While touching this test, replace the existing "truncate" invocation by
"$QEMU_IMG convert -f raw".

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/102     | 21 +++++++++++++++++++--
 tests/qemu-iotests/102.out | 11 +++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
index 34b363f..161b197 100755
--- a/tests/qemu-iotests/102
+++ b/tests/qemu-iotests/102
@@ -34,9 +34,10 @@ _cleanup()
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
-# get standard environment, filters and checks
+# get standard environment, filters and qemu instance handling
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt qcow2
 _supported_proto file
@@ -53,11 +54,27 @@ _make_test_img $IMG_SIZE
 $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
 # Remove data cluster from image (first cluster: image header, second: reftable,
 # third: refblock, fourth: L1 table, fifth: L2 table)
-truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
+$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024))
 
 $QEMU_IO -c map "$TEST_IMG"
 $QEMU_IMG map "$TEST_IMG"
 
+echo
+echo '=== Testing map on an image file truncated outside of qemu ==='
+echo
+
+# Same as above, only now we concurrently truncate and map the image
+_make_test_img $IMG_SIZE
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+qemu_comm_method=monitor _launch_qemu -drive if=none,file="$TEST_IMG",id=drv0
+
+$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024))
+
+_send_qemu_cmd $QEMU_HANDLE 'qemu-io drv0 map' 'allocated' \
+    | sed -e 's/^(qemu).*qemu-io drv0 map...$/(qemu) qemu-io drv0 map/'
+_send_qemu_cmd $QEMU_HANDLE 'quit' ''
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/102.out b/tests/qemu-iotests/102.out
index e0e9cdc..eecde16 100644
--- a/tests/qemu-iotests/102.out
+++ b/tests/qemu-iotests/102.out
@@ -5,6 +5,17 @@ QA output created by 102
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Image resized.
 [                       0]      128/     128 sectors     allocated at offset 0 bytes (1)
 Offset          Length          Mapped to       File
+
+=== Testing map on an image file truncated outside of qemu ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Image resized.
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io drv0 map
+[                       0]      128/     128 sectors     allocated at offset 0 bytes (1)
 *** done
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 3/3] iotests: Add test for external image truncation

[Eric Blake]

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

On 10/24/2014 04:58 AM, Max Reitz wrote:
> It should not be happening, but it is possible to truncate an image
> outside of qemu while qemu is running (or any of the qemu tools using
> the block layer. raw_co_get_block_status() should not break then.
> 
> While touching this test, replace the existing "truncate" invocation by
> "$QEMU_IMG convert -f raw".
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/102     | 21 +++++++++++++++++++--
>  tests/qemu-iotests/102.out | 11 +++++++++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] raw-posix: Fix raw_co_get_block_status()

[Kevin Wolf]

Am 24.10.2014 um 12:57 hat Max Reitz geschrieben:
> raw_co_get_block_status() should return 0 and set *pnum to 0 after the
> EOF; currently it does this merely by accident, so implement it
> directly. Also, nb_sectors should be clamped against the image end.
> 
> While doing that, centralize the generation of
> raw_co_get_block_status()'s return value along the way.
> 
> 
> v4:
> - Patch 1: Use DIV_ROUND_UP() for the range until the image end instead
>   of truncating integer division [Eric]
> - Patch 3:
>   - Use qemu-img resize -f raw instrad of truncate [Eric]
>   - Use qemu with HMP qemu-io instead of qemu-io itself; drop the
>     qemu-img test [Eric]

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v4 0/3] raw-posix: Fix raw_co_get_block_status()

[Stefan Hajnoczi]

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

On Fri, Oct 24, 2014 at 12:57:57PM +0200, Max Reitz wrote:
> raw_co_get_block_status() should return 0 and set *pnum to 0 after the
> EOF; currently it does this merely by accident, so implement it
> directly. Also, nb_sectors should be clamped against the image end.
> 
> While doing that, centralize the generation of
> raw_co_get_block_status()'s return value along the way.
> 
> 
> v4:
> - Patch 1: Use DIV_ROUND_UP() for the range until the image end instead
>   of truncating integer division [Eric]
> - Patch 3:
>   - Use qemu-img resize -f raw instrad of truncate [Eric]
>   - Use qemu with HMP qemu-io instead of qemu-io itself; drop the
>     qemu-img test [Eric]
> 
> 
> git-backport-diff against v3:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/3:[0002] [FC] 'raw-posix: Fix raw_co_get_block_status() after EOF'
> 002/3:[----] [-C] 'raw-posix: raw_co_get_block_status() return value'
> 003/3:[0021] [FC] 'iotests: Add test for external image truncation'
> 
> Max Reitz (3):
>   raw-posix: Fix raw_co_get_block_status() after EOF
>   raw-posix: raw_co_get_block_status() return value
>   iotests: Add test for external image truncation
> 
>  block/raw-posix.c          | 42 ++++++++++++++++++++++++------------------
>  tests/qemu-iotests/102     | 21 +++++++++++++++++++--
>  tests/qemu-iotests/102.out | 11 +++++++++++
>  3 files changed, 54 insertions(+), 20 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]