Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-10 Thread Peter Krempa 
On Mon, Dec 09, 2019 at 11:52:33 -0600, Eric Blake wrote:
> On 12/3/19 11:17 AM, Peter Krempa wrote:
> > This allows to start and manage the backup job.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> 
> resuming where I left off last time

[...]

> > +if (!reuse_external) {
> > +if (qemuBlockStorageSourceCreateDetectSize(blockNamedNodeData,
> > +   dd->store, 
> > dd->domdisk->src) < 0)
> > +return -1;
> > +
> > +if (qemuBlockStorageSourceCreate(vm, dd->store, NULL, NULL,
> > + dd->crdata->srcdata[0],
> > + QEMU_ASYNC_JOB_BACKUP) < 0)
> > +return -1;
> > +} else {
> > +if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, 
> > QEMU_ASYNC_JOB_BACKUP) < 0)
> > +return -1;
> > +
> > +rc = qemuBlockStorageSourceAttachApply(priv->mon, 
> > dd->crdata->srcdata[0]);
> > +
> > +if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || rc < 0)
> > +return -1;
> 
> Offlist, we were wondering if this should blindly trust whatever is already
> in the external file, or blindly pass backing:null.  It may depend on
> whether it is push mode (probably trust the image) vs. pull mode (we will be
> hooking up the backing file ourselves when we create the sync:none job, so
> if the scratch disk already has a backing file that gets in the way, which
> argues we want a blind backing:null in that case).

I'll add the following diff:

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index caef12cf94..1ca9f8dfe0 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -283,6 +283,7 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,
  bool removeStore)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+g_autoptr(virStorageSource) terminator = NULL;

 /* set data structure */
 dd->backupdisk = backupdisk;
@@ -311,13 +312,17 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,
 return -1;
 }

+/* install terminator to prevent qemu form opening backing images */
+if (!(terminator = virStorageSourceNew()))
+return -1;
+
 if (!(dd->blockjob = qemuBlockJobDiskNewBackup(vm, dd->domdisk, dd->store,
removeStore,
dd->incrementalBitmap)))
 return -1;

 if (!(dd->crdata = 
qemuBuildStorageSourceChainAttachPrepareBlockdevTop(dd->store,
-   
NULL,
+   
terminator,

priv->qemuCaps)))
 return -1;


> > +/**
> > + * qemuBackupBeginCollectIncrementalCheckpoints:
> > + * @vm: domain object
> > + * @incrFrom: name of checkpoint representing starting point of 
> > incremental backup
> > + *
> > + * Returns a NULL terminated list of pointers to checkpoints in 
> > chronological
> > + * order starting from the 'current' checkpoint until reaching @incrFrom.
> > + */
> > +static virDomainMomentObjPtr *
> > +qemuBackupBeginCollectIncrementalCheckpoints(virDomainObjPtr vm,
> > + const char *incrFrom)
> > +{
> > +virDomainMomentObjPtr n = 
> > virDomainCheckpointGetCurrent(vm->checkpoints);
> > +g_autofree virDomainMomentObjPtr *incr = NULL;
> > +size_t nincr = 0;
> > +
> > +while (n) {
> > +if (VIR_APPEND_ELEMENT_COPY(incr, nincr, n) < 0)
> 
> Are there better glib functions for doing this string management?

Probably yes. We didn't adopt them yet though and I don't feel brave
enough to do it as a part of this series.

> 
> > +return NULL;
> > +
> > +if (STREQ(n->def->name, incrFrom)) {
> > +virDomainMomentObjPtr terminator = NULL;
> > +if (VIR_APPEND_ELEMENT_COPY(incr, nincr, terminator) < 0)

[...]

> > +
> > +if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, 
> > QEMU_ASYNC_JOB_BACKUP) < 0)
> > +goto endjob;
> > +blockNamedNodeData = qemuMonitorBlockGetNamedNodeData(priv->mon);
> > +if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || 
> > !blockNamedNodeData)
> > +goto endjob;
> 
> Do you still need to ask the monitor for named node data, or should you
> already have access to all that information since backups require -blockdev
> support?

