block/vdi: Use bdrv_flush after metadata updates

From: phoeagon <[email protected]>

In reference to b0ad5a455d7e5352d4c86ba945112011dbeadfb8~078a458e077d6b0db262c4b05fee51d01de2d1d2, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

---
 block/vdi.c | 3 +++
 1 file changed, 3 insertions(+)

block/vdi.c +3 -0

index 7642ef3..dfe8ade 100644
@@ -713,6 +713,9 @@ static int vdi_co_write(BlockDriverState *bs,
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
         ret = bdrv_write(bs->file, offset, base, n_sectors);
+        if (ret >= 0) {
+            ret = bdrv_flush(bs->file);
+        }
     }
 
     return ret;

Signature · 1 line

2.4.0

From: phoeagon <[email protected]>

In reference to b0ad5a455d7e5352d4c86ba945112011dbeadfb8~078a458e077d6b0db262c4b05fee51d01de2d1d2, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

---
  block/vdi.c | 3 +++
  1 file changed, 3 insertions(+)

block/vdi.c +3 -0

index 7642ef3..dfe8ade 100644
@@ -713,6 +713,9 @@ static int vdi_co_write(BlockDriverState *bs,
          logout("will write %u block map sectors starting from entry %u\n",
                 n_sectors, bmap_first);
          ret = bdrv_write(bs->file, offset, base, n_sectors);
+        if (ret >= 0) {
+            ret = bdrv_flush(bs->file);
+        }
      }
 
      return ret;

Quoted 5 lines

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Quoted 4 lines

Signed-off-by Zhe Qiu <[email protected]>

---
 block/vdi.c | 3 +++
 1 file changed, 3 insertions(+)

I missed Kevin's arguments before, but I think that adding this is more correct than not having it; and when thinking about speed, this is vdi, a format supported for compatibility.

So if we wanted to optimize it, we'd probably have to cache multiple allocations, do them at once and then flush afterwards (like the metadata cache we have in qcow2?)

Am 08.05.2015 um 15:14 hat Max Reitz geschrieben:

Quoted 6 lines · Zhe Qiu

Quoted 5 lines

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Zhe Qiu <[email protected]>

---
 block/vdi.c | 3 +++
 1 file changed, 3 insertions(+)

I missed Kevin's arguments before, but I think that adding this is more correct than not having it; and when thinking about speed, this is vdi, a format supported for compatibility.

If you use it only as a convert target, you probably care more about speed than about leaks in case of a host crash.

Quoted 3 lines

So if we wanted to optimize it, we'd probably have to cache multiple allocations, do them at once and then flush afterwards (like the metadata cache we have in qcow2?)

That would defeat the purpose of this patch which aims at having metadata and data written out almost at the same time. On the other hand, fully avoiding the problem instead of just making the window smaller would require a journal, which VDI just doesn't have.

I'm not convinced of this patch, but I'll defer to Stefan Weil as the VDI maintainer.

Kevin

Am 08.05.2015 um 15:14 hat Max Reitz geschrieben:

Quoted 6 lines · Zhe Qiu

Quoted 5 lines

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Zhe Qiu <[email protected]>

---
  block/vdi.c | 3 +++
  1 file changed, 3 insertions(+)

I missed Kevin's arguments before, but I think that adding this is more correct than not having it; and when thinking about speed, this is vdi, a format supported for compatibility.

If you use it only as a convert target, you probably care more about speed than about leaks in case of a host crash.

Quoted 3 lines

So if we wanted to optimize it, we'd probably have to cache multiple allocations, do them at once and then flush afterwards (like the metadata cache we have in qcow2?)

That would defeat the purpose of this patch which aims at having metadata and data written out almost at the same time. On the other hand, fully avoiding the problem instead of just making the window smaller would require a journal, which VDI just doesn't have.

I'm not convinced of this patch, but I'll defer to Stefan Weil as the VDI maintainer.

Kevin

Am 08.05.2015 um 15:55 schrieb Kevin Wolf:

Quoted 25 lines

