[v4] block: Fix resize (extending) of short overlays

[PATCH v4 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

Posted by Kevin Wolf 5 years, 9 months ago

If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
undo any previous preallocation, but just adds the zero flag to all
relevant L2 entries. If an external data file is in use, a write_zeroes
request to the data file is made instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6c6d6101ce..7a70c1c090 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     bs->supported_zero_flags = header.version >= 3 ?
                                BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
+    bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
 
     /* Repair image if dirty */
     if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
@@ -4213,6 +4214,14 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         g_assert_not_reached();
     }
 
+    if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
+        ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to zero out the new area");
+            goto fail;
+        }
+    }
+
     if (prealloc != PREALLOC_MODE_OFF) {
         /* Flush metadata before actually changing the image size */
         ret = qcow2_write_caches(bs);
-- 
2.20.1

Re: [PATCH v4 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

Posted by Alberto Garcia 5 years, 9 months ago

On Mon 20 Apr 2020 03:32:09 PM CEST, Kevin Wolf wrote:
> If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
> qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
> undo any previous preallocation, but just adds the zero flag to all
> relevant L2 entries. If an external data file is in use, a write_zeroes
> request to the data file is made instead.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v4 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

Posted by Vladimir Sementsov-Ogievskiy 5 years, 9 months ago

20.04.2020 16:32, Kevin Wolf wrote:
> If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
> qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
> undo any previous preallocation, but just adds the zero flag to all
> relevant L2 entries. If an external data file is in use, a write_zeroes
> request to the data file is made instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6c6d6101ce..7a70c1c090 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>   
>       bs->supported_zero_flags = header.version >= 3 ?
>                                  BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
> +    bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
>   
>       /* Repair image if dirty */
>       if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
> @@ -4213,6 +4214,14 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>           g_assert_not_reached();
>       }
>   
> +    if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
> +        ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0);

Hmm. As I understand, qcow2_cluster_zeroize is unprepared to cluster-unaligned offset/size. I think we should handle it somehow.

> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Failed to zero out the new area");
> +            goto fail;
> +        }
> +    }
> +
>       if (prealloc != PREALLOC_MODE_OFF) {
>           /* Flush metadata before actually changing the image size */
>           ret = qcow2_write_caches(bs);
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

Posted by Alberto Garcia 5 years, 9 months ago

On Tue 21 Apr 2020 10:47:17 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> +    if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
>> +        ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0);
>
> Hmm. As I understand, qcow2_cluster_zeroize is unprepared to
> cluster-unaligned offset/size. I think we should handle it somehow.

You're right, it actually hits an assertion :-/

I suppose you can simply round the size up to the next cluster.

Berto

Re: [PATCH v4 4/9] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

Posted by Vladimir Sementsov-Ogievskiy 5 years, 9 months ago

21.04.2020 13:50, Alberto Garcia wrote:
> On Tue 21 Apr 2020 10:47:17 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> +    if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
>>> +        ret = qcow2_cluster_zeroize(bs, old_length, offset - old_length, 0);
>>
>> Hmm. As I understand, qcow2_cluster_zeroize is unprepared to
>> cluster-unaligned offset/size. I think we should handle it somehow.
> 
> You're right, it actually hits an assertion :-/
> 
> I suppose you can simply round the size up to the next cluster.
> 


But we also need to handle unaligned old_length, we can't just align it down, we should write zero to existing cluster if it is NORMAL cluster..


-- 
Best regards,
Vladimir