Yes. This is for detecting the state of bitmaps and the sizing of the
images. When we are pre-creating the images we need to know the current
size.


> > +
> > +if (qemuBackupDiskPrepareStorage(vm, dd, ndd, blockNamedNodeData, 
> > reuse) < 0)
> > +goto endjob;
> > +
> > +priv->backup = g_steal_pointer();

[...]

> > +if (chk &&
> > +qemuCheckpointCreateFinalize(priv->driver, vm, cfg, chk,

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-09 Thread Eric Blake 

On 12/3/19 11:17 AM, Peter Krempa wrote:

This allows to start and manage the backup job.

Signed-off-by: Peter Krempa 
---


resuming where I left off last time


+
+static int
+qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,



+
+static int
+qemuBackupDiskPrepareDataOnePush(virJSONValuePtr actions,
+ struct qemuBackupDiskData *dd)
+{
+qemuMonitorTransactionBackupSyncMode syncmode = 
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_FULL;
+
+if (dd->incrementalBitmap)
+syncmode = QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_INCREMENTAL;


Looks correct for both forms of push mode backup.



+
+static int
+qemuBackupDiskPrepareDataOnePull(virJSONValuePtr actions,
+ struct qemuBackupDiskData *dd)
+{
+if (qemuMonitorTransactionBackup(actions,
+ dd->domdisk->src->nodeformat,
+ dd->blockjob->name,
+ dd->store->nodeformat,
+ NULL,
+ 
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE) < 0)


and this is the correct mode for the blockjob used in managing push mode 
backup.



+
+static int
+qemuBackupDiskPrepareOneStorage(virDomainObjPtr vm,
+virHashTablePtr blockNamedNodeData,
+struct qemuBackupDiskData *dd,
+bool reuse_external)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int rc;
+
+if (!reuse_external &&
+dd->store->type == VIR_STORAGE_TYPE_FILE &&
+virStorageFileSupportsCreate(dd->store)) {
+
+if (virFileExists(dd->store->path)) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("store '%s' for backup of '%s' existst"),


exists


+   dd->store->path, dd->domdisk->dst);
+return -1;
+}
+
+if (qemuDomainStorageFileInit(priv->driver, vm, dd->store, NULL) < 0)
+return -1;
+
+dd->initialized = true;
+
+if (virStorageFileCreate(dd->store) < 0) {
+virReportSystemError(errno,
+ _("failed to create image file '%s'"),
+ NULLSTR(dd->store->path));
+return -1;
+}
+
+dd->created = true;
+}
+
+if (qemuDomainStorageSourceAccessAllow(priv->driver, vm, dd->store, false,
+   true) < 0)
+return -1;
+
+dd->labelled = true;
+
+if (!reuse_external) {
+if (qemuBlockStorageSourceCreateDetectSize(blockNamedNodeData,
+   dd->store, dd->domdisk->src) 
< 0)
+return -1;
+
+if (qemuBlockStorageSourceCreate(vm, dd->store, NULL, NULL,
+ dd->crdata->srcdata[0],
+ QEMU_ASYNC_JOB_BACKUP) < 0)
+return -1;
+} else {
+if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, 
QEMU_ASYNC_JOB_BACKUP) < 0)
+return -1;
+
+rc = qemuBlockStorageSourceAttachApply(priv->mon, 
dd->crdata->srcdata[0]);
+
+if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || rc < 0)
+return -1;


Offlist, we were wondering if this should blindly trust whatever is 
already in the external file, or blindly pass backing:null.  It may 
depend on whether it is push mode (probably trust the image) vs. pull 
mode (we will be hooking up the backing file ourselves when we create 
the sync:none job, so if the scratch disk already has a backing file 
that gets in the way, which argues we want a blind backing:null in that 
case).