Am 08.05.2015 um 15:14 hat Max Reitz geschrieben:

Quoted 6 lines · Zhe Qiu

Quoted 5 lines

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Zhe Qiu <[email protected]>

---
  block/vdi.c | 3 +++
  1 file changed, 3 insertions(+)

I missed Kevin's arguments before, but I think that adding this is more correct than not having it; and when thinking about speed, this is vdi, a format supported for compatibility.

If you use it only as a convert target, you probably care more about speed than about leaks in case of a host crash.

Quoted 3 lines

So if we wanted to optimize it, we'd probably have to cache multiple allocations, do them at once and then flush afterwards (like the metadata cache we have in qcow2?)

That would defeat the purpose of this patch which aims at having metadata and data written out almost at the same time. On the other hand, fully avoiding the problem instead of just making the window smaller would require a journal, which VDI just doesn't have.

Quoted 4 lines

I'm not convinced of this patch, but I'll defer to Stefan Weil as the VDI maintainer.

Kevin

Thanks for asking. I share your concerns regarding reduced performance caused by bdrv_flush. Conversions to VDI will take longer (how much?), and also installation of an OS on a new VDI disk image will be slower because that are the typical scenarios where the disk usage grows.

@phoeagon: Did the benchmark which you used allocate additional disk storage? If not or if it only allocated once and then spent some time on already allocated blocks, that benchmark was not valid for this case.

On the other hand I don't see a need for the flushing because the kind of failures (power failure) and their consequences seem to be acceptable for typical VDI usage, namely either image conversion or tests with existing images.

That's why I'd prefer not to use bdrv_flush here. Could we make bdrv_flush optional (either generally or for cases like this one) so both people who prefer speed and people who would want bdrv_flush to decrease the likelihood of inconsistencies can be satisfied?

Stefan

Thanks. Dbench does not logically allocate new disk space all the time, because it's a FS level benchmark that creates file and deletes them. Therefore it also depends on the guest FS, say, a btrfs guest FS allocates about 1.8x space of that from EXT4, due to its COW nature. It does cause the FS to allocate some space during about 1/3 of the test duration I think. But this does not mitigate it too much because a FS often writes in a stride rather than consecutively, which causes write amplification at allocation times.

So I tested it with qemu-img convert from a 400M raw file:
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t unsafe -O vdi /run/shm/rand 1.vdi

real 0m0.402s
user 0m0.206s
sys 0m0.202s
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

real 0m8.678s
user 0m0.169s
sys 0m0.500s
zheq-PC sdb # time qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

real 0m4.320s
user 0m0.148s
sys 0m0.471s
zheq-PC sdb # time qemu-img convert -f raw -t unsafe -O vdi /run/shm/rand 1.vdi
real 0m0.489s
user 0m0.173s
sys 0m0.325s

zheq-PC sdb # time qemu-img convert -f raw -O vdi /run/shm/rand 1.vdi

real 0m0.515s
user 0m0.168s
sys 0m0.357s
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -O vdi /run/shm/rand 1.vdi

real 0m0.431s
user 0m0.192s
sys 0m0.248s

Although 400M is not a giant file, it does show the trend.
As you can see when there's drastic allocation needs, and when there no extra buffering from a virtualized host, the throughput drops about 50%. But still it has no effect on "unsafe" mode, as predicted. Also I believe that expecting to use a half-converted image is seldom the use case, while host crash and power loss are not so unimaginable.
Looks like qemu-img convert is using "unsafe" as default as well, so even novice "qemu-img convert" users are not likely to find performance degradation.

I have not yet tried guest OS installation on top, but I guess a new flag for one-time faster OS installation is not likely useful, and "cache=unsafe" already does the trick.

Quoted 54 lines · Stefan Weil

Am 08.05.2015 um 15:55 schrieb Kevin Wolf:

Quoted 25 lines

Am 08.05.2015 um 15:14 hat Max Reitz geschrieben:

Quoted 6 lines · Zhe Qiu

Quoted 5 lines

