Don't write headers if BDS is INACTIVE

The on disk image format 'inuse' header field is updated blindly if the image is opened RDWR. This can cause problems if the QEMU runstate is set to INMIGRATE, at which point the underlying file is set to INACTIVE. This causes an assert in bdrv_co_pwritev().

Do something similar to what is done in VHDX; latch the first write, and update the header the first time we modify the file.

Signed-off-by Jeff Cody <[email protected]>

Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:

Quoted 5 lines

The on disk image format 'inuse' header field is updated blindly if the image is opened RDWR. This can cause problems if the QEMU runstate is set to INMIGRATE, at which point the underlying file is set to INACTIVE. This causes an assert in bdrv_co_pwritev().

Quoted 4 lines

Do something similar to what is done in VHDX; latch the first write, and update the header the first time we modify the file.

Signed-off-by Jeff Cody <[email protected]>

For VHDX, it seems that we have to have the header update in the write path anyway, so it might be justifiable, but I think for parallels, it's just ugly.

The conservative approach to this would be doing the header write in .bdrv_open() only if BDRV_O_INACTIVE is cleared, and otherwise do it during .bdrv_invalidate_cache().

By the way, random design thought: It might make sense to change .bdrv_open() so that it always opens inactive images and then call .bdrv_invalidate_cache() (possibly renamed to .bdrv_activate()) unconditionally even without migration.

Kevin

Quoted 6 lines

Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:

Quoted 5 lines

The on disk image format 'inuse' header field is updated blindly if the image is opened RDWR. This can cause problems if the QEMU runstate is set to INMIGRATE, at which point the underlying file is set to INACTIVE. This causes an assert in bdrv_co_pwritev().

Quoted 9 lines

Quoted 4 lines

Do something similar to what is done in VHDX; latch the first write, and update the header the first time we modify the file.

Signed-off-by Jeff Cody <[email protected]>

For VHDX, it seems that we have to have the header update in the write path anyway, so it might be justifiable, but I think for parallels, it's just ugly.

A bit ugly. I think we could get around VHDX needing to do it as well; it does it in the write path for two scenarios:

* First normal write, or
* Journal log replay, if dirty, on open (if r/w)

The log check happens early in vhdx_open(). If it does not write anything, then we can just write the headers during the open like normal, if we are R/W (and !BDRV_O_INACTIVE, of course).

Quoted 3 lines

The conservative approach to this would be doing the header write in .bdrv_open() only if BDRV_O_INACTIVE is cleared, and otherwise do it during .bdrv_invalidate_cache().

What scenarios cause BDRV_O_INACTIVE to not be set on bs, but set on bs->file-bs?

Quoted 7 lines

By the way, random design thought: It might make sense to change .bdrv_open() so that it always opens inactive images and then call .bdrv_invalidate_cache() (possibly renamed to .bdrv_activate()) unconditionally even without migration.

Kevin

The on disk image format 'inuse' header field is updated blindly if the image is opened RDWR. This can cause problems if the QEMU runstate is set to INMIGRATE, at which point the underlying file is set to INACTIVE. This causes an assert in bdrv_co_pwritev().

Do something similar to what is done in VHDX; latch the first write, and update the header the first time we modify the file.

Signed-off-by Jeff Cody <[email protected]>

---
 block/parallels.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

block/parallels.c +17 -4

index fed199eccd..c560e2fcf2 100644
@@ -100,6 +100,8 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+
+    bool first_write_latch;
 } BDRVParallelsState;
 
 

@@ -317,6 +319,16 @@ static coroutine_fn int parallels_co_writev(BlockDriverState *bs,
     QEMUIOVector hd_qiov;
     int ret = 0;
 
+    if (s->first_write_latch) {
+        s->first_write_latch = false;
+        qemu_co_mutex_lock(&s->lock);
+        ret = parallels_update_header(bs);
+        qemu_co_mutex_unlock(&s->lock);
+    }
+    if (ret < 0) {
+        return ret;
+    }
+
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
     while (nb_sectors > 0) {

@@ -416,6 +428,9 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
             /* parallels_close will do the job right */
             res->corruptions_fixed++;
             s->header_unclean = false;
+            /* set that a write has occurred, so that parallels_close() will
+             * update the inuse field in the header */
+            s->first_write_latch = false;
         }
     }
 

@@ -597,6 +612,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     char *buf;
 
+    s->first_write_latch = true;
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {

@@ -710,10 +727,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (flags & BDRV_O_RDWR) {
         s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
-        ret = parallels_update_header(bs);
-        if (ret < 0) {
-            goto fail;
-        }
     }
 
     s->bat_dirty_block = 4 * getpagesize();