[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

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>

[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

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

[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 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 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

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

Changes from v1:
- fixed problem in patch as pointed out by Vova
- ported to current (was made on top of active block mirror)
- minor spelling changes in commit messages

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>

Denis V. Lunev (3):
  block: fixed BdrvTrackedRequest filling in bdrv_co_discard
  block: fix race in bdrv_co_discard with drive-mirror
  block: process before_write_notifiers in bdrv_co_discard

 block/io.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.1.4

[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 area in mirror_iteration.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index aca175e..11fcfcb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2263,7 +2263,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
 
     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);
     while (nb_sectors > 0) {
@@ -2312,6 +2311,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.1.4