12:35 p.m.
Right now, we create qemu snapshots to a file by using an exec: monitor
command that passes 'compressor | { dd && dd; }' with stdout connected
to the target file. However, since dd is using a larger bs= than
PIPE_MAX, it is conceivable that under heavy machine load, that dd will
get a short read from the pipe, and unless we use the GNU extension of
iflag=fullblock, that short read will be padded out to the output block
size and result in data corruption in the destination file.
The possibility of corruption due to short reads when dd is fed input
through a pipe but produces output through a large block size has
occurred several times on the coreutils mailing list:
http://lists.gnu.org/archive/html/bug-coreutils/2010-11/msg00092.html
Coreutils currently obeys the (non-intuitive) POSIX requirements for
this short read behavior, and while it is being considered to make dd
when POSIXLY_CORRECT is unset be more sensible, you can't rely on that
being a default.
Should we refuse to make snapshots if we don't detect the GNU extension
of 'dd iflag=fullblock'? Probably safe to do on GNU/Linux machines, but
I'm wondering if it will have negative effects on BSD machines where GNU
coreutils is not installed.
Is there a way to make monitor commands use fd: style migration, where
we can avoid dd altogether by just passing an already open fd that has
already positioned correctly via lseek()? Then again, it seems like fd:
migration requires passing an open fd, which only seems possible on the
command line at startup rather than something you can do on the fly with
monitor commands.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:06 p.m.
On 23/12/2010, at 5:35 AM, Eric Blake wrote:
<snip>
Should we refuse to make snapshots if we don't detect the GNU
extension
of 'dd iflag=fullblock'? Probably safe to do on GNU/Linux machines, but
I'm wondering if it will have negative effects on BSD machines where GNU
coreutils is not installed.
As a thought, that option isn't known to the dd supplied with OSX 10.6.5:
$ dd if=/dev/zero of=foo bs=1024 count=1
1+0 records in
1+0 records out
1024 bytes transferred in 0.000041 secs (24970740 bytes/sec)
$ rm foo
$ dd if=/dev/zero of=foo bs=1024 count=1 iflag=fullblock
dd: unknown operand iflag
$
3:02 p.m.
On 12/22/2010 11:35 AM, Eric Blake wrote:
Right now, we create qemu snapshots to a file by using an exec:
monitor
command that passes 'compressor | { dd && dd; }' with stdout connected
to the target file. However, since dd is using a larger bs= than
PIPE_MAX, it is conceivable that under heavy machine load, that dd will
get a short read from the pipe, and unless we use the GNU extension of
iflag=fullblock, that short read will be padded out to the output block
size and result in data corruption in the destination file.
The possibility of corruption due to short reads when dd is fed input
through a pipe but produces output through a large block size has
occurred several times on the coreutils mailing list:
http://lists.gnu.org/archive/html/bug-coreutils/2010-11/msg00092.html
Coreutils currently obeys the (non-intuitive) POSIX requirements for
this short read behavior, and while it is being considered to make dd
when POSIXLY_CORRECT is unset be more sensible, you can't rely on that
being a default.
Should we refuse to make snapshots if we don't detect the GNU extension
of 'dd iflag=fullblock'? Probably safe to do on GNU/Linux machines, but
I'm wondering if it will have negative effects on BSD machines where GNU
coreutils is not installed.
I've gone back and read POSIX again. It states:
If the bs=expr operand is specified and no conversions other than sync,
noerror, or notrunc are requested, the data returned from each input
block shall be written as a separate output block; if the read returns
less than a full block and the sync conversion is not specified, the
resulting output block shall be the same size as the input block. If the
bs=expr operand is not specified, or a conversion other than sync,
noerror, or notrunc is requested, the input shall be processed and
collected into full-sized output blocks until the end of the input is
reached.
So I stand corrected - because we aren't using conv=sync, there is no
zero padding or data corruption; however, behavior can still be
improved. With bs=, a short read results in a short write, so we aren't
using the full 1M block size that we asked of dd, and end up with more
syscalls than necessary. Swapping to ibs= obs= instead of bs= allows dd
to do fewer syscalls on output. Patch coming up soon, but I'm grateful
that this is not a data corruption bug after all. And the GNU extension
of iflag=fullblock is only useful when mixing bs= and conv=sync, so
there's no need to use that extension.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:06 p.m.
New subject: [libvirt] [PATCH] qemu: improve efficiency of dd during snapshots
POSIX states about dd:
If the bs=expr operand is specified and no conversions other than
sync, noerror, or notrunc are requested, the data returned from each
input block shall be written as a separate output block; if the read
returns less than a full block and the sync conversion is not
specified, the resulting output block shall be the same size as the
input block. If the bs=expr operand is not specified, or a conversion
other than sync, noerror, or notrunc is requested, the input shall be
processed and collected into full-sized output blocks until the end of
the input is reached.
Since we aren't using conv=trunc, there is no zero-padding, but our
use of bs= means that a short read results in a short write. If
instead we use ibs= and obs=, then short reads are collected and dd
only has to do a single write, which can make dd more efficient.
* src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrateToFile):
Avoid 'dd bs=', since it can cause short writes.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONMigrateToFile):
Likewise.
---
src/qemu/qemu_monitor_json.c | 3 ++-
src/qemu/qemu_monitor_text.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7877731..284f9c4 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1695,10 +1695,11 @@ int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
* <> redirection to avoid truncating a regular file. */
if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s |
"
"{ dd bs=%llu seek=%llu if=/dev/null && "
- "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX,
+ "dd ibs=%llu; obs=%llu } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
argstr, QEMU_MONITOR_MIGRATE_TO_FILE_BS,
offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS,
QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE,
+ QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE,
safe_target) < 0) {
virReportOOMError();
goto cleanup;
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 11a9391..9c139ed 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1280,10 +1280,11 @@ int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon,
* <> redirection to avoid truncating a regular file. */
if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s |
"
"{ dd bs=%llu seek=%llu if=/dev/null && "
- "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX,
+ "dd ibs=%llu obs=%llu; } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
argstr, QEMU_MONITOR_MIGRATE_TO_FILE_BS,
offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS,
QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE,
+ QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE,
safe_target) < 0) {
virReportOOMError();
goto cleanup;
--
1.7.3.3
2:48 a.m.
New subject: [libvirt] [PATCH] qemu: improve efficiency of dd during snapshots
On Wed, Dec 22, 2010 at 22:06, Eric Blake <eblake(a)redhat.com> wrote:
diff --git a/src/qemu/qemu_monitor_json.c
b/src/qemu/qemu_monitor_json.c
index 7877731..284f9c4 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1695,10 +1695,11 @@ int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
* <> redirection to avoid truncating a regular file. */
if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s |
"
"{ dd bs=%llu seek=%llu if=/dev/null && "
- "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX,
+ "dd ibs=%llu; obs=%llu } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
typo: semicolon before obs. Should be:
"dd ibs=%llu obs=%llu } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
--
Pawel
9:47 a.m.
New subject: [libvirt] [PATCH] qemu: improve efficiency of dd during snapshots
On 12/30/2010 01:48 AM, Paweł Krześniak wrote:
On Wed, Dec 22, 2010 at 22:06, Eric Blake <eblake(a)redhat.com>
wrote:
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7877731..284f9c4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1695,10 +1695,11 @@ int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
> * <> redirection to avoid truncating a regular file. */
> if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s |
"
> "{ dd bs=%llu seek=%llu if=/dev/null && "
> - "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX,
> + "dd ibs=%llu; obs=%llu } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
typo: semicolon before obs. Should be:
"dd ibs=%llu obs=%llu } 1<>%s"
VIR_WRAPPER_SHELL_SUFFIX,
Thanks for the review. But your correction is also wrong. It should be:
"dd ibs=%llu obs=%llu; } 1<>%s"
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5117
days inactive
5125
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Justin Clift -
Paweł Krześniak