[PATCH v2] block/backup-top: fix flags handling

[PATCH v2] block/backup-top: fix flags handling

[Vladimir Sementsov-Ogievskiy]

backup-top "supports" write-unchanged, by skipping CBW operation in
backup_top_co_pwritev. But it forgets to do the same in
backup_top_co_pwrite_zeroes, as well as declare support for
BDRV_REQ_WRITE_UNCHANGED.

Fix this, and, while being here, declare also support for flags
supported by source child.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

v2: restrict flags propagation like it is done in other filters [Eric]
    move state variable initialization to the top

 block/backup-top.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 9aed2eb4c0..a4cec60859 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -48,11 +48,17 @@ static coroutine_fn int backup_top_co_preadv(
 }
 
 static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
-                                       uint64_t bytes)
+                                       uint64_t bytes, BdrvRequestFlags flags)
 {
     BDRVBackupTopState *s = bs->opaque;
-    uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
-    uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
+    uint64_t off, end;
+
+    if (flags & BDRV_REQ_WRITE_UNCHANGED) {
+        return 0;
+    }
+
+    off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
+    end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
 
     return block_copy(s->bcs, off, end - off, NULL);
 }
@@ -60,7 +66,7 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
 static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
                                                int64_t offset, int bytes)
 {
-    int ret = backup_top_cbw(bs, offset, bytes);
+    int ret = backup_top_cbw(bs, offset, bytes, 0);
     if (ret < 0) {
         return ret;
     }
@@ -71,7 +77,7 @@ static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
 static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs,
         int64_t offset, int bytes, BdrvRequestFlags flags)
 {
-    int ret = backup_top_cbw(bs, offset, bytes);
+    int ret = backup_top_cbw(bs, offset, bytes, flags);
     if (ret < 0) {
         return ret;
     }
@@ -84,11 +90,9 @@ static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs,
                                               uint64_t bytes,
                                               QEMUIOVector *qiov, int flags)
 {
-    if (!(flags & BDRV_REQ_WRITE_UNCHANGED)) {
-        int ret = backup_top_cbw(bs, offset, bytes);
-        if (ret < 0) {
-            return ret;
-        }
+    int ret = backup_top_cbw(bs, offset, bytes, flags);
+    if (ret < 0) {
+        return ret;
     }
 
     return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
@@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
                                          Error **errp)
 {
     Error *local_err = NULL;
-    BDRVBackupTopState *state;
     BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
                                                  filter_node_name,
                                                  BDRV_O_RDWR, errp);
+    BDRVBackupTopState *state = top->opaque;
 
     if (!top) {
         return NULL;
     }
 
     top->total_sectors = source->total_sectors;
-    state = top->opaque;
+    top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+            (BDRV_REQ_FUA & source->supported_write_flags);
+    top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+            ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+             source->supported_zero_flags);
 
     bdrv_ref(target);
     state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
-- 
2.21.0

Re: [PATCH v2] block/backup-top: fix flags handling

[Eric Blake]

On 2/3/20 7:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> backup-top "supports" write-unchanged, by skipping CBW operation in
> backup_top_co_pwritev. But it forgets to do the same in
> backup_top_co_pwrite_zeroes, as well as declare support for
> BDRV_REQ_WRITE_UNCHANGED.
> 
> Fix this, and, while being here, declare also support for flags
> supported by source child.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] block/backup-top: fix flags handling

[Vladimir Sementsov-Ogievskiy]

03.02.2020 18:42, Eric Blake wrote:
> On 2/3/20 7:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>> backup-top "supports" write-unchanged, by skipping CBW operation in
>> backup_top_co_pwritev. But it forgets to do the same in
>> backup_top_co_pwrite_zeroes, as well as declare support for
>> BDRV_REQ_WRITE_UNCHANGED.
>>
>> Fix this, and, while being here, declare also support for flags
>> supported by source child.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

-- 
Best regards,
Vladimir

Re: [PATCH v2] block/backup-top: fix flags handling

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1665 bytes --]

On 03.02.20 14:42, Vladimir Sementsov-Ogievskiy wrote:
> backup-top "supports" write-unchanged, by skipping CBW operation in
> backup_top_co_pwritev. But it forgets to do the same in
> backup_top_co_pwrite_zeroes, as well as declare support for
> BDRV_REQ_WRITE_UNCHANGED.
> 
> Fix this, and, while being here, declare also support for flags
> supported by source child.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> v2: restrict flags propagation like it is done in other filters [Eric]
>     move state variable initialization to the top
> 
>  block/backup-top.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 9aed2eb4c0..a4cec60859 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c


[...]

> @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>                                           Error **errp)
>  {
>      Error *local_err = NULL;
> -    BDRVBackupTopState *state;
>      BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>                                                   filter_node_name,
>                                                   BDRV_O_RDWR, errp);
> +    BDRVBackupTopState *state = top->opaque;
>  
>      if (!top) {
>          return NULL;
>      }

If top can be NULL, then we shouldn’t dereference it before (with
state = top->opaque).

(Pulling up the initialization of @state is also unrelated to this
patch, I don’t exactly know why you’re doing it here.)

Max


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

Re: [PATCH v2] block/backup-top: fix flags handling

[Vladimir Sementsov-Ogievskiy]

06.02.2020 19:31, Max Reitz wrote:
> On 03.02.20 14:42, Vladimir Sementsov-Ogievskiy wrote:
>> backup-top "supports" write-unchanged, by skipping CBW operation in
>> backup_top_co_pwritev. But it forgets to do the same in
>> backup_top_co_pwrite_zeroes, as well as declare support for
>> BDRV_REQ_WRITE_UNCHANGED.
>>
>> Fix this, and, while being here, declare also support for flags
>> supported by source child.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> v2: restrict flags propagation like it is done in other filters [Eric]
>>      move state variable initialization to the top
>>
>>   block/backup-top.c | 32 ++++++++++++++++++++------------
>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/backup-top.c b/block/backup-top.c
>> index 9aed2eb4c0..a4cec60859 100644
>> --- a/block/backup-top.c
>> +++ b/block/backup-top.c
> 
> 
> [...]
> 
>> @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>                                            Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    BDRVBackupTopState *state;
>>       BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>>                                                    filter_node_name,
>>                                                    BDRV_O_RDWR, errp);
>> +    BDRVBackupTopState *state = top->opaque;
>>   
>>       if (!top) {
>>           return NULL;
>>       }
> 
> If top can be NULL, then we shouldn’t dereference it before (with
> state = top->opaque).

Oh yes.

> 
> (Pulling up the initialization of @state is also unrelated to this
> patch, I don’t exactly know why you’re doing it here.)
> 

I just though, that it is small good style enhancement, but it isn't :)

Let's keep state variable handling as it was.

-- 
Best regards,
Vladimir