[Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Boris Sukholitko]

This patchset is a small rebase of the 9p live migration patches made a year
ago by Benoit Canet.

See http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02190.html
for the previous thread.

I took the liberty to drop the second patch (waiting for completion of 9p
operations) as it wasn't working in my testing.

Thanks,
Boris.

Boris Sukholitko (2):
  virtio-9p: Add support for 9p migration.
  virtio-9p: Remove migration blockers.

 hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p.c        |  24 -------
 hw/9pfs/virtio-9p.h        |   2 -
 3 files changed, 152 insertions(+), 26 deletions(-)

-- 
2.0.2

[Qemu-devel] [PATCH 1/2] virtio-9p: Add support for 9p migration.

[Boris Sukholitko]

This patch is a rebase of Aneesh Kumar's and Benoit Canet's previous
work.

Signed-off-by: Boris Sukholitko <boriss@gmail.com>
---
 hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 2572747..078ad39 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,6 +21,149 @@
 #include "virtio-9p-coth.h"
 #include "hw/virtio/virtio-access.h"
 
+static void virtio_9p_save_path(QEMUFile *f, V9fsPath *path)
+{
+    qemu_put_be16(f, path->size);
+    qemu_put_buffer(f, (const uint8_t *) path->data, path->size);
+}
+
+static void virtio_9p_save_string(QEMUFile *f, V9fsString *s)
+{
+    qemu_put_be16(f, s->size);
+    qemu_put_buffer(f, (const uint8_t *) s->data, s->size);
+}
+
+static void virtio_9p_save_xattr(QEMUFile *f, V9fsXattr *xattr)
+{
+    qemu_put_be64(f, xattr->copied_len);
+    qemu_put_be64(f, xattr->len);
+    qemu_put_buffer(f, (const uint8_t *) xattr->value, xattr->len);
+    virtio_9p_save_string(f, &xattr->name);
+    qemu_put_be32(f, xattr->flags);
+}
+
+static void virtio_9p_save_fid(QEMUFile *f, V9fsFidState *fid)
+{
+    /* First close the fid and mark it for reopen if migration fail */
+    if (fid->fid_type == P9_FID_FILE) {
+        close(fid->fs.fd);
+        fid->fs.fd = -1;
+    } else if (fid->fid_type == P9_FID_DIR) {
+        closedir(fid->fs.dir);
+        fid->fs.dir = NULL;
+    }
+
+    qemu_put_be32(f, fid->fid_type);
+    if (fid->fid_type == P9_FID_XATTR) {
+        /* we don't save fs_reclaim */
+        virtio_9p_save_xattr(f, &fid->fs.xattr);
+    }
+    qemu_put_be32(f, fid->fid);
+    virtio_9p_save_path(f, &fid->path);
+    qemu_put_be32(f, fid->flags);
+    qemu_put_be32(f, fid->open_flags);
+    qemu_put_be32(f, fid->uid);
+    qemu_put_be32(f, fid->ref);
+    qemu_put_be32(f, fid->clunked);
+}
+
+static void virtio_9p_save(QEMUFile *f, void *opaque)
+{
+    int fidcount = 0;
+    V9fsState *s = opaque;
+    V9fsFidState *fid;
+
+    virtio_save(VIRTIO_DEVICE(s), f);
+
+    for (fid = s->fid_list; fid; fid = fid->next) {
+        fidcount++;
+    }
+    /* Write the total number of fid structure */
+    qemu_put_be32(f, fidcount);
+
+    for (fid = s->fid_list; fid; fid = fid->next) {
+        virtio_9p_save_fid(f, fid);
+    }
+
+    qemu_put_be32(f, s->proto_version);
+    qemu_put_be32(f, s->msize);
+}
+
+static void virtio_9p_load_path(QEMUFile *f, V9fsPath *path)
+{
+    path->size = qemu_get_be16(f);
+    path->data = g_malloc0(path->size);
+    qemu_get_buffer(f, (uint8_t *) path->data, path->size);
+}
+
+static void virtio_9p_load_string(QEMUFile *f, V9fsString *s)
+{
+    s->size = qemu_get_be16(f);
+    s->data = g_malloc0(s->size);
+    qemu_get_buffer(f, (uint8_t *) s->data, s->size);
+}
+
+static void virtio_9p_load_xattr(QEMUFile *f, V9fsXattr *xattr)
+{
+    xattr->copied_len = qemu_get_be64(f);
+    xattr->len = qemu_get_be64(f);
+    qemu_get_buffer(f, (uint8_t *) xattr->value, xattr->len);
+    virtio_9p_load_string(f, &xattr->name);
+    xattr->flags = qemu_get_be32(f);
+}
+
+static V9fsFidState *virtio_9p_load_fid(QEMUFile *f)
+{
+    V9fsFidState *fid;
+    fid = g_new0(V9fsFidState, 1);
+
+    fid->fid_type    = qemu_get_be32(f);
+    if (fid->fid_type == P9_FID_XATTR) {
+        virtio_9p_load_xattr(f, &fid->fs.xattr);
+    }
+    fid->fid         = qemu_get_be32(f);
+    virtio_9p_load_path(f, &fid->path);
+    fid->flags       = qemu_get_be32(f);
+    fid->open_flags  = qemu_get_be32(f);
+    fid->uid         = qemu_get_be32(f);
+    fid->ref         = qemu_get_be32(f);
+    fid->clunked     = qemu_get_be32(f);
+
+    /* If it's a file fid mark the file descriptors as closed.
+     * DIR is null thanks to g_new0.
+     * When doing get_fid v9fs_reopen_fid will reopen the file or the directory.
+     */
+    if (fid->fid_type == P9_FID_FILE) {
+        fid->fs.fd = -1;
+    }
+    return fid;
+}
+
+static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
+{
+    int fidcount;
+    V9fsState *s = opaque;
+    V9fsFidState **fid;
+
+    if (version_id != 2) {
+        return -EINVAL;
+    }
+    virtio_load(VIRTIO_DEVICE(s), f, version_id);
+    fidcount = qemu_get_be32(f);
+
+    fid = &s->fid_list;
+    while (fidcount) {
+        *fid = virtio_9p_load_fid(f);
+        fid = &((*fid)->next);
+        fidcount--;
+    }
+
+    s->proto_version  = qemu_get_be32(f);
+    s->msize          = qemu_get_be32(f);
+
+    return 0;
+}
+
 static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
 {
     features |= 1 << VIRTIO_9P_MOUNT_TAG;
@@ -50,6 +193,7 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
     struct stat stat;
     FsDriverEntry *fse;
     V9fsPath path;
+    static int virtio_9p_id;
 
     virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
                 sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
@@ -98,6 +242,14 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
     s->ops = fse->ops;
     s->config_size = sizeof(struct virtio_9p_config) + len;
     s->fid_list = NULL;
+
+    /* Original patch says that instance id must be derived of tag
+     * name. Hash functions do have collisions so why would it be better
+     * than an increment ?
+     */
+    register_savevm(dev, "virtio-9p", virtio_9p_id++, 2,
+                    virtio_9p_save, virtio_9p_load, s);
+
     qemu_co_rwlock_init(&s->rename_lock);
 
     if (s->ops->init(&s->ctx) < 0) {
-- 
2.0.2

[Qemu-devel] [PATCH 2/2] virtio-9p: Remove migration blockers.

[Boris Sukholitko]

Signed-off-by: Boris Sukholitko <boriss@gmail.com>
---
 hw/9pfs/virtio-9p.c | 24 ------------------------
 hw/9pfs/virtio-9p.h |  2 --
 2 files changed, 26 deletions(-)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 5861a5b..2bf3c0a 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -332,19 +332,6 @@ static int put_fid(V9fsPDU *pdu, V9fsFidState *fidp)
      * Don't free the fid if it is in reclaim list
      */
     if (!fidp->ref && fidp->clunked) {
-        if (fidp->fid == pdu->s->root_fid) {
-            /*
-             * if the clunked fid is root fid then we
-             * have unmounted the fs on the client side.
-             * delete the migration blocker. Ideally, this
-             * should be hooked to transport close notification
-             */
-            if (pdu->s->migration_blocker) {
-                migrate_del_blocker(pdu->s->migration_blocker);
-                error_free(pdu->s->migration_blocker);
-                pdu->s->migration_blocker = NULL;
-            }
-        }
         return free_fid(pdu, fidp);
     }
     return 0;
@@ -979,17 +966,6 @@ static void v9fs_attach(void *opaque)
     err += offset;
     trace_v9fs_attach_return(pdu->tag, pdu->id,
                              qid.type, qid.version, qid.path);
-    /*
-     * disable migration if we haven't done already.
-     * attach could get called multiple times for the same export.
-     */
-    if (!s->migration_blocker) {
-        s->root_fid = fid;
-        error_setg(&s->migration_blocker,
-                   "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
-                   s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
-        migrate_add_blocker(s->migration_blocker);
-    }
 out:
     put_fid(pdu, fidp);
 out_nofid:
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 2c3603a..23f8eba 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -222,8 +222,6 @@ typedef struct V9fsState
      * on rename.
      */
     CoRwlock rename_lock;
-    int32_t root_fid;
-    Error *migration_blocker;
     V9fsConf fsconf;
 } V9fsState;
 
-- 
2.0.2

Re: [Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Benoît Canet]

The Friday 26 Sep 2014 à 18:19:55 (+0300), Boris Sukholitko wrote :
> This patchset is a small rebase of the 9p live migration patches made a year
> ago by Benoit Canet.
> 
> See http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02190.html
> for the previous thread.
> 
> I took the liberty to drop the second patch (waiting for completion of 9p
> operations) as it wasn't working in my testing.

It's probable that the second patch has bitrot but I remember I was asked to
write it for a meaningfull reason.
Maybe you should give it a bit more love to resurect it properly.

Best regards

Benoît

> 
> Thanks,
> Boris.
> 
> Boris Sukholitko (2):
>   virtio-9p: Add support for 9p migration.
>   virtio-9p: Remove migration blockers.
> 
>  hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/virtio-9p.c        |  24 -------
>  hw/9pfs/virtio-9p.h        |   2 -
>  3 files changed, 152 insertions(+), 26 deletions(-)
> 
> -- 
> 2.0.2
> 
> 

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: Add support for 9p migration.

[Benoît Canet]

The Friday 26 Sep 2014 à 18:19:56 (+0300), Boris Sukholitko wrote :


> This patch is a rebase of Aneesh Kumar's and Benoit Canet's previous
> work.
> 
> Signed-off-by: Boris Sukholitko <boriss@gmail.com>

If Aneesh and me worked on this you should also keep our signed-off in addition
of yours.

Best regards

Benoît

> ---
>  hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 152 insertions(+)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 2572747..078ad39 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -21,6 +21,149 @@
>  #include "virtio-9p-coth.h"
>  #include "hw/virtio/virtio-access.h"
>  
> +static void virtio_9p_save_path(QEMUFile *f, V9fsPath *path)
> +{
> +    qemu_put_be16(f, path->size);
> +    qemu_put_buffer(f, (const uint8_t *) path->data, path->size);
> +}
> +
> +static void virtio_9p_save_string(QEMUFile *f, V9fsString *s)
> +{
> +    qemu_put_be16(f, s->size);
> +    qemu_put_buffer(f, (const uint8_t *) s->data, s->size);
> +}
> +
> +static void virtio_9p_save_xattr(QEMUFile *f, V9fsXattr *xattr)
> +{
> +    qemu_put_be64(f, xattr->copied_len);
> +    qemu_put_be64(f, xattr->len);
> +    qemu_put_buffer(f, (const uint8_t *) xattr->value, xattr->len);
> +    virtio_9p_save_string(f, &xattr->name);
> +    qemu_put_be32(f, xattr->flags);
> +}
> +
> +static void virtio_9p_save_fid(QEMUFile *f, V9fsFidState *fid)
> +{
> +    /* First close the fid and mark it for reopen if migration fail */
> +    if (fid->fid_type == P9_FID_FILE) {
> +        close(fid->fs.fd);
> +        fid->fs.fd = -1;
> +    } else if (fid->fid_type == P9_FID_DIR) {
> +        closedir(fid->fs.dir);
> +        fid->fs.dir = NULL;
> +    }
> +
> +    qemu_put_be32(f, fid->fid_type);
> +    if (fid->fid_type == P9_FID_XATTR) {
> +        /* we don't save fs_reclaim */
> +        virtio_9p_save_xattr(f, &fid->fs.xattr);
> +    }
> +    qemu_put_be32(f, fid->fid);
> +    virtio_9p_save_path(f, &fid->path);
> +    qemu_put_be32(f, fid->flags);
> +    qemu_put_be32(f, fid->open_flags);
> +    qemu_put_be32(f, fid->uid);
> +    qemu_put_be32(f, fid->ref);
> +    qemu_put_be32(f, fid->clunked);
> +}
> +
> +static void virtio_9p_save(QEMUFile *f, void *opaque)
> +{
> +    int fidcount = 0;
> +    V9fsState *s = opaque;
> +    V9fsFidState *fid;
> +
> +    virtio_save(VIRTIO_DEVICE(s), f);
> +
> +    for (fid = s->fid_list; fid; fid = fid->next) {
> +        fidcount++;
> +    }
> +    /* Write the total number of fid structure */
> +    qemu_put_be32(f, fidcount);
> +
> +    for (fid = s->fid_list; fid; fid = fid->next) {
> +        virtio_9p_save_fid(f, fid);
> +    }
> +
> +    qemu_put_be32(f, s->proto_version);
> +    qemu_put_be32(f, s->msize);
> +}
> +
> +static void virtio_9p_load_path(QEMUFile *f, V9fsPath *path)
> +{
> +    path->size = qemu_get_be16(f);
> +    path->data = g_malloc0(path->size);
> +    qemu_get_buffer(f, (uint8_t *) path->data, path->size);
> +}
> +
> +static void virtio_9p_load_string(QEMUFile *f, V9fsString *s)
> +{
> +    s->size = qemu_get_be16(f);
> +    s->data = g_malloc0(s->size);
> +    qemu_get_buffer(f, (uint8_t *) s->data, s->size);
> +}
> +
> +static void virtio_9p_load_xattr(QEMUFile *f, V9fsXattr *xattr)
> +{
> +    xattr->copied_len = qemu_get_be64(f);
> +    xattr->len = qemu_get_be64(f);
> +    qemu_get_buffer(f, (uint8_t *) xattr->value, xattr->len);
> +    virtio_9p_load_string(f, &xattr->name);
> +    xattr->flags = qemu_get_be32(f);
> +}
> +
> +static V9fsFidState *virtio_9p_load_fid(QEMUFile *f)
> +{
> +    V9fsFidState *fid;
> +    fid = g_new0(V9fsFidState, 1);
> +
> +    fid->fid_type    = qemu_get_be32(f);
> +    if (fid->fid_type == P9_FID_XATTR) {
> +        virtio_9p_load_xattr(f, &fid->fs.xattr);
> +    }
> +    fid->fid         = qemu_get_be32(f);
> +    virtio_9p_load_path(f, &fid->path);
> +    fid->flags       = qemu_get_be32(f);
> +    fid->open_flags  = qemu_get_be32(f);
> +    fid->uid         = qemu_get_be32(f);
> +    fid->ref         = qemu_get_be32(f);
> +    fid->clunked     = qemu_get_be32(f);
> +
> +    /* If it's a file fid mark the file descriptors as closed.
> +     * DIR is null thanks to g_new0.
> +     * When doing get_fid v9fs_reopen_fid will reopen the file or the directory.
> +     */
> +    if (fid->fid_type == P9_FID_FILE) {
> +        fid->fs.fd = -1;
> +    }
> +    return fid;
> +}
> +
> +static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    int fidcount;
> +    V9fsState *s = opaque;
> +    V9fsFidState **fid;
> +
> +    if (version_id != 2) {
> +        return -EINVAL;
> +    }
> +    virtio_load(VIRTIO_DEVICE(s), f, version_id);
> +    fidcount = qemu_get_be32(f);
> +
> +    fid = &s->fid_list;
> +    while (fidcount) {
> +        *fid = virtio_9p_load_fid(f);
> +        fid = &((*fid)->next);
> +        fidcount--;
> +    }
> +
> +    s->proto_version  = qemu_get_be32(f);
> +    s->msize          = qemu_get_be32(f);
> +
> +    return 0;
> +}
> +
>  static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
>  {
>      features |= 1 << VIRTIO_9P_MOUNT_TAG;
> @@ -50,6 +193,7 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>      struct stat stat;
>      FsDriverEntry *fse;
>      V9fsPath path;
> +    static int virtio_9p_id;
>  
>      virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
>                  sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
> @@ -98,6 +242,14 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>      s->ops = fse->ops;
>      s->config_size = sizeof(struct virtio_9p_config) + len;
>      s->fid_list = NULL;
> +
> +    /* Original patch says that instance id must be derived of tag
> +     * name. Hash functions do have collisions so why would it be better
> +     * than an increment ?
> +     */
> +    register_savevm(dev, "virtio-9p", virtio_9p_id++, 2,
> +                    virtio_9p_save, virtio_9p_load, s);
> +
>      qemu_co_rwlock_init(&s->rename_lock);
>  
>      if (s->ops->init(&s->ctx) < 0) {
> -- 
> 2.0.2
> 
> 

Re: [Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Boris Sukholitko]

On Tue, Sep 30, 2014 at 12:46 AM, Benoît Canet <benoit.canet@irqsave.net> wrote:
> The Friday 26 Sep 2014 à 18:19:55 (+0300), Boris Sukholitko wrote :
>> This patchset is a small rebase of the 9p live migration patches made a year
>> ago by Benoit Canet.
>>
>> See http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02190.html
>> for the previous thread.
>>
>> I took the liberty to drop the second patch (waiting for completion of 9p
>> operations) as it wasn't working in my testing.
>
> It's probable that the second patch has bitrot but I remember I was asked to
> write it for a meaningfull reason.

AFAICT, the reason was to drain requests queue before saving the state.

Unfortunately, releasing BQL haven't led to the callbacks being executed.
Therefore deadlock ensued.

> Maybe you should give it a bit more love to resurect it properly.
>

I probably should. Still, IMHO, the two patches work good enough
to deserve merging on their own right :)

Thanks,
Boris.

> Best regards
>
> Benoît
>
>>
>> Thanks,
>> Boris.
>>
>> Boris Sukholitko (2):
>>   virtio-9p: Add support for 9p migration.
>>   virtio-9p: Remove migration blockers.
>>
>>  hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>>  hw/9pfs/virtio-9p.c        |  24 -------
>>  hw/9pfs/virtio-9p.h        |   2 -
>>  3 files changed, 152 insertions(+), 26 deletions(-)
>>
>> --
>> 2.0.2
>>
>>

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: Add support for 9p migration.

[Boris Sukholitko]

On Tue, Sep 30, 2014 at 12:48 AM, Benoît Canet <benoit.canet@irqsave.net> wrote:
> The Friday 26 Sep 2014 à 18:19:56 (+0300), Boris Sukholitko wrote :
>
>
>> This patch is a rebase of Aneesh Kumar's and Benoit Canet's previous
>> work.
>>
>> Signed-off-by: Boris Sukholitko <boriss@gmail.com>
>
> If Aneesh and me worked on this you should also keep our signed-off in addition
> of yours.
>

I will resend the patches with your Signed-off.

Aneesh, should I add your Signed-off as well?

Thanks,
Boris.


> Best regards
>
> Benoît
>
>> ---
>>  hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 152 insertions(+)
>>
>> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
>> index 2572747..078ad39 100644
>> --- a/hw/9pfs/virtio-9p-device.c
>> +++ b/hw/9pfs/virtio-9p-device.c
>> @@ -21,6 +21,149 @@
>>  #include "virtio-9p-coth.h"
>>  #include "hw/virtio/virtio-access.h"
>>
>> +static void virtio_9p_save_path(QEMUFile *f, V9fsPath *path)
>> +{
>> +    qemu_put_be16(f, path->size);
>> +    qemu_put_buffer(f, (const uint8_t *) path->data, path->size);
>> +}
>> +
>> +static void virtio_9p_save_string(QEMUFile *f, V9fsString *s)
>> +{
>> +    qemu_put_be16(f, s->size);
>> +    qemu_put_buffer(f, (const uint8_t *) s->data, s->size);
>> +}
>> +
>> +static void virtio_9p_save_xattr(QEMUFile *f, V9fsXattr *xattr)
>> +{
>> +    qemu_put_be64(f, xattr->copied_len);
>> +    qemu_put_be64(f, xattr->len);
>> +    qemu_put_buffer(f, (const uint8_t *) xattr->value, xattr->len);
>> +    virtio_9p_save_string(f, &xattr->name);
>> +    qemu_put_be32(f, xattr->flags);
>> +}
>> +
>> +static void virtio_9p_save_fid(QEMUFile *f, V9fsFidState *fid)
>> +{
>> +    /* First close the fid and mark it for reopen if migration fail */
>> +    if (fid->fid_type == P9_FID_FILE) {
>> +        close(fid->fs.fd);
>> +        fid->fs.fd = -1;
>> +    } else if (fid->fid_type == P9_FID_DIR) {
>> +        closedir(fid->fs.dir);
>> +        fid->fs.dir = NULL;
>> +    }
>> +
>> +    qemu_put_be32(f, fid->fid_type);
>> +    if (fid->fid_type == P9_FID_XATTR) {
>> +        /* we don't save fs_reclaim */
>> +        virtio_9p_save_xattr(f, &fid->fs.xattr);
>> +    }
>> +    qemu_put_be32(f, fid->fid);
>> +    virtio_9p_save_path(f, &fid->path);
>> +    qemu_put_be32(f, fid->flags);
>> +    qemu_put_be32(f, fid->open_flags);
>> +    qemu_put_be32(f, fid->uid);
>> +    qemu_put_be32(f, fid->ref);
>> +    qemu_put_be32(f, fid->clunked);
>> +}
>> +
>> +static void virtio_9p_save(QEMUFile *f, void *opaque)
>> +{
>> +    int fidcount = 0;
>> +    V9fsState *s = opaque;
>> +    V9fsFidState *fid;
>> +
>> +    virtio_save(VIRTIO_DEVICE(s), f);
>> +
>> +    for (fid = s->fid_list; fid; fid = fid->next) {
>> +        fidcount++;
>> +    }
>> +    /* Write the total number of fid structure */
>> +    qemu_put_be32(f, fidcount);
>> +
>> +    for (fid = s->fid_list; fid; fid = fid->next) {
>> +        virtio_9p_save_fid(f, fid);
>> +    }
>> +
>> +    qemu_put_be32(f, s->proto_version);
>> +    qemu_put_be32(f, s->msize);
>> +}
>> +
>> +static void virtio_9p_load_path(QEMUFile *f, V9fsPath *path)
>> +{
>> +    path->size = qemu_get_be16(f);
>> +    path->data = g_malloc0(path->size);
>> +    qemu_get_buffer(f, (uint8_t *) path->data, path->size);
>> +}
>> +
>> +static void virtio_9p_load_string(QEMUFile *f, V9fsString *s)
>> +{
>> +    s->size = qemu_get_be16(f);
>> +    s->data = g_malloc0(s->size);
>> +    qemu_get_buffer(f, (uint8_t *) s->data, s->size);
>> +}
>> +
>> +static void virtio_9p_load_xattr(QEMUFile *f, V9fsXattr *xattr)
>> +{
>> +    xattr->copied_len = qemu_get_be64(f);
>> +    xattr->len = qemu_get_be64(f);
>> +    qemu_get_buffer(f, (uint8_t *) xattr->value, xattr->len);
>> +    virtio_9p_load_string(f, &xattr->name);
>> +    xattr->flags = qemu_get_be32(f);
>> +}
>> +
>> +static V9fsFidState *virtio_9p_load_fid(QEMUFile *f)
>> +{
>> +    V9fsFidState *fid;
>> +    fid = g_new0(V9fsFidState, 1);
>> +
>> +    fid->fid_type    = qemu_get_be32(f);
>> +    if (fid->fid_type == P9_FID_XATTR) {
>> +        virtio_9p_load_xattr(f, &fid->fs.xattr);
>> +    }
>> +    fid->fid         = qemu_get_be32(f);
>> +    virtio_9p_load_path(f, &fid->path);
>> +    fid->flags       = qemu_get_be32(f);
>> +    fid->open_flags  = qemu_get_be32(f);
>> +    fid->uid         = qemu_get_be32(f);
>> +    fid->ref         = qemu_get_be32(f);
>> +    fid->clunked     = qemu_get_be32(f);
>> +
>> +    /* If it's a file fid mark the file descriptors as closed.
>> +     * DIR is null thanks to g_new0.
>> +     * When doing get_fid v9fs_reopen_fid will reopen the file or the directory.
>> +     */
>> +    if (fid->fid_type == P9_FID_FILE) {
>> +        fid->fs.fd = -1;
>> +    }
>> +    return fid;
>> +}
>> +
>> +static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
>> +{
>> +    int fidcount;
>> +    V9fsState *s = opaque;
>> +    V9fsFidState **fid;
>> +
>> +    if (version_id != 2) {
>> +        return -EINVAL;
>> +    }
>> +    virtio_load(VIRTIO_DEVICE(s), f, version_id);
>> +    fidcount = qemu_get_be32(f);
>> +
>> +    fid = &s->fid_list;
>> +    while (fidcount) {
>> +        *fid = virtio_9p_load_fid(f);
>> +        fid = &((*fid)->next);
>> +        fidcount--;
>> +    }
>> +
>> +    s->proto_version  = qemu_get_be32(f);
>> +    s->msize          = qemu_get_be32(f);
>> +
>> +    return 0;
>> +}
>> +
>>  static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
>>  {
>>      features |= 1 << VIRTIO_9P_MOUNT_TAG;
>> @@ -50,6 +193,7 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>>      struct stat stat;
>>      FsDriverEntry *fse;
>>      V9fsPath path;
>> +    static int virtio_9p_id;
>>
>>      virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
>>                  sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
>> @@ -98,6 +242,14 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
>>      s->ops = fse->ops;
>>      s->config_size = sizeof(struct virtio_9p_config) + len;
>>      s->fid_list = NULL;
>> +
>> +    /* Original patch says that instance id must be derived of tag
>> +     * name. Hash functions do have collisions so why would it be better
>> +     * than an increment ?
>> +     */
>> +    register_savevm(dev, "virtio-9p", virtio_9p_id++, 2,
>> +                    virtio_9p_save, virtio_9p_load, s);
>> +
>>      qemu_co_rwlock_init(&s->rename_lock);
>>
>>      if (s->ops->init(&s->ctx) < 0) {
>> --
>> 2.0.2
>>
>>

Re: [Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Benoît Canet]

The Tuesday 30 Sep 2014 à 22:08:12 (+0300), Boris Sukholitko wrote :
> On Tue, Sep 30, 2014 at 12:46 AM, Benoît Canet <benoit.canet@irqsave.net> wrote:
> > The Friday 26 Sep 2014 à 18:19:55 (+0300), Boris Sukholitko wrote :
> >> This patchset is a small rebase of the 9p live migration patches made a year
> >> ago by Benoit Canet.
> >>
> >> See http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02190.html
> >> for the previous thread.
> >>
> >> I took the liberty to drop the second patch (waiting for completion of 9p
> >> operations) as it wasn't working in my testing.
> >
> > It's probable that the second patch has bitrot but I remember I was asked to
> > write it for a meaningfull reason.
> 
> AFAICT, the reason was to drain requests queue before saving the state.
> 
> Unfortunately, releasing BQL haven't led to the callbacks being executed.
> Therefore deadlock ensued.
> 
> > Maybe you should give it a bit more love to resurect it properly.
> >
> 
> I probably should. Still, IMHO, the two patches work good enough
> to deserve merging on their own right :)

I am afraid nobody will want to merge a patchset where there is a
theorical potential bug.

It should work as well on paper as on silicon.

Best regards

Benoît

> 
> Thanks,
> Boris.
> 
> > Best regards
> >
> > Benoît
> >
> >>
> >> Thanks,
> >> Boris.
> >>
> >> Boris Sukholitko (2):
> >>   virtio-9p: Add support for 9p migration.
> >>   virtio-9p: Remove migration blockers.
> >>
> >>  hw/9pfs/virtio-9p-device.c | 152 +++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/9pfs/virtio-9p.c        |  24 -------
> >>  hw/9pfs/virtio-9p.h        |   2 -
> >>  3 files changed, 152 insertions(+), 26 deletions(-)
> >>
> >> --
> >> 2.0.2
> >>
> >>
> 

Re: [Qemu-devel] [PATCH 0/2] Virtio-9p live migration patchset

[Markus Armbruster]

Benoît Canet <benoit.canet@irqsave.net> writes:

> The Tuesday 30 Sep 2014 à 22:08:12 (+0300), Boris Sukholitko wrote :
>> On Tue, Sep 30, 2014 at 12:46 AM, Benoît Canet
>> <benoit.canet@irqsave.net> wrote:
>> > The Friday 26 Sep 2014 à 18:19:55 (+0300), Boris Sukholitko wrote :
>> >> This patchset is a small rebase of the 9p live migration patches
>> >> made a year
>> >> ago by Benoit Canet.
>> >>
>> >> See http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02190.html
>> >> for the previous thread.
>> >>
>> >> I took the liberty to drop the second patch (waiting for completion of 9p
>> >> operations) as it wasn't working in my testing.
>> >
>> > It's probable that the second patch has bitrot but I remember I was asked to
>> > write it for a meaningfull reason.
>> 
>> AFAICT, the reason was to drain requests queue before saving the state.
>> 
>> Unfortunately, releasing BQL haven't led to the callbacks being executed.
>> Therefore deadlock ensued.
>> 
>> > Maybe you should give it a bit more love to resurect it properly.
>> >
>> 
>> I probably should. Still, IMHO, the two patches work good enough
>> to deserve merging on their own right :)
>
> I am afraid nobody will want to merge a patchset where there is a
> theorical potential bug.
>
> It should work as well on paper as on silicon.

Imperfect or incomplete patches *may* be acceptable when

A. they strictly improve things, and

B. their shortcomings are written down.

Example of a strict improvement: before the patch, live migration always
fails.  After the patch, it succeeds most of the time, but can still
fail in certain states.

Counter-example: before the patch, live migration always fails.  After
the patch, it succeeds, but can corrupt data in certain states.