Re: [libvirt] [PATCH] qemu: reduce file padding requirements

Followup to https://bugzilla.redhat.com/show_bug.cgi?id=599091, commit 20206a4b, to reduce disk waste in padding. * src/qemu/qemu_monitor.h (QEMU_MONITOR_MIGRATE_TO_FILE_BS): Drop back to 512. (QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE): New macro. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Update comment. * src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrateToFile): Use two invocations of dd to output non-aligned large blocks. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONMigrateToFile): Likewise. --- This should result in much less wasted space (but I don't have a good environment set up for testing migration at the moment; Laine, would you mind testing this patch, since you wrote the last one?). Also, should we bump QEMU_MONITOR_MIGRATE_TO_FILE_BS to 4k, for the benefit of newer disks that have 4k blocks? src/qemu/qemu_driver.c | 7 ------- src/qemu/qemu_monitor.h | 12 ++++++++---- src/qemu/qemu_monitor_json.c | 13 +++++++++---- src/qemu/qemu_monitor_text.c | 13 +++++++++---- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bc8dcfa..4edf5ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5000,13 +5000,6 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, * we need to ensure there's a 512 byte boundary. Unfortunately * we don't have an explicit offset in the header, so we fake * it by padding the XML string with NULLs. - * - * XXX: This means there will be (QEMU_MONITOR_MIGRATE_TO_FILE_BS - * - strlen(xml)) bytes of wastage in each file. - * Unfortunately, a large BS is needed for reasonable - * performance. It would be nice to find a replacement for dd - * that could specify the start offset in bytes rather than - * blocks, to eliminate this waste. */ if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) { unsigned long long pad = diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index e03b4df..8391ebd 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1,7 +1,7 @@ /* * qemu_monitor.h: interaction with QEMU monitor console * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -261,10 +261,14 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon, unsigned int background, const char * const *argv); -/* In general, a larger BS means better domain save performance, - * at the expense of a larger resulting file - see qemu_driver.c +/* In general, BS is the smallest fundamental block size we can use to + * access a block device; everything must be aligned to a multiple of + * this. However, operating on blocks this small is painfully slow, + * so we also have a transfer size that is larger but only aligned to + * the smaller block size. */ -# define QEMU_MONITOR_MIGRATE_TO_FILE_BS (1024llu * 1024) +# define QEMU_MONITOR_MIGRATE_TO_FILE_BS 512llu +# define QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE (1024llu * 1024) int qemuMonitorMigrateToFile(qemuMonitorPtr mon, unsigned int background, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e89ad43..5e6cedd 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1695,10 +1695,15 @@ int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon, goto cleanup; } - if (virAsprintf(&dest, "exec:%s | dd of=%s bs=%llu seek=%llu", - argstr, safe_target, - QEMU_MONITOR_MIGRATE_TO_FILE_BS, - offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS) < 0) { + /* Two dd processes, sharing the same stdout, are necessary to + * allow starting at an alignment of 512, but without wasting + * padding to get to the larger alignment useful for speed. */ + if (virAsprintf(&dest, "exec:%s | { dd bs=%llu seek=%llu if=/dev/null && " + "dd bs=%llu; } >%s",