[Qemu-devel] [PATCH 0/3] potentially broken drive-mirror/drive-backup in bdrv_co_discard

[Qemu-devel] [PATCH 0/3] potentially broken drive-mirror/drive-backup in bdrv_co_discard

[Denis V. Lunev]

Actually I have found this problem running iotest 132 for active async
mirror I have sent yesturday. Anyway, the problem is actual for current
backup/mirror implementation.

bdrv_co_discard must mark areas dirty after writing zeroes, it must call
before_write_notifier chain to push underlying data to backup and it also
must properly fill tracked request information.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH 1/3] block: fixed BdrvTrackedRequest filling in bdrv_co_discard

[Denis V. Lunev]

The request is area is specified in bytes, not in sectors.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index afb99c4..b51f681 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2263,8 +2263,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
-    tracked_request_begin(&req, bs, NULL, sector_num, nb_sectors,
-                          BDRV_TRACKED_DISCARD);
+    tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
+                          nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
     bdrv_set_dirty(bs, sector_num, nb_sectors);
 
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
-- 
2.5.0

[Qemu-devel] [PATCH 2/3] block: fix race in bdrv_co_discard with drive-mirror

[Denis V. Lunev]

Actually we must set dirty bitmap dirty after we have written all our
zeroes for correct processing in drive mirror code. In the other case
we can face not zeroes in this dirty area there in mirror_iteration.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index b51f681..513bd99 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2265,7 +2265,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
 
     tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
                           nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
-    bdrv_set_dirty(bs, sector_num, nb_sectors);
 
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
     while (nb_sectors > 0) {
@@ -2314,6 +2313,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
     }
     ret = 0;
 out:
+    bdrv_set_dirty(bs, req.offset << BDRV_SECTOR_BITS,
+                   req.bytes << BDRV_SECTOR_BITS);
     tracked_request_end(&req);
     return ret;
 }
-- 
2.5.0

[Qemu-devel] [PATCH 3/3] block: process before_write_notifiers in bdrv_co_discard

[Denis V. Lunev]

This is mandatory for correct backup creation. In the other case the
content under this area would be lost.

Dirty bits are set exactly like in bdrv_aligned_pwritev, i.e. they are set
even if notifier has returned a error.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/io.c b/block/io.c
index 513bd99..92d9fe3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2266,6 +2266,11 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
     tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
                           nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
 
+    ret = notifier_with_return_list_notify(&bs->before_write_notifiers, &req);
+    if (ret < 0) {
+        goto out;
+    }
+
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
     while (nb_sectors > 0) {
         int ret;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] block: fixed BdrvTrackedRequest filling in bdrv_co_discard

[Fam Zheng]

On Thu, 06/16 09:58, Denis V. Lunev wrote:
> The request is area is specified in bytes, not in sectors.

s/request is/request/

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index afb99c4..b51f681 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2263,8 +2263,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>          return 0;
>      }
>  
> -    tracked_request_begin(&req, bs, NULL, sector_num, nb_sectors,
> -                          BDRV_TRACKED_DISCARD);
> +    tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
> +                          nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
>      bdrv_set_dirty(bs, sector_num, nb_sectors);
>  
>      max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
> -- 
> 2.5.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] block: fix race in bdrv_co_discard with drive-mirror

[Fam Zheng]

On Thu, 06/16 09:58, Denis V. Lunev wrote:
> Actually we must set dirty bitmap dirty after we have written all our
> zeroes for correct processing in drive mirror code. In the other case
> we can face not zeroes in this dirty area there in mirror_iteration.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index b51f681..513bd99 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2265,7 +2265,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>  
>      tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
>                            nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
> -    bdrv_set_dirty(bs, sector_num, nb_sectors);
>  
>      max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>      while (nb_sectors > 0) {
> @@ -2314,6 +2313,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>      }
>      ret = 0;
>  out:
> +    bdrv_set_dirty(bs, req.offset << BDRV_SECTOR_BITS,
> +                   req.bytes << BDRV_SECTOR_BITS);
>      tracked_request_end(&req);
>      return ret;
>  }
> -- 
> 2.5.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] block: process before_write_notifiers in bdrv_co_discard

[Fam Zheng]

On Thu, 06/16 09:58, Denis V. Lunev wrote:
> This is mandatory for correct backup creation. In the other case the
> content under this area would be lost.
> 
> Dirty bits are set exactly like in bdrv_aligned_pwritev, i.e. they are set
> even if notifier has returned a error.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 513bd99..92d9fe3 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2266,6 +2266,11 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>      tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
>                            nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
>  
> +    ret = notifier_with_return_list_notify(&bs->before_write_notifiers, &req);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>      max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>      while (nb_sectors > 0) {
>          int ret;
> -- 
> 2.5.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] block: fix race in bdrv_co_discard with drive-mirror