+/**
+ * qemuBackupBeginPullExportDisks:
+ * @vm: domain object
+ * @disks: backup disk data list
+ * @ndisks: number of valid disks in @disks
+ *
+ * Exports all disks from @dd when doing a pull backup in the NBD server. This
+ * function must be called while in the monitor context.
+ */
+static int
+qemuBackupBeginPullExportDisks(virDomainObjPtr vm,
+   struct qemuBackupDiskData *disks,
+   size_t ndisks)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+size_t i;
+
+for (i = 0; i < ndisks; i++) {
+struct qemuBackupDiskData *dd = disks + i;
+
+if (qemuMonitorNBDServerAdd(priv->mon,
+dd->store->nodeformat,
+dd->domdisk->dst,
+false,
+dd->incrementalBitmap) < 0)
+return -1;
+}


When there's more than one disk, this process is non-atomic (a lucky 
observer on the NBD port could see one but not all of the disks exported 
yet).  Or even with just one disk, a lucky observer can see the NBD port 
created but no disk exported yet.  At any rate,

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-09 Thread Ján Tomko 

On Tue, Dec 03, 2019 at 06:17:42PM +0100, Peter Krempa wrote:

This allows to start and manage the backup job.

Signed-off-by: Peter Krempa 
---
po/POTFILES.in   |   1 +
src/qemu/Makefile.inc.am |   2 +
src/qemu/qemu_backup.c   | 941 +++
src/qemu/qemu_backup.h   |  41 ++
src/qemu/qemu_driver.c   |  47 ++
5 files changed, 1032 insertions(+)
create mode 100644 src/qemu/qemu_backup.c
create mode 100644 src/qemu/qemu_backup.h



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-09 Thread Peter Krempa 
On Tue, Dec 03, 2019 at 18:17:42 +0100, Peter Krempa wrote:
> This allows to start and manage the backup job.
> 
> Signed-off-by: Peter Krempa 
> ---
>  po/POTFILES.in   |   1 +
>  src/qemu/Makefile.inc.am |   2 +
>  src/qemu/qemu_backup.c   | 941 +++
>  src/qemu/qemu_backup.h   |  41 ++
>  src/qemu/qemu_driver.c   |  47 ++
>  5 files changed, 1032 insertions(+)
>  create mode 100644 src/qemu/qemu_backup.c
>  create mode 100644 src/qemu/qemu_backup.h