In reference to b0ad5a45...078a458e, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Zhe Qiu <[email protected]>

---
  block/vdi.c | 3 +++
  1 file changed, 3 insertions(+)

I missed Kevin's arguments before, but I think that adding this is more correct than not having it; and when thinking about speed, this is vdi, a format supported for compatibility.

If you use it only as a convert target, you probably care more about speed than about leaks in case of a host crash.

Quoted 3 lines

So if we wanted to optimize it, we'd probably have to cache multiple allocations, do them at once and then flush afterwards (like the metadata cache we have in qcow2?)

That would defeat the purpose of this patch which aims at having metadata and data written out almost at the same time. On the other hand, fully avoiding the problem instead of just making the window smaller would require a journal, which VDI just doesn't have.

Quoted 4 lines

I'm not convinced of this patch, but I'll defer to Stefan Weil as the VDI maintainer.

Kevin

Thanks for asking. I share your concerns regarding reduced performance caused by bdrv_flush. Conversions to VDI will take longer (how much?), and also installation of an OS on a new VDI disk image will be slower because that are the typical scenarios where the disk usage grows.

@phoeagon: Did the benchmark which you used allocate additional disk storage? If not or if it only allocated once and then spent some time on already allocated blocks, that benchmark was not valid for this case.

On the other hand I don't see a need for the flushing because the kind of failures (power failure) and their consequences seem to be acceptable for typical VDI usage, namely either image conversion or tests with existing images.

That's why I'd prefer not to use bdrv_flush here. Could we make bdrv_flush optional (either generally or for cases like this one) so both people who prefer speed and people who would want bdrv_flush to decrease the likelihood of inconsistencies can be satisfied?

Stefan

BTW, how do you usually measure the time to install a Linux distro within? Most distros ISOs do NOT have unattended installation ISOs in place. (True I can bake my own ISOs for this...) But do you have any ISOs made ready for this purpose?

On Sat, May 9, 2015 at 11:54 AM phoeagon <[email protected] <mailto:[email protected]>> wrote:

Thanks. Dbench does not logically allocate new disk space all the time, because it's a FS level benchmark that creates file and deletes them. Therefore it also depends on the guest FS, say, a btrfs guest FS allocates about 1.8x space of that from EXT4, due to its COW nature. It does cause the FS to allocate some space during about 1/3 of the test duration I think. But this does not mitigate it too much because a FS often writes in a stride rather than consecutively, which causes write amplification at allocation times.

So I tested it with qemu-img convert from a 400M raw file:
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t unsafe -O vdi /run/shm/rand 1.vdi

real0m0.402s
user0m0.206s
sys0m0.202s
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

Am 09.05.2015 um 05:59 schrieb phoeagon:

BTW, how do you usually measure the time to install a Linux distro within? Most distros ISOs do NOT have unattended installation ISOs in place. (True I can bake my own ISOs for this...) But do you have any ISOs made ready for this purpose?

Quoted 13 lines · phoeagon

Thanks. Dbench does not logically allocate new disk space all the time, because it's a FS level benchmark that creates file and deletes them. Therefore it also depends on the guest FS, say, a btrfs guest FS allocates about 1.8x space of that from EXT4, due to its COW nature. It does cause the FS to allocate some space during about 1/3 of the test duration I think. But this does not mitigate it too much because a FS often writes in a stride rather than consecutively, which causes write amplification at allocation times.

So I tested it with qemu-img convert from a 400M raw file:
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t unsafe -O vdi /run/shm/rand 1.vdi

Quoted 6 lines

real 0m0.402s
user 0m0.206s
sys 0m0.202s
zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

I assume that the target file /run/shm/rand 1.vdi is not on a physical disk.
Then flushing data will be fast. For real hard disks (not SSDs) the situation is
different: the r/w heads of the hard disk have to move between data location
and the beginning of the written file where the metadata is written, so I expect a larger effect there.