[Vladimir Sementsov-Ogievskiy]

On 16.06.2016 09:58, Denis V. Lunev wrote:
> Actually we must set dirty bitmap dirty after we have written all our
> zeroes for correct processing in drive mirror code. In the other case
> we can face not zeroes in this dirty area there in mirror_iteration.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>   block/io.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/io.c b/block/io.c
> index b51f681..513bd99 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2265,7 +2265,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>   
>       tracked_request_begin(&req, bs, NULL, sector_num << BDRV_SECTOR_BITS,
>                             nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
> -    bdrv_set_dirty(bs, sector_num, nb_sectors);
>   
>       max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>       while (nb_sectors > 0) {
> @@ -2314,6 +2313,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>       }
>       ret = 0;
>   out:
> +    bdrv_set_dirty(bs, req.offset << BDRV_SECTOR_BITS,
  s/<</>>/
> +                   req.bytes << BDRV_SECTOR_BITS);
and here
>       tracked_request_end(&req);
>       return ret;
>   }


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] block: fix race in bdrv_co_discard with drive-mirror

[Denis V. Lunev]

On 06/16/2016 12:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 16.06.2016 09:58, Denis V. Lunev wrote:
>> Actually we must set dirty bitmap dirty after we have written all our
>> zeroes for correct processing in drive mirror code. In the other case
>> we can face not zeroes in this dirty area there in mirror_iteration.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Fam Zheng <famz@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/io.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index b51f681..513bd99 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2265,7 +2265,6 @@ int coroutine_fn 
>> bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>         tracked_request_begin(&req, bs, NULL, sector_num << 
>> BDRV_SECTOR_BITS,
>>                             nb_sectors << BDRV_SECTOR_BITS, 
>> BDRV_TRACKED_DISCARD);
>> -    bdrv_set_dirty(bs, sector_num, nb_sectors);
>>         max_discard = MIN_NON_ZERO(bs->bl.max_discard, 
>> BDRV_REQUEST_MAX_SECTORS);
>>       while (nb_sectors > 0) {
>> @@ -2314,6 +2313,8 @@ int coroutine_fn 
>> bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>       }
>>       ret = 0;
>>   out:
>> +    bdrv_set_dirty(bs, req.offset << BDRV_SECTOR_BITS,
>  s/<</>>/
>> +                   req.bytes << BDRV_SECTOR_BITS);
> and here
>>       tracked_request_end(&req);
>>       return ret;
>>   }
>
>
very nice, thanks ;)

Re: [Qemu-devel] [PATCH 0/3] potentially broken drive-mirror/drive-backup in bdrv_co_discard

[Paolo Bonzini]

On 16/06/2016 08:58, Denis V. Lunev wrote:
> Actually I have found this problem running iotest 132 for active async
> mirror I have sent yesturday. Anyway, the problem is actual for current
> backup/mirror implementation.
> 
> bdrv_co_discard must mark areas dirty after writing zeroes, it must call
> before_write_notifier chain to push underlying data to backup and it also
> must properly fill tracked request information.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>

Three good catches!  Thanks for these patches.  I think 2 and 3 should
be Cced to qemu-stable too.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 1/3] block: fixed BdrvTrackedRequest filling in bdrv_co_discard

[Eric Blake]

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

On 06/16/2016 10:09 AM, Denis V. Lunev wrote:
> The request area is specified in bytes, not in sectors.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

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

[By the way, next on my list of sector-to-byte conversions is discard,
so thanks for finding this first]

-- 
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: 604 bytes --]

[Qemu-devel] [PATCH 1/3] block: fixed BdrvTrackedRequest filling in bdrv_co_discard

[Denis V. Lunev]

The request area is specified in bytes, not in sectors.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index fb99a71..aca175e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2261,8 +2261,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
-    tracked_request_begin(&req, bs, sector_num, nb_sectors,
-                          BDRV_TRACKED_DISCARD);
+    tracked_request_begin(&req, bs, sector_num << BDRV_SECTOR_BITS,
+                          nb_sectors << BDRV_SECTOR_BITS, BDRV_TRACKED_DISCARD);
     bdrv_set_dirty(bs, sector_num, nb_sectors);
 
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
-- 
2.1.4