Following diff is required when rebasing this on top of current master
due to changes in passing of virCaps:

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 3780eb1ea4..5954f90d5c 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -638,7 +638,6 @@ qemuBackupBegin(virDomainObjPtr vm,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
 g_autoptr(virDomainBackupDef) def = NULL;
-g_autoptr(virCaps) caps = NULL;
 g_autofree char *suffix = NULL;
 struct timeval tv;
 bool pull = false;
@@ -663,14 +662,11 @@ qemuBackupBegin(virDomainObjPtr vm,
 return -1;
 }

-if (!(caps = virQEMUDriverGetCapabilities(priv->driver, false)))
-return -1;
-
 if (!(def = virDomainBackupDefParseString(backupXML, priv->driver->xmlopt, 
0)))
 return -1;

 if (checkpointXML) {
-if (!(chkdef = virDomainCheckpointDefParseString(checkpointXML, caps,
+if (!(chkdef = virDomainCheckpointDefParseString(checkpointXML,
  priv->driver->xmlopt,
  priv->qemuCaps, 0)))
 return -1;
@@ -724,7 +720,7 @@ qemuBackupBegin(virDomainObjPtr vm,
 goto endjob;

 if (chkdef) {
-if (qemuCheckpointCreateCommon(priv->driver, vm, caps, ,
+if (qemuCheckpointCreateCommon(priv->driver, vm, ,
, ) < 0)
 goto endjob;
 }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-08 Thread Cole Robinson 
On 12/3/19 12:17 PM, Peter Krempa wrote:
> This allows to start and manage the backup job.
> 
> Signed-off-by: Peter Krempa 
> ---
>  po/POTFILES.in   |   1 +
>  src/qemu/Makefile.inc.am |   2 +
>  src/qemu/qemu_backup.c   | 941 +++
>  src/qemu/qemu_backup.h   |  41 ++
>  src/qemu/qemu_driver.c   |  47 ++
>  5 files changed, 1032 insertions(+)
>  create mode 100644 src/qemu/qemu_backup.c
>  create mode 100644 src/qemu/qemu_backup.h
...
> +
> +static int
> +qemuBackupDiskPrepareOneStorage(virDomainObjPtr vm,
> +virHashTablePtr blockNamedNodeData,
> +struct qemuBackupDiskData *dd,
> +bool reuse_external)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +int rc;
> +
> +if (!reuse_external &&
> +dd->store->type == VIR_STORAGE_TYPE_FILE &&
> +virStorageFileSupportsCreate(dd->store)) {
> +
> +if (virFileExists(dd->store->path)) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("store '%s' for backup of '%s' existst"),

Noticed this in testing, s/existst/exists/

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-06 Thread Vladimir Sementsov-Ogievskiy 
05.12.2019 21:13, Eric Blake wrote:
> [adding some qemu visibility]
> 
> On 12/5/19 7:34 AM, Peter Krempa wrote:
> 
 +    if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))
 +    return -1;
 +
 +    if (qemuMonitorTransactionBitmapAdd(actions,
 +    dd->domdisk->src->nodeformat,
 +    dd->incrementalBitmap,
 +    false,
 +    true) < 0)
 +    return -1;
 +
 +    if (qemuMonitorTransactionBitmapMerge(actions,
 +  dd->domdisk->src->nodeformat,
 +  dd->incrementalBitmap,
 +  ) < 0)
 +    return -1;
 +
 +    if (qemuMonitorTransactionBitmapAdd(actions,
 +    dd->store->nodeformat,
 +    dd->incrementalBitmap,
 +    false,
 +    true) < 0)
 +    return -1;
>>>
>>> Why do we need two of these calls?
>>> /me reads on
>>>
 +
 +    if (qemuMonitorTransactionBitmapMerge(actions,
 +  dd->store->nodeformat,
 +  dd->incrementalBitmap,
 +  ) < 0)
 +    return -1;
>>>
>>> okay, it looks like you are trying to merge the same bitmap into two
>>> different merge commands, which will all be part of the same transaction.  I
>>> guess it would be helpful to see a trace of the transaction in action to see
>>> if we can simplify it (using 2 instead of 4 qemuMonitor* commands).
>>
>> This is required because the backup blockjob requires the bitmap to be
>> present on the source ('device') image of the backup. The same also
>> applies for the image exported by NBD. The catch is that we expose the
>> scratch file via NBD which is actually the target image of the backup.
>> This means that in case of a pull backup we need two instances of the
>> bitmap so both the block job and the NBD server can use them. Arguably
>> there's a possible simplification here for push-mode backups where the
>> target bitmap doesn't make sense.
> 
> The backup job requires the bitmap to be on the source, but the qemu NBD 
> export code only requires the bitmap to be locatable somewhere on the qemu 
> NBD server requires the bitmap to be discoverable from anywhere on the chain, 
> and since the temporary target of the block job has the source image as its 
> backing file, that should be the case.  That is:
> 
> base <- active <- temp
>    |
>      bitmap0
> 
> looking up [active, bitmap0] or [temp, bitmap0] should both succeed; we need 
> the former for the blockjob, and the latter for the NBD export.
> 
> If the NBD export _can't_ find bitmap0 through the backing chain, that may be 
> a symptom of the problem that Max has been trying to fix (his upcoming v7 
> "deal with filters" is hinted at here, but will not be in 4.2):
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04520.html

these problems will hit if some filters are in use, like throttling, 
copy-on-read, etc. They use file child, which breaks backing chains. But normal 
backing chains should work well.

> 
> In my original implementation, I did not need a duplicated bitmap in the temp 
> file.  But that was pre-blockdev.  Are we really hitting filter limitations 
> in qemu where the use of blockdev is preventing [temp, bitmap0] from finding 
> the bitmap across the backing chain?  If so, we should fix that in qemu - but 
> we're so late for 4.2, that I guess libvirt will have to work around it.
> 



-- 
Best regards,
Vladimir

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-05 Thread Eric Blake 

[adding some qemu visibility]

On 12/5/19 7:34 AM, Peter Krempa wrote:


+if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))
+return -1;
+
+if (qemuMonitorTransactionBitmapAdd(actions,
+dd->domdisk->src->nodeformat,
+dd->incrementalBitmap,
+false,
+true) < 0)
+return -1;
+
+if (qemuMonitorTransactionBitmapMerge(actions,
+  dd->domdisk->src->nodeformat,
+  dd->incrementalBitmap,
+  ) < 0)
+return -1;
+
+if (qemuMonitorTransactionBitmapAdd(actions,
+dd->store->nodeformat,
+dd->incrementalBitmap,
+false,
+true) < 0)
+return -1;


Why do we need two of these calls?
/me reads on


+
+if (qemuMonitorTransactionBitmapMerge(actions,
+  dd->store->nodeformat,
+  dd->incrementalBitmap,
+  ) < 0)
+return -1;


okay, it looks like you are trying to merge the same bitmap into two
different merge commands, which will all be part of the same transaction.  I
guess it would be helpful to see a trace of the transaction in action to see
if we can simplify it (using 2 instead of 4 qemuMonitor* commands).


This is required because the backup blockjob requires the bitmap to be
present on the source ('device') image of the backup. The same also
applies for the image exported by NBD. The catch is that we expose the
scratch file via NBD which is actually the target image of the backup.
This means that in case of a pull backup we need two instances of the
bitmap so both the block job and the NBD server can use them. Arguably
there's a possible simplification here for push-mode backups where the
target bitmap doesn't make sense.


The backup job requires the bitmap to be on the source, but the qemu NBD 
export code only requires the bitmap to be locatable somewhere on the 
qemu NBD server requires the bitmap to be discoverable from anywhere on 
the chain, and since the temporary target of the block job has the 
source image as its backing file, that should be the case.  That is:


base <- active <- temp
  |
bitmap0

looking up [active, bitmap0] or [temp, bitmap0] should both succeed; we 
need the former for the blockjob, and the latter for the NBD export.


If the NBD export _can't_ find bitmap0 through the backing chain, that 
may be a symptom of the problem that Max has been trying to fix (his 
upcoming v7 "deal with filters" is hinted at here, but will not be in 4.2):

https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04520.html

In my original implementation, I did not need a duplicated bitmap in the 
temp file.  But that was pre-blockdev.  Are we really hitting filter 
limitations in qemu where the use of blockdev is preventing [temp, 
bitmap0] from finding the bitmap across the backing chain?  If so, we 
should fix that in qemu - but we're so late for 4.2, that I guess 
libvirt will have to work around it.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-05 Thread Peter Krempa 
On Wed, Dec 04, 2019 at 17:12:14 -0600, Eric Blake wrote:
> On 12/3/19 11:17 AM, Peter Krempa wrote:
> > This allows to start and manage the backup job.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   po/POTFILES.in   |   1 +
> >   src/qemu/Makefile.inc.am |   2 +
> >   src/qemu/qemu_backup.c   | 941 +++
> 
> Large patch, but I'm not sure how it could be subdivided.
> 
> >   src/qemu/qemu_backup.h   |  41 ++
> >   src/qemu/qemu_driver.c   |  47 ++
> >   5 files changed, 1032 insertions(+)
> >   create mode 100644 src/qemu/qemu_backup.c
> >   create mode 100644 src/qemu/qemu_backup.h
> > 
> 
> > +++ b/src/qemu/qemu_backup.c
> 
> > +static int
> > +qemuBackupPrepare(virDomainBackupDefPtr def)
> > +{
> > +
> > +if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
> > +if (!def->server) {
> > +def->server = g_new(virStorageNetHostDef, 1);
> > +
> > +def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> > +def->server->name = g_strdup("localhost");
> > +}
> > +
> > +switch ((virStorageNetHostTransport) def->server->transport) {
> > +case VIR_STORAGE_NET_HOST_TRANS_TCP:
> > +/* TODO: Update qemu.conf to provide a port range,
> > + * probably starting at 10809, for obtaining automatic
> > + * port via virPortAllocatorAcquire, as well as store
> > + * somewhere if we need to call virPortAllocatorRelease
> > + * during BackupEnd. Until then, user must provide port */
> 
> This TODO survives from my initial code, and does not seem to be addressed
> later in the series.  Not a show-stopper for the initial implementation, but
> something to remember for followup patches.
> 
> > +if (!def->server->port) {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > +   _(" must specify TCP port for 
> > now"));
> > +return -1;
> > +}
> > +break;
> > +
> > +case VIR_STORAGE_NET_HOST_TRANS_UNIX:
> > +/* TODO: Do we need to mess with selinux? */
> 
> This should be addressed as well (or deleted, if it works out of the box).
> 
> 
> > +static int
> > +qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,
> > +virJSONValuePtr actions,
> > +virDomainMomentObjPtr *incremental)
> > +{
> > +g_autoptr(virJSONValue) mergebitmaps = NULL;
> > +g_autoptr(virJSONValue) mergebitmapsstore = NULL;
> > +
> > +if (!(mergebitmaps = virJSONValueNewArray()))
> > +return -1;
> > +
> > +/* TODO: this code works only if the bitmaps are present on a single 
> > node.
> > + * The algorithm needs to be changed so that it looks into the backing 
> > chain
> > + * so that we can combine all relevant bitmaps for a given backing 
> > chain */
> 
> Correct - but mixing incremental backup with external snapshots is something
> that we know is future work. It's okay for the initial implementation that
> we only support a single node.
> 
> > +while (*incremental) {
> > +if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(mergebitmaps,
> > + 
> > dd->domdisk->src->nodeformat,
> > + 
> > (*incremental)->def->name) < 0)
> > +return -1;
> > +
> > +incremental++;
> > +}
> > +
> > +if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))
> > +return -1;
> > +
> > +if (qemuMonitorTransactionBitmapAdd(actions,
> > +dd->domdisk->src->nodeformat,
> > +dd->incrementalBitmap,
> > +false,
> > +true) < 0)
> > +return -1;
> > +
> > +if (qemuMonitorTransactionBitmapMerge(actions,
> > +  dd->domdisk->src->nodeformat,
> > +  dd->incrementalBitmap,
> > +  ) < 0)
> > +return -1;
> > +
> > +if (qemuMonitorTransactionBitmapAdd(actions,
> > +dd->store->nodeformat,
> > +dd->incrementalBitmap,
> > +false,
> > +true) < 0)
> > +return -1;
> 
> Why do we need two of these calls?
> /me reads on
> 
> > +
> > +if (qemuMonitorTransactionBitmapMerge(actions,
> > +  dd->store->nodeformat,
> > +  dd->incrementalBitmap,
> > +  ) < 0)
> > +return -1;
> 
> okay, it looks like you are trying to merge the same bitmap into two
>

Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-04 Thread Eric Blake 

On 12/3/19 11:17 AM, Peter Krempa wrote:

This allows to start and manage the backup job.

Signed-off-by: Peter Krempa 
---
  po/POTFILES.in   |   1 +
  src/qemu/Makefile.inc.am |   2 +
  src/qemu/qemu_backup.c   | 941 +++


Large patch, but I'm not sure how it could be subdivided.


  src/qemu/qemu_backup.h   |  41 ++
  src/qemu/qemu_driver.c   |  47 ++
  5 files changed, 1032 insertions(+)
  create mode 100644 src/qemu/qemu_backup.c
  create mode 100644 src/qemu/qemu_backup.h




+++ b/src/qemu/qemu_backup.c



+static int
+qemuBackupPrepare(virDomainBackupDefPtr def)
+{
+
+if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
+if (!def->server) {
+def->server = g_new(virStorageNetHostDef, 1);
+
+def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+def->server->name = g_strdup("localhost");
+}
+
+switch ((virStorageNetHostTransport) def->server->transport) {
+case VIR_STORAGE_NET_HOST_TRANS_TCP:
+/* TODO: Update qemu.conf to provide a port range,
+ * probably starting at 10809, for obtaining automatic
+ * port via virPortAllocatorAcquire, as well as store
+ * somewhere if we need to call virPortAllocatorRelease
+ * during BackupEnd. Until then, user must provide port */


This TODO survives from my initial code, and does not seem to be 
addressed later in the series.  Not a show-stopper for the initial 
implementation, but something to remember for followup patches.



+if (!def->server->port) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _(" must specify TCP port for 
now"));
+return -1;
+}
+break;
+
+case VIR_STORAGE_NET_HOST_TRANS_UNIX:
+/* TODO: Do we need to mess with selinux? */


This should be addressed as well (or deleted, if it works out of the box).



+static int
+qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,
+virJSONValuePtr actions,
+virDomainMomentObjPtr *incremental)
+{
+g_autoptr(virJSONValue) mergebitmaps = NULL;
+g_autoptr(virJSONValue) mergebitmapsstore = NULL;
+
+if (!(mergebitmaps = virJSONValueNewArray()))
+return -1;
+
+/* TODO: this code works only if the bitmaps are present on a single node.
+ * The algorithm needs to be changed so that it looks into the backing 
chain
+ * so that we can combine all relevant bitmaps for a given backing chain */


Correct - but mixing incremental backup with external snapshots is 
something that we know is future work. It's okay for the initial 
implementation that we only support a single node.



+while (*incremental) {
+if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(mergebitmaps,
+ 
dd->domdisk->src->nodeformat,
+ 
(*incremental)->def->name) < 0)
+return -1;
+
+incremental++;
+}
+
+if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))
+return -1;
+
+if (qemuMonitorTransactionBitmapAdd(actions,
+dd->domdisk->src->nodeformat,
+dd->incrementalBitmap,
+false,
+true) < 0)
+return -1;
+
+if (qemuMonitorTransactionBitmapMerge(actions,
+  dd->domdisk->src->nodeformat,
+  dd->incrementalBitmap,
+  ) < 0)
+return -1;
+
+if (qemuMonitorTransactionBitmapAdd(actions,
+dd->store->nodeformat,
+dd->incrementalBitmap,
+false,
+true) < 0)
+return -1;


Why do we need two of these calls?
/me reads on


+
+if (qemuMonitorTransactionBitmapMerge(actions,
+  dd->store->nodeformat,
+  dd->incrementalBitmap,
+  ) < 0)
+return -1;


okay, it looks like you are trying to merge the same bitmap into two 
different merge commands, which will all be part of the same 
transaction.  I guess it would be helpful to see a trace of the 
transaction in action to see if we can simplify it (using 2 instead of 4 
qemuMonitor* commands).



+
+
+return 0;
+}
+
+
+static int
+qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,


and this is as far as my review got today.  I'll resume again as soon as 
I can.


Other than one obvious thing I saw in passing:


@@ -22953,6 +22998,8 @@ static

[libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

2019-12-03 Thread Peter Krempa 
This allows to start and manage the backup job.

Signed-off-by: Peter Krempa 
---
 po/POTFILES.in   |   1 +
 src/qemu/Makefile.inc.am |   2 +
 src/qemu/qemu_backup.c   | 941 +++
 src/qemu/qemu_backup.h   |  41 ++
 src/qemu/qemu_driver.c   |  47 ++
 5 files changed, 1032 insertions(+)
 create mode 100644 src/qemu/qemu_backup.c
 create mode 100644 src/qemu/qemu_backup.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 48f3f431ec..5afecf21ba 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -140,6 +140,7 @@
 @SRCDIR@/src/phyp/phyp_driver.c
 @SRCDIR@/src/qemu/qemu_agent.c
 @SRCDIR@/src/qemu/qemu_alias.c
+@SRCDIR@/src/qemu/qemu_backup.c
 @SRCDIR@/src/qemu/qemu_block.c
 @SRCDIR@/src/qemu/qemu_blockjob.c
 @SRCDIR@/src/qemu/qemu_capabilities.c
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index bf30f8a3c5..839b1cacb8 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -69,6 +69,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_vhost_user_gpu.h \
qemu/qemu_checkpoint.c \
qemu/qemu_checkpoint.h \
+   qemu/qemu_backup.c \
+   qemu/qemu_backup.h \
$(NULL)


diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
new file mode 100644
index 00..8307a42e1c
--- /dev/null
+++ b/src/qemu/qemu_backup.c
@@ -0,0 +1,941 @@
+/*
+ * qemu_backup.c: Implementation and handling of the backup jobs
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "qemu_block.h"
+#include "qemu_conf.h"
+#include "qemu_capabilities.h"
+#include "qemu_monitor.h"
+#include "qemu_process.h"
+#include "qemu_backup.h"
+#include "qemu_monitor_json.h"
+#include "qemu_checkpoint.h"
+#include "qemu_command.h"
+
+#include "virerror.h"
+#include "virlog.h"
+#include "virbuffer.h"
+#include "viralloc.h"
+#include "virxml.h"
+#include "virstoragefile.h"
+#include "virstring.h"
+#include "backup_conf.h"
+#include "virdomaincheckpointobjlist.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+VIR_LOG_INIT("qemu.qemu_backup");
+
+
+static virDomainBackupDefPtr
+qemuDomainGetBackup(virDomainObjPtr vm)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+if (!priv->backup) {
+virReportError(VIR_ERR_NO_DOMAIN_BACKUP, "%s",
+   _("no domain backup job present"));
+return NULL;
+}
+
+return priv->backup;
+}
+
+
+static int
+qemuBackupPrepare(virDomainBackupDefPtr def)
+{
+
+if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
+if (!def->server) {
+def->server = g_new(virStorageNetHostDef, 1);
+
+def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+def->server->name = g_strdup("localhost");
+}
+
+switch ((virStorageNetHostTransport) def->server->transport) {
+case VIR_STORAGE_NET_HOST_TRANS_TCP:
+/* TODO: Update qemu.conf to provide a port range,
+ * probably starting at 10809, for obtaining automatic
+ * port via virPortAllocatorAcquire, as well as store
+ * somewhere if we need to call virPortAllocatorRelease
+ * during BackupEnd. Until then, user must provide port */
+if (!def->server->port) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _(" must specify TCP port for 
now"));
+return -1;
+}
+break;
+
+case VIR_STORAGE_NET_HOST_TRANS_UNIX:
+/* TODO: Do we need to mess with selinux? */
+break;
+
+case VIR_STORAGE_NET_HOST_TRANS_RDMA:
+case VIR_STORAGE_NET_HOST_TRANS_LAST:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("unexpected transport in "));
+return -1;
+}
+}
+
+return 0;
+}
+
+
+struct qemuBackupDiskData {
+virDomainBackupDiskDefPtr backupdisk;
+virDomainDiskDefPtr domdisk;
+qemuBlockJobDataPtr blockjob;
+virStorageSourcePtr store;
+char *incrementalBitmap;
+qemuBlockStorageSourceChainDataPtr crdata;
+bool labelled;
+bool initialized;
+bool created;
+bool added;
+bool started;
+bool done;
+};
+
+
+static void
+qemuBackupDiskDataCleanupOne(virDomainObjPtr vm,
+ struct qemuBackupDiskData *dd)
+{
+