[PATCH 1/2] block: zero data data corruption using prealloc-filter

[Andrey Drobyshev]

From: "Denis V. Lunev" <den@openvz.org>

We have observed that some clusters in the QCOW2 files are zeroed
while preallocation filter is used.

We are able to trace down the following sequence when prealloc-filter
is used:
    co=0x55e7cbed7680 qcow2_co_pwritev_task()
    co=0x55e7cbed7680 preallocate_co_pwritev_part()
    co=0x55e7cbed7680 handle_write()
    co=0x55e7cbed7680 bdrv_co_do_pwrite_zeroes()
    co=0x55e7cbed7680 raw_do_pwrite_zeroes()
    co=0x7f9edb7fe500 do_fallocate()

Here coroutine 0x55e7cbed7680 is being blocked waiting while coroutine
0x7f9edb7fe500 will finish with fallocate of the file area. OK. It is
time to handle next coroutine, which
    co=0x55e7cbee91b0 qcow2_co_pwritev_task()
    co=0x55e7cbee91b0 preallocate_co_pwritev_part()
    co=0x55e7cbee91b0 handle_write()
    co=0x55e7cbee91b0 bdrv_co_do_pwrite_zeroes()
    co=0x55e7cbee91b0 raw_do_pwrite_zeroes()
    co=0x7f9edb7deb00 do_fallocate()

The trouble comes here. Coroutine 0x55e7cbed7680 has not advanced
file_end yet and coroutine 0x55e7cbee91b0 will start fallocate() for
the same area. This means that if (once fallocate is started inside
0x7f9edb7deb00) original fallocate could end and the real write will
be executed. In that case write() request is handled at the same time
as fallocate().

Normally we should protect s->file_end while it is detected that
preallocation is need. The patch introduces file_end_lock for it to be
protected when run in the coroutine context.

Note: the lock is taken only once it is detected that the preallocation
is really required. This is not a frequent case due to the preallocation
nature thus the patch should not have performance impact.

Originally-by: Denis V. Lunev <den@openvz.org>
Co-authored-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/preallocate.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/preallocate.c b/block/preallocate.c
index d215bc5d6d..9cb2c97635 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -78,6 +78,8 @@ typedef struct BDRVPreallocateState {
 
     /* Gives up the resize permission on children when parents don't need it */
     QEMUBH *drop_resize_bh;
+
+    CoMutex file_end_lock;
 } BDRVPreallocateState;
 
 static int preallocate_drop_resize(BlockDriverState *bs, Error **errp);
@@ -170,6 +172,8 @@ static int preallocate_open(BlockDriverState *bs, QDict 
*options, int flags,
         ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
             bs->file->bs->supported_zero_flags);
 
+    qemu_co_mutex_init(&s->file_end_lock);
+
     return 0;
 }
 
@@ -342,6 +346,7 @@ handle_write(BlockDriverState *bs, int64_t offset, int64_t 
bytes,
             return false;
         }
 
+        QEMU_LOCK_GUARD(&s->file_end_lock);
         if (s->file_end < 0) {
             s->file_end = s->data_end;
         }
@@ -353,6 +358,8 @@ handle_write(BlockDriverState *bs, int64_t offset, int64_t 
bytes,
 
     /* We have valid s->data_end, and request writes beyond it. */
 
+    QEMU_LOCK_GUARD(&s->file_end_lock);
+
     s->data_end = end;
     if (s->zero_start < 0 || !want_merge_zero) {
         s->zero_start = end;
@@ -428,6 +435,8 @@ preallocate_co_truncate(BlockDriverState *bs, int64_t 
offset,
     BDRVPreallocateState *s = bs->opaque;
     int ret;
 
+    QEMU_LOCK_GUARD(&s->file_end_lock);
+
     if (s->data_end >= 0 && offset > s->data_end) {
         if (s->file_end < 0) {
             s->file_end = bdrv_co_getlength(bs->file->bs);
@@ -501,6 +510,8 @@ preallocate_co_getlength(BlockDriverState *bs)
         return s->data_end;
     }
 
+    QEMU_LOCK_GUARD(&s->file_end_lock);
+
     ret = bdrv_co_getlength(bs->file->bs);
 
     if (has_prealloc_perms(bs)) {
-- 
2.39.3