qcow2: Fix qcow2_make_empty() with external data file
diff mbox series

Message ID 20190429105741.31033-1-kwolf@redhat.com
State New
Headers show
Series
  • qcow2: Fix qcow2_make_empty() with external data file
Related show

Commit Message

Kevin Wolf April 29, 2019, 10:57 a.m. UTC
make_completely_empty() is an optimisated path for bdrv_make_empty()
where completely new metadata is created inside the image file instead
of going through all clusters and discarding them. For an external data
file, however, we actually need to do discard operations on the data
file; just overwriting the qcow2 file doesn't get rid of the data.

The necessary slow path with an explicit discard operation already
exists for other cases. Use it for external data files, too.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kevin Wolf April 29, 2019, 11:21 a.m. UTC | #1
Am 29.04.2019 um 12:57 hat Kevin Wolf geschrieben:
> make_completely_empty() is an optimisated path for bdrv_make_empty()
> where completely new metadata is created inside the image file instead
> of going through all clusters and discarding them. For an external data
> file, however, we actually need to do discard operations on the data
> file; just overwriting the qcow2 file doesn't get rid of the data.
> 
> The necessary slow path with an explicit discard operation already
> exists for other cases. Use it for external data files, too.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7fbef97aab..097fde56f9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4384,7 +4384,8 @@ static int qcow2_make_empty(BlockDriverState *bs)
>  
>      if (s->qcow_version >= 3 && !s->snapshots && !s->nb_bitmaps &&
>          3 + l1_clusters <= s->refcount_block_size &&
> -        s->crypt_method_header != QCOW_CRYPT_LUKS) {
> +        s->crypt_method_header != QCOW_CRYPT_LUKS &&
> +        !has_data_file(bs)) {
>          /* The following function only works for qcow2 v3 images (it
>           * requires the dirty flag) and only as long as there are no
>           * features that reserve extra clusters (such as snapshots,

Oops, I hadn't everything committed yet. I'll add a comment change as
well:

          * LUKS header, or persistent bitmaps), because it completely
          * empties the image.  Furthermore, the L1 table and three
          * additional clusters (image header, refcount table, one
-         * refcount block) have to fit inside one refcount block. */
+         * refcount block) have to fit inside one refcount block. It
+         * only resets the image file, i.e. does not work with an
+         * external data file. */
         return make_completely_empty(bs);
     }

Kevin
Eric Blake April 29, 2019, 2:40 p.m. UTC | #2
On 4/29/19 6:21 AM, Kevin Wolf wrote:
> Am 29.04.2019 um 12:57 hat Kevin Wolf geschrieben:
>> make_completely_empty() is an optimisated path for bdrv_make_empty()
>> where completely new metadata is created inside the image file instead
>> of going through all clusters and discarding them. For an external data
>> file, however, we actually need to do discard operations on the data
>> file; just overwriting the qcow2 file doesn't get rid of the data.
>>
>> The necessary slow path with an explicit discard operation already
>> exists for other cases. Use it for external data files, too.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/qcow2.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)

> Oops, I hadn't everything committed yet. I'll add a comment change as
> well:

With the comment change squashed in,
Reviewed-by: Eric Blake <eblake@redhat.com>
Stefan Hajnoczi April 29, 2019, 4:13 p.m. UTC | #3
On Mon, Apr 29, 2019 at 12:57:41PM +0200, Kevin Wolf wrote:
> make_completely_empty() is an optimisated path for bdrv_make_empty()
> where completely new metadata is created inside the image file instead
> of going through all clusters and discarding them. For an external data
> file, however, we actually need to do discard operations on the data
> file; just overwriting the qcow2 file doesn't get rid of the data.
> 
> The necessary slow path with an explicit discard operation already
> exists for other cases. Use it for external data files, too.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
no-reply@patchew.org April 30, 2019, 1:54 p.m. UTC | #4
Patchew URL: https://patchew.org/QEMU/20190429105741.31033-1-kwolf@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190429105741.31033-1-kwolf@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Eric Blake April 30, 2019, 3:32 p.m. UTC | #5
On 4/30/19 8:54 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190429105741.31033-1-kwolf@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
> 
> 
> 
> The full log is available at

Patchew trimmed a bit too much; also, the error appears to be transient
and unrelated to the patch at hand. Quoting from:

> http://patchew.org/logs/20190429105741.31033-1-kwolf@redhat.com/testing.docker-mingw@fedora/?type=message.



Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: git fetch_pack: expected ACK/NAK, got 'ERR upload-pack: not our
ref 1cd70b1217a3e02617dbba76d15d21be1e8e4aa0'
fatal: The remote end hung up unexpectedly
error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
Traceback (most recent call last):
  File "patchew-tester/src/patchew-cli", line 521, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester/src/patchew-cli", line 48, in git_clone_repo
    stdout=logf, stderr=logf)
  File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f',
'--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384',
'https://github.com/patchew-project/qemu']' returned non-zero exit status 1


> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
>
no-reply@patchew.org April 30, 2019, 3:42 p.m. UTC | #6
Patchew URL: https://patchew.org/QEMU/20190429105741.31033-1-kwolf@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190429105741.31033-1-kwolf@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Patch
diff mbox series

diff --git a/block/qcow2.c b/block/qcow2.c
index 7fbef97aab..097fde56f9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4384,7 +4384,8 @@  static int qcow2_make_empty(BlockDriverState *bs)
 
     if (s->qcow_version >= 3 && !s->snapshots && !s->nb_bitmaps &&
         3 + l1_clusters <= s->refcount_block_size &&
-        s->crypt_method_header != QCOW_CRYPT_LUKS) {
+        s->crypt_method_header != QCOW_CRYPT_LUKS &&
+        !has_data_file(bs)) {
         /* The following function only works for qcow2 v3 images (it
          * requires the dirty flag) and only as long as there are no
          * features that reserve extra clusters (such as snapshots,