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

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

[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

[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>
Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/io.c b/block/io.c
index 11fcfcb..2177cc6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2264,6 +2264,11 @@ 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);
 
+    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.1.4

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

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

[Stefan Hajnoczi]

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

On Thu, Jun 16, 2016 at 07:09:38PM +0300, 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.
> 
> 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(-)

Makes sense to me so I'm merging it provisionally instead of waiting for
more reviews.  If there are objections I will revert it so that
discussion can continue.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Kevin Wolf]

Am 16.06.2016 um 18:09 hat Denis V. Lunev geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>