For measuring installation time of an OS, I'd take a reproducible installation
source (hard disk or DVD, no network connection) and take the time for those parts of the installation where many packets are installed without any user interaction. For Linux you won't need a stop watch, because the packet directories in /usr/share/doc have nice timestamps.

Stefan

zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

real0m8.678s
user0m0.169s
sys0m0.500s

zheq-PC sdb # time qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi real0m4.320s
user0m0.148s
sys0m0.471s

zheq-PC sdb # time qemu-img convert -f raw -t unsafe -O vdi /run/shm/rand 1.vdi
real 0m0.489s
user 0m0.173s
sys 0m0.325s

Quoted 10 lines

zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi

real0m8.678s
user0m0.169s
sys0m0.500s

zheq-PC sdb # time qemu-img convert -f raw -t writeback -O vdi /run/shm/rand 1.vdi real0m4.320s
user0m0.148s
sys0m0.471s

This means that 3.83 seconds are spent when bdrv_close() calls bdrv_flush(). That's the only difference between writeback and unsafe in qemu-img convert.

The remaining part of the time (4.85 seconds instead of 0.49 seconds) means that, at least on your hardware, sequential writes to unallocated space become 10 times slower with your patch.

Since the default qemu-img convert case isn't slowed down, I would think that correctness trumps performance. Nevertheless, it's a huge difference.

Paolo

Am 10.05.2015 um 17:01 schrieb Paolo Bonzini:

Quoted 3 lines · phoeagon

Quoted 1 line

zheq-PC sdb # time ~/qemu-sync-test/bin/qemu-img convert -f raw -t

writeback -O vdi /run/shm/rand 1.vdi

Quoted 6 lines

Quoted 6 lines

real0m8.678s
user0m0.169s
sys0m0.500s

zheq-PC sdb # time qemu-img convert -f raw -t writeback -O vdi

/run/shm/rand 1.vdi

Quoted 11 lines

Quoted 3 lines

real0m4.320s
user0m0.148s
sys0m0.471s

This means that 3.83 seconds are spent when bdrv_close() calls bdrv_flush(). That's the only difference between writeback and unsafe in qemu-img convert.

The remaining part of the time (4.85 seconds instead of 0.49 seconds) means that, at least on your hardware, sequential writes to unallocated space become 10 times slower with your patch.

Quoted 5 lines

Since the default qemu-img convert case isn't slowed down, I would think that correctness trumps performance. Nevertheless, it's a huge difference.

Paolo

I doubt that the convert case isn't slowed down.

Writing to a tmpfs as it was obviously done for the test is not a typical use case.
With real hard disks I expect a significant slowdown.

Stefan

Quoted 3 lines

Since the default qemu-img convert case isn't slowed down, I would think that correctness trumps performance. Nevertheless, it's a huge difference.

I doubt that the convert case isn't slowed down.

Writing to a tmpfs as it was obviously done for the test is not a typical use case.
With real hard disks I expect a significant slowdown.

Stefan

Quoted 4 lines

Quoted 3 lines

Since the default qemu-img convert case isn't slowed down, I would think that correctness trumps performance. Nevertheless, it's a huge difference.

I doubt that the convert case isn't slowed down.

The *default* convert case isn't slowed down because "qemu-img convert" defaults to the "unsafe" cache mode.

The *non-default* convert case with flushes was slowed down indeed: 2x in total (if you include the final flush done by bdrv_close), and 10x if you only consider the sequential write part of convert.

Paolo

Am 10.05.2015 um 18:10 schrieb Paolo Bonzini:

Quoted 5 lines · Stefan Weil

Quoted 4 lines

Quoted 3 lines

Since the default qemu-img convert case isn't slowed down, I would think that correctness trumps performance. Nevertheless, it's a huge difference.

I doubt that the convert case isn't slowed down.

Quoted 8 lines

The *default* convert case isn't slowed down because "qemu-img convert" defaults to the "unsafe" cache mode.

The *non-default* convert case with flushes was slowed down indeed: 2x in total (if you include the final flush done by bdrv_close), and 10x if you only consider the sequential write part of convert.

Paolo

For those who might be interested:

The relevant GPL source code from VirtualBox is available here:

https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Storage

If I interpret that code correctly, blocks are normally written asynchronously,
but changes of metadata (new block allocation) are written synchronously.

See file VDI.cpp (function vdiBlockAllocUpdate) and VD.cpp (vdIOIntWriteMeta).

Stefan

From: phoeagon <[email protected]>

In reference to b0ad5a455d7e5352d4c86ba945112011dbeadfb8~078a458e077d6b0db262c4b05fee51d01de2d1d2, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

Am 07.05.2015 um 06:04 hat Zhe Qiu geschrieben:

Quoted 3 lines

From: phoeagon <[email protected]>

In reference to

b0ad5a455d7e5352d4c86ba945112011dbeadfb8~078a458e077d6b0db262c4b05fee51d01de2d1d2, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

The justification for these patches (in 2010!) was more or less that we didn't know whether the implementations were safe without the flushes. Many of the flushes added back then have been removed again until today because they have turned out to be unnecessary.

Quoted 3 lines

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

Please describe why VDI needs this flush to be safe. This is best done by describing a case where not doing the flush would lead to image corruption in case of a crash.

To my knowledge, the VDI driver is completely safe without it.

Kevin

In case of correctness, lacking a sync here does not introduce data corruption I can think of. But this reduces the volatile window during which the metadata changes are NOT guaranteed on disk. Without a barrier, in case of power loss you may end up with the bitmap changes on disk and not the header block, or vice versa. Neither introduces data corruption directly, but since VDI doesn't have proper fix mechanism for qemu-img, once the leak is introduced you have to "convert" to fix it, consuming a long time if the disk is large.

This patch does not fix the issue entirely, and it does not substitute for proper check-and-fix implementation. But this should bring about minor performance degradation (only 1 extra sync per allocation) but greatly reduces the metadata inconsistency window.

Am 08.05.2015 um 13:50 hat phoeagon geschrieben:

Quoted 1 line

In case of correctness, lacking a sync here does not introduce data

corruption

Quoted 1 line

I can think of. But this reduces the volatile window during which the

metadata

Quoted 1 line

changes are NOT guaranteed on disk. Without a barrier, in case of power

loss

Quoted 1 line

you may end up with the bitmap changes on disk and not the header block,

or

Quoted 1 line

vice versa. Neither introduces data corruption directly, but since VDI

doesn't

Quoted 1 line

have proper fix mechanism for qemu-img, once the leak is introduced you

have to

Quoted 1 line

"convert" to fix it, consuming a long time if the disk is large.

This is true. I'm not sure how big a problem this is in practice, though.

Quoted 1 line

This patch does not fix the issue entirely, and it does not substitute

for

Quoted 2 lines

proper check-and-fix implementation. But this should bring about minor performance degradation (only 1 extra sync per allocation) but greatly

reduces

Quoted 1 line

the metadata inconsistency window.

Did you benchmark this? From the past experience with flushes in qemu block drivers, one sync per allocation certainly doesn't sound "minor".

What could possibly save us from the worst is that VDI has a relatively large block size (or rather, that we don't support images with different block sizes).

Kevin

From: phoeagon <[email protected]>

In reference to b0ad5a455d7e5352d4c86ba945112011dbeadfb8~078a458e077d6b0db262c4b05fee51d01de2d1d2, metadata writes to qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes.

Only when write is successful that bdrv_flush is called.

Signed-off-by Zhe Qiu <[email protected]>

---
  block/vdi.c | 3 +++
  1 file changed, 3 insertions(+)

block/vdi.c +3 -0

index 7642ef3..dfe8ade 100644
@@ -713,6 +713,9 @@ static int vdi_co_write(BlockDriverState *bs,
          logout("will write %u block map sectors starting from entry %u\n",
                 n_sectors, bmap_first);
          ret = bdrv_write(bs->file, offset, base, n_sectors);
+        if (ret >= 0) {
+            ret = bdrv_flush(bs->file);
+        }
      }
 
      return ret;