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

[Qemu-devel] [PATCH v3 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.


v3:
- Patch 1:
  - Remove the whole ENXIO assert(), just return -errno. ENXIO is not a
    very useful message for the user, but it's better than a broken
    assertion for sure (and there is no good way of returning 0 and
    setting *pnum to 0 as well, because this would mean acknowledging
    the new size of the file and we'd have to update its length in the
    BDS, etc. pp.; just return ENXIO and be done with it) [Kevin]
  - bdrv_getlength() may fail [Kevin]
- Patch 2: Remove the pnum parameter from try_fiemap() and
  try_seek_hole(), it isn't used anyway
- Patch 3: Added.


git-backport-diff against v2:

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:[0008] [FC] 'raw-posix: Fix raw_co_get_block_status() after EOF'
002/3:[0008] [FC] 'raw-posix: raw_co_get_block_status() return value'
003/3:[down] '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     | 15 +++++++++++++++
 tests/qemu-iotests/102.out |  9 +++++++++
 3 files changed, 48 insertions(+), 18 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 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>
---
 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 ee4ca3c..bd21fff 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1519,10 +1519,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;
     }
 
@@ -1562,6 +1558,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);
@@ -1570,6 +1567,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 = (total_size - start) / BDRV_SECTOR_SIZE;
+    }
 
     ret = try_seek_hole(bs, start, &data, &hole, pnum);
     if (ret < 0) {
-- 
1.9.3

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

[Eric Blake]

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

On 10/22/2014 09:57 AM, Max Reitz wrote:
> 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>
> ---
>  block/raw-posix.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

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

> +    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 = (total_size - start) / BDRV_SECTOR_SIZE;

Should this round up instead of truncate?  But it would only matter for
a file size that is not a multiple of sectors, where we probably have
other issues, and where reporting just the full sectors also seems
reasonable.

-- 
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 v3 1/3] raw-posix: Fix raw_co_get_block_status() after EOF

[Max Reitz]

On 2014-10-22 at 18:57, Eric Blake wrote:
> On 10/22/2014 09:57 AM, Max Reitz wrote:
>> 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>
>> ---
>>   block/raw-posix.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +    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 = (total_size - start) / BDRV_SECTOR_SIZE;
> Should this round up instead of truncate?  But it would only matter for
> a file size that is not a multiple of sectors, where we probably have
> other issues, and where reporting just the full sectors also seems
> reasonable.

There already was a series (as far as I remember) that somehow tried to 
make all or at least some block drivers compatible with sizes which are 
not a multiple of the sector size, so I shouldn't be nullifying that 
work. Will use ROUND_UP().

Max

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

[Max Reitz]

On 2014-10-23 at 09:27, Max Reitz wrote:
> On 2014-10-22 at 18:57, Eric Blake wrote:
>> On 10/22/2014 09:57 AM, Max Reitz wrote:
>>> 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>
>>> ---
>>>   block/raw-posix.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>>> +    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 = (total_size - start) / BDRV_SECTOR_SIZE;
>> Should this round up instead of truncate?  But it would only matter for
>> a file size that is not a multiple of sectors, where we probably have
>> other issues, and where reporting just the full sectors also seems
>> reasonable.
>
> There already was a series (as far as I remember) that somehow tried 
> to make all or at least some block drivers compatible with sizes which 
> are not a multiple of the sector size, so I shouldn't be nullifying 
> that work. Will use ROUND_UP().

Sorry, DIV_ROUND_UP(), of course.

Max

[Qemu-devel] [PATCH v3 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>
---
 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 bd21fff..0058678 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1465,12 +1465,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;
@@ -1511,8 +1511,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;
@@ -1532,7 +1532,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
@@ -1559,7 +1559,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) {
@@ -1577,28 +1577,28 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
         nb_sectors = (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

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

[Eric Blake]

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

On 10/22/2014 09:57 AM, Max Reitz wrote:
> 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>
> ---
>  block/raw-posix.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

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

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

I'm not sure the assertion adds much (the lines above are fairly easy to
reason about ret always being set), but it does protect against later
code addition, so I'm not opposed to leaving it.

-- 
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 v3 2/3] raw-posix: raw_co_get_block_status() return value

[Max Reitz]

On 2014-10-22 at 19:00, Eric Blake wrote:
> On 10/22/2014 09:57 AM, Max Reitz wrote:
>> 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>
>> ---
>>   block/raw-posix.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> -    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);
> I'm not sure the assertion adds much (the lines above are fairly easy to
> reason about ret always being set), but it does protect against later
> code addition, so I'm not opposed to leaving it.

I just liked it there to make clear that the previous code always goes 
into a fallback, so while some parts of it may throw errors, after 
everything is done, we're fine. The compiler will hopefully optimize it 
out anyway.

Max

[Qemu-devel] [PATCH v3 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.

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

diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
index 34b363f..027198b 100755
--- a/tests/qemu-iotests/102
+++ b/tests/qemu-iotests/102
@@ -58,6 +58,21 @@ truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
 $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
+
+(sleep 0.2; $QEMU_IO -c map "$TEST_IMG"; $QEMU_IMG map "$TEST_IMG") &
+truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
+# To be sure the image has been truncated before $QEMU_IO and $QEMU_IMG run
+echo '--- truncated ---'
+
+sleep 0.3
+
 # 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..96f4a33 100644
--- a/tests/qemu-iotests/102.out
+++ b/tests/qemu-iotests/102.out
@@ -7,4 +7,13 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 [                       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)
+--- truncated ---
+[                       0]      128/     128 sectors     allocated at offset 0 bytes (1)
+Offset          Length          Mapped to       File
 *** done
-- 
1.9.3

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

[Eric Blake]

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

On 10/22/2014 09:57 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/102     | 15 +++++++++++++++
>  tests/qemu-iotests/102.out |  9 +++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
> index 34b363f..027198b 100755
> --- a/tests/qemu-iotests/102
> +++ b/tests/qemu-iotests/102
> @@ -58,6 +58,21 @@ truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
>  $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
> +
> +(sleep 0.2; $QEMU_IO -c map "$TEST_IMG"; $QEMU_IMG map "$TEST_IMG") &

Should you use '&&' instead of ';' between the three operations, to
ensure that you can detect failure of the overall background subshell? [1]

Fractional sleep is a GNU extension, and won't work on BSD.  It adds .8
seconds to make this sleep 1, but the extra portability may be worth it.
 It also makes the test more robust, and less likely to fail a race in a
heavily-loaded tester.  Then again, it is not the first use of
fractional sleep in the testsuite.

Hmm - does the blkdebug driver allow us to pause qemu operation to
reliably allow an external action callback, and then resume qemu?  That
might be less prone to race failure, as well as reducing the need to
blindly sleep for a fixed amount of time.

> +truncate -s $((5 * 64 * 1024)) "$TEST_IMG"

truncate is a GNU program, not necessarily available on all platforms;
but this is not the first test using it.

> +# To be sure the image has been truncated before $QEMU_IO and $QEMU_IMG run
> +echo '--- truncated ---'
> +
> +sleep 0.3
> +
>  # success, all done

I think you should have a 'wait' here to reap the background child
process before declaring this test all done.  Particularly if you take
my advice above[1] about detecting errors.

Looks like a good start, but I'm worried about false positives if you
can't guarantee some timing between actions.

-- 
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 v3 3/3] iotests: Add test for external image truncation

[Max Reitz]

On 2014-10-22 at 18:50, Eric Blake wrote:
> On 10/22/2014 09:57 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/102     | 15 +++++++++++++++
>>   tests/qemu-iotests/102.out |  9 +++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
>> index 34b363f..027198b 100755
>> --- a/tests/qemu-iotests/102
>> +++ b/tests/qemu-iotests/102
>> @@ -58,6 +58,21 @@ truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
>>   $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
>> +
>> +(sleep 0.2; $QEMU_IO -c map "$TEST_IMG"; $QEMU_IMG map "$TEST_IMG") &
> Should you use '&&' instead of ';' between the three operations, to
> ensure that you can detect failure of the overall background subshell? [1]

No, I don't think so. The output is compared against the test output and 
I probably want to have both the output of qemu-io -c map and qemu-img 
map, even if one fails.

> Fractional sleep is a GNU extension, and won't work on BSD.  It adds .8
> seconds to make this sleep 1, but the extra portability may be worth it.

Probably so, yes.

> It also makes the test more robust, and less likely to fail a race in a
> heavily-loaded tester.  Then again, it is not the first use of
> fractional sleep in the testsuite.
>
> Hmm - does the blkdebug driver allow us to pause qemu operation to
> reliably allow an external action callback, and then resume qemu?  That
> might be less prone to race failure, as well as reducing the need to
> blindly sleep for a fixed amount of time.

It does not yet. But when you're asking like this, I'm willing to build 
a time block driver which pauses one second for every sector you're 
reading from it.

Okay, so without kidding, I think to make this right, we could try to 
keep qemu-io open, do the truncate, and then continue writing commands 
to qemu-io. I think I can do that by not using qemu-io but qemu directly 
and then use the common.qemu functions (along with HMP qemu-io). Of 
course, this makes testing qemu-img map impossible, but using blkdebug 
would have done the same, probably. Also, just qemu-io -c map should be 
enough for this case.

>> +truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
> truncate is a GNU program, not necessarily available on all platforms;
> but this is not the first test using it.

Well, if it's not the first test, I'm inclined to leave it. But since 
I'm going to respin anyway and you're asking so kindly, I'll reach deep 
into my box of tricks and use qemu-img resize 
"json:{'driver':'raw','file':{'driver':'file','filename':'$TEST_IMG'}}" 
$((5 * 64 * 1024)).

Speaking of resize not taking a format, we should probably at some point 
in time add an input format parameter to all of the qemu-img subcommands 
(resize and snapshot don't have one yet).

>> +# To be sure the image has been truncated before $QEMU_IO and $QEMU_IMG run
>> +echo '--- truncated ---'
>> +
>> +sleep 0.3
>> +
>>   # success, all done
> I think you should have a 'wait' here to reap the background child
> process before declaring this test all done.  Particularly if you take
> my advice above[1] about detecting errors.

Will probably not take the advice (unless I failed to understand you 
correctly and you can convince me otherwise), but will use `wait` anyway.

> Looks like a good start, but I'm worried about false positives if you
> can't guarantee some timing between actions.

Yes, you're right. Thanks!

Max

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

[Kevin Wolf]

Am 23.10.2014 um 09:26 hat Max Reitz geschrieben:
> On 2014-10-22 at 18:50, Eric Blake wrote:
> >On 10/22/2014 09:57 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.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  tests/qemu-iotests/102     | 15 +++++++++++++++
> >>  tests/qemu-iotests/102.out |  9 +++++++++
> >>  2 files changed, 24 insertions(+)
> >>
> >>diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
> >>index 34b363f..027198b 100755
> >>--- a/tests/qemu-iotests/102
> >>+++ b/tests/qemu-iotests/102
> >>@@ -58,6 +58,21 @@ truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
> >>  $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
> >>+
> >>+(sleep 0.2; $QEMU_IO -c map "$TEST_IMG"; $QEMU_IMG map "$TEST_IMG") &
> >Should you use '&&' instead of ';' between the three operations, to
> >ensure that you can detect failure of the overall background subshell? [1]
> 
> No, I don't think so. The output is compared against the test output
> and I probably want to have both the output of qemu-io -c map and
> qemu-img map, even if one fails.
> 
> >Fractional sleep is a GNU extension, and won't work on BSD.  It adds .8
> >seconds to make this sleep 1, but the extra portability may be worth it.
> 
> Probably so, yes.
> 
> >It also makes the test more robust, and less likely to fail a race in a
> >heavily-loaded tester.  Then again, it is not the first use of
> >fractional sleep in the testsuite.
> >
> >Hmm - does the blkdebug driver allow us to pause qemu operation to
> >reliably allow an external action callback, and then resume qemu?  That
> >might be less prone to race failure, as well as reducing the need to
> >blindly sleep for a fixed amount of time.
> 
> It does not yet. But when you're asking like this, I'm willing to
> build a time block driver which pauses one second for every sector
> you're reading from it.
> 
> Okay, so without kidding, I think to make this right, we could try
> to keep qemu-io open, do the truncate, and then continue writing
> commands to qemu-io. I think I can do that by not using qemu-io but
> qemu directly and then use the common.qemu functions (along with HMP
> qemu-io). Of course, this makes testing qemu-img map impossible, but
> using blkdebug would have done the same, probably. Also, just
> qemu-io -c map should be enough for this case.

It should be possible to stop qemu-img at a good enough place with
blkdebug. The problem would just be how to resume...

I agree that the qemu-img test doesn't add much here anyway.

> >>+truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
> >truncate is a GNU program, not necessarily available on all platforms;
> >but this is not the first test using it.
> 
> Well, if it's not the first test, I'm inclined to leave it. But
> since I'm going to respin anyway and you're asking so kindly, I'll
> reach deep into my box of tricks and use qemu-img resize "json:{'driver':'raw','file':{'driver':'file','filename':'$TEST_IMG'}}"
> $((5 * 64 * 1024)).

What's wrong with 'qemu-img resize -f raw "$TEST_IMG"'? Also doesn't
hardcode the protocol.

> Speaking of resize not taking a format, we should probably at some
> point in time add an input format parameter to all of the qemu-img
> subcommands (resize and snapshot don't have one yet).

Well, _my_ resize does have one and has been there since resize was
introduced in 2010 (commit ae6b0ed6).

Kevin

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

[Max Reitz]

On 2014-10-23 at 09:46, Kevin Wolf wrote:
> Am 23.10.2014 um 09:26 hat Max Reitz geschrieben:
>> On 2014-10-22 at 18:50, Eric Blake wrote:
>>> On 10/22/2014 09:57 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.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/102     | 15 +++++++++++++++
>>>>   tests/qemu-iotests/102.out |  9 +++++++++
>>>>   2 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
>>>> index 34b363f..027198b 100755
>>>> --- a/tests/qemu-iotests/102
>>>> +++ b/tests/qemu-iotests/102
>>>> @@ -58,6 +58,21 @@ truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
>>>>   $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
>>>> +
>>>> +(sleep 0.2; $QEMU_IO -c map "$TEST_IMG"; $QEMU_IMG map "$TEST_IMG") &
>>> Should you use '&&' instead of ';' between the three operations, to
>>> ensure that you can detect failure of the overall background subshell? [1]
>> No, I don't think so. The output is compared against the test output
>> and I probably want to have both the output of qemu-io -c map and
>> qemu-img map, even if one fails.
>>
>>> Fractional sleep is a GNU extension, and won't work on BSD.  It adds .8
>>> seconds to make this sleep 1, but the extra portability may be worth it.
>> Probably so, yes.
>>
>>> It also makes the test more robust, and less likely to fail a race in a
>>> heavily-loaded tester.  Then again, it is not the first use of
>>> fractional sleep in the testsuite.
>>>
>>> Hmm - does the blkdebug driver allow us to pause qemu operation to
>>> reliably allow an external action callback, and then resume qemu?  That
>>> might be less prone to race failure, as well as reducing the need to
>>> blindly sleep for a fixed amount of time.
>> It does not yet. But when you're asking like this, I'm willing to
>> build a time block driver which pauses one second for every sector
>> you're reading from it.
>>
>> Okay, so without kidding, I think to make this right, we could try
>> to keep qemu-io open, do the truncate, and then continue writing
>> commands to qemu-io. I think I can do that by not using qemu-io but
>> qemu directly and then use the common.qemu functions (along with HMP
>> qemu-io). Of course, this makes testing qemu-img map impossible, but
>> using blkdebug would have done the same, probably. Also, just
>> qemu-io -c map should be enough for this case.
> It should be possible to stop qemu-img at a good enough place with
> blkdebug. The problem would just be how to resume...
>
> I agree that the qemu-img test doesn't add much here anyway.
>
>>>> +truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
>>> truncate is a GNU program, not necessarily available on all platforms;
>>> but this is not the first test using it.
>> Well, if it's not the first test, I'm inclined to leave it. But
>> since I'm going to respin anyway and you're asking so kindly, I'll
>> reach deep into my box of tricks and use qemu-img resize "json:{'driver':'raw','file':{'driver':'file','filename':'$TEST_IMG'}}"
>> $((5 * 64 * 1024)).
> What's wrong with 'qemu-img resize -f raw "$TEST_IMG"'? Also doesn't
> hardcode the protocol.
>
>> Speaking of resize not taking a format, we should probably at some
>> point in time add an input format parameter to all of the qemu-img
>> subcommands (resize and snapshot don't have one yet).
> Well, _my_ resize does have one and has been there since resize was
> introduced in 2010 (commit ae6b0ed6).

Tell that to my qemu-img --help. :-P

Max