[Qemu-devel] [PATCH 0/4] Don't write headers if BDS is INACTIVE

[Qemu-devel] [PATCH 0/4] Don't write headers if BDS is INACTIVE

[Jeff Cody]

VHDX and Parallels both blindly write headers to the image file
if the images are opened R/W.  This causes an assert if the QEMU run
state is INMIGRATE.  Rather than blindly write on open, latch the first
write to the image, and update the header then.

Jeff Cody (4):
  block/vhdx.c: Don't blindly update the header
  block/parallels: code movement
  block/parallels: Don't update header until the first actual write
  qemu-iotests: update unsupported image formats in 194

 block/parallels.c      | 49 ++++++++++++++++++++++++++++++++-----------------
 block/vhdx.c           |  7 -------
 tests/qemu-iotests/194 |  2 +-
 3 files changed, 33 insertions(+), 25 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/4] block/vhdx.c: Don't blindly update the header

[Jeff Cody]

The VHDX specification requires that before user data modification of
the vhdx image, the VHDX header file and data GUIDs need to be updated.
In vhdx_open(), if the image is set to RDWR, we go ahead and update the
header.

However, just because the image is set to RDWR does not mean we can go
ahead and write at this point - specifically, if the QEMU run state is
INMIGRATE, the underlying file BS may be set to inactive via the BDS
open flag of BDRV_O_INACTIVE.  Attempting to write under this condition
will cause an assert in bdrv_co_pwritev().

We can alternatively latch the first time the image is written.  And lo
and behold, we do just that, via vhdx_user_visible_write() in
vhdx_co_writev().  This means the call to vhdx_update_headers() in
vhdx_open() is likely just vestigial, and can be removed.

Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 7ae4589879..9956933da6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1008,13 +1008,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    if (flags & BDRV_O_RDWR) {
-        ret = vhdx_update_headers(bs, s, false, NULL);
-        if (ret < 0) {
-            goto fail;
-        }
-    }
-
     /* TODO: differencing files */
 
     return 0;
-- 
2.13.6

[Qemu-devel] [PATCH 2/4] block/parallels: code movement

[Jeff Cody]

Code movement, to make subsequent patches simpler.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/parallels.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2b6c6e5709..fed199eccd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -159,6 +159,18 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static int parallels_update_header(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    unsigned size = MAX(bdrv_opt_mem_align(bs->file->bs),
+                        sizeof(ParallelsHeader));
+
+    if (size > s->header_size) {
+        size = s->header_size;
+    }
+    return bdrv_pwrite_sync(bs->file, 0, s->header, size);
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -575,18 +587,6 @@ static int parallels_probe(const uint8_t *buf, int buf_size,
     return 0;
 }
 
-static int parallels_update_header(BlockDriverState *bs)
-{
-    BDRVParallelsState *s = bs->opaque;
-    unsigned size = MAX(bdrv_opt_mem_align(bs->file->bs),
-                        sizeof(ParallelsHeader));
-
-    if (size > s->header_size) {
-        size = s->header_size;
-    }
-    return bdrv_pwrite_sync(bs->file, 0, s->header, size);
-}
-
 static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
-- 
2.13.6

[Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Jeff Cody]

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 <jcody@redhat.com>
---
 block/parallels.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index fed199eccd..c560e2fcf2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -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();
@@ -741,7 +754,9 @@ static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
 
-    if (bs->open_flags & BDRV_O_RDWR) {
+    /* Only need to update the header, if we ever actually wrote to the
+     * image at all */
+    if ((bs->open_flags & BDRV_O_RDWR) && !s->first_write_latch) {
         s->header->inuse = 0;
         parallels_update_header(bs);
     }
-- 
2.13.6

Re: [Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Kevin Wolf]

Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:
> 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 <jcody@redhat.com>

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

Re: [Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Jeff Cody]

On Fri, Oct 27, 2017 at 11:09:19AM +0200, Kevin Wolf wrote:
> Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:
> > 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 <jcody@redhat.com>
> 
> 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).

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

> 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

Re: [Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Kevin Wolf]

Am 27.10.2017 um 12:18 hat Jeff Cody geschrieben:
> On Fri, Oct 27, 2017 at 11:09:19AM +0200, Kevin Wolf wrote:
> > Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:
> > > 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 <jcody@redhat.com>
> > 
> > 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).

Technically, you can do the same as I suggest for parallels, the
difference is just that by the letter, the VHDX spec seems to require
that you update the header on the first write (going by the comment in
our VHDX driver...).

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

Bugs? :-)

Kevin

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

Re: [Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Denis V. Lunev]

On 10/27/2017 10:57 AM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block/parallels.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index fed199eccd..c560e2fcf2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -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();
> @@ -741,7 +754,9 @@ static void parallels_close(BlockDriverState *bs)
>  {
>      BDRVParallelsState *s = bs->opaque;
>  
> -    if (bs->open_flags & BDRV_O_RDWR) {
> +    /* Only need to update the header, if we ever actually wrote to the
> +     * image at all */
> +    if ((bs->open_flags & BDRV_O_RDWR) && !s->first_write_latch) {
>          s->header->inuse = 0;
>          parallels_update_header(bs);
>      }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH 4/4] qemu-iotests: update unsupported image formats in 194

[Jeff Cody]

Test 194 checks for 'luks' to exclude as an unsupported format,
However, most formats are unsupported, due to migration blockers.

Rather than specifying a blacklist of unsupported formats, whitelist
supported formats (specifically, qcow2, qed, raw, dmg).

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/194 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 8d973b440f..1d4214aca3 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,7 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(unsupported_fmts=['luks'])
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw', 'dmg'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
-- 
2.13.6