[Qemu-devel] [PULL 00/14] Migration pull request

[Qemu-devel] [PULL 00/14] Migration pull request

[Juan Quintela]

Hi

In thiss pull request:
- snapshot fixes.  As it caused a crash are imprtant (Denis)
- optimization that was lost on postcopy merge (Dave)
- fix two very small issues discovered by coverity. (Dave)

Apply, please.


The following changes since commit 8f280309030331a912fd8924c129d8bd59e1bdc7:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-11-18 17:07:24 +0000)

are available in the git repository at:

  git://github.com/juanquintela/qemu.git tags/migration/20151119

for you to fetch changes up to 79b3c12ac5714e036a16d1a163a3517d74504f87:

  migration: normalize locking in migration/savevm.c (2015-11-19 11:50:00 +0100)

----------------------------------------------------------------
migration/next for 20151119

----------------------------------------------------------------
Denis V. Lunev (11):
      snapshot: create helper to test that block drivers supports snapshots
      snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
      snapshot: create bdrv_all_delete_snapshot helper
      snapshot: create bdrv_all_goto_snapshot helper
      migration: factor our snapshottability check in load_vmstate
      snapshot: create bdrv_all_find_snapshot helper
      migration: drop find_vmstate_bs check in hmp_delvm
      snapshot: create bdrv_all_create_snapshot helper
      migration: reorder processing in hmp_savevm
      migration: implement bdrv_all_find_vmstate_bs helper
      migration: normalize locking in migration/savevm.c

Dr. David Alan Gilbert (3):
      Set last_sent_block
      migration: Dead assignment of current_time
      Unneeded NULL check

 block/snapshot.c         | 134 +++++++++++++++++++++++++++++-
 include/block/snapshot.h |  24 +++++-
 migration/migration.c    |   3 +-
 migration/ram.c          |   1 +
 migration/savevm.c       | 207 +++++++++++++++--------------------------------
 5 files changed, 217 insertions(+), 152 deletions(-)

[Qemu-devel] [PULL 01/14] Set last_sent_block

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

In a82d593b61054b3dea43 I accidentally removed the setting of
last_sent_block,  put it back.

Symptoms:
  Multithreaded compression only uses one thread.
  Migration is a bit less efficient since it won't use 'cont' flags.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Fixes: a82d593b61054b3dea43
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/ram.c b/migration/ram.c
index 7f32696..1eb155a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1249,6 +1249,7 @@ static int ram_save_target_page(MigrationState *ms, QEMUFile *f,
         if (unsentmap) {
             clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap);
         }
+        last_sent_block = block;
     }

     return res;
-- 
2.5.0

[Qemu-devel] [PULL 02/14] migration: Dead assignment of current_time

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

I set current_time before the postcopy test but never use it;
(I think this was from the original version where it was time based).
Spotted by coverity, CID 1339208

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7e4e27b..265d13a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1643,7 +1643,6 @@ static void *migration_thread(void *opaque)
             if (pending_size && pending_size >= max_size) {
                 /* Still a significant amount to transfer */

-                current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
                 if (migrate_postcopy_ram() &&
                     s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE &&
                     pend_nonpost <= max_size &&
-- 
2.5.0

[Qemu-devel] [PULL 03/14] Unneeded NULL check

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The check is unneccesary, we read the value at the start of the
thread, use it, and never change it.  The value is checked to be
non-NULL before thread creation.

Spotted by coverity, CID 1339211

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 265d13a..1a42aee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1345,7 +1345,7 @@ static void *source_return_path_thread(void *opaque)
             break;
         }
     }
-    if (rp && qemu_file_get_error(rp)) {
+    if (qemu_file_get_error(rp)) {
         trace_source_return_path_thread_bad_end();
         mark_source_rp_bad(ms);
     }
-- 
2.5.0

[Qemu-devel] [PULL 04/14] snapshot: create helper to test that block drivers supports snapshots

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

The patch enforces proper locking for this operation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 24 ++++++++++++++++++++++++
 include/block/snapshot.h |  8 ++++++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 89500f2..d929d08 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -356,3 +356,27 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,

     return ret;
 }
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers) */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+{
+    bool ok = true;
+    BlockDriverState *bs = NULL;
+
+    while (ok && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
+            ok = bdrv_can_snapshot(bs);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return ok;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..6195c9c 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -75,4 +75,12 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
                                          const char *id_or_name,
                                          Error **errp);
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d90e228..4646aa1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1958,19 +1958,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;

-    /* Verify if there is a device that doesn't support snapshots and is writable */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-            continue;
-        }
-
-        if (!bdrv_can_snapshot(bs)) {
-            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
-                               bdrv_get_device_name(bs));
-            return;
-        }
+    if (!bdrv_all_can_snapshot(&bs)) {
+        monitor_printf(mon, "Device '%s' is writable but does not "
+                       "support snapshots.\n", bdrv_get_device_name(bs));
+        return;
     }

     bs = find_vmstate_bs();
-- 
2.5.0

[Qemu-devel] [PULL 05/14] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

this will make code better in the next patch

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 7 ++++---
 include/block/snapshot.h | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index d929d08..ed0422d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -253,9 +253,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
     return -ENOTSUP;
 }

-void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
-                                        const char *id_or_name,
-                                        Error **errp)
+int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name,
+                                       Error **errp)
 {
     int ret;
     Error *local_err = NULL;
@@ -270,6 +270,7 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
     if (ret < 0) {
         error_propagate(errp, local_err);
     }
+    return ret;
 }

 int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 6195c9c..9ddfd42 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -63,9 +63,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
                          const char *snapshot_id,
                          const char *name,
                          Error **errp);
-void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
-                                        const char *id_or_name,
-                                        Error **errp);
+int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name,
+                                       Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-- 
2.5.0

[Qemu-devel] [PULL 06/14] snapshot: create bdrv_all_delete_snapshot helper

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

to delete snapshots from all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 22 ++++++++++++++++++++
 include/block/snapshot.h |  2 ++
 migration/savevm.c       | 54 +++++++++---------------------------------------
 3 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index ed0422d..61a6ad1 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -381,3 +381,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return ok;
 }
+
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
+                             Error **err)
+{
+    int ret = 0;
+    BlockDriverState *bs = NULL;
+    QEMUSnapshotInfo sn1, *snapshot = &sn1;
+
+    while (ret == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs) &&
+                bdrv_snapshot_find(bs, snapshot, name) >= 0) {
+            ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return ret;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9ddfd42..d02d2b1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -82,5 +82,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
  * when appropriate for appropriate block drivers */

 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
+                             Error **err);

 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4646aa1..c52cbbe 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1916,35 +1916,6 @@ static BlockDriverState *find_vmstate_bs(void)
     return NULL;
 }

-/*
- * Deletes snapshots of a given name in all opened images.
- */
-static int del_existing_snapshots(Monitor *mon, const char *name)
-{
-    BlockDriverState *bs;
-    QEMUSnapshotInfo sn1, *snapshot = &sn1;
-    Error *err = NULL;
-
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs) &&
-            bdrv_snapshot_find(bs, snapshot, name) >= 0) {
-            bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
-            if (err) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on device '%s':"
-                               " %s\n",
-                               bdrv_get_device_name(bs),
-                               error_get_pretty(err));
-                error_free(err);
-                return -1;
-            }
-        }
-    }
-
-    return 0;
-}
-
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
@@ -2002,7 +1973,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }

     /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(mon, name) < 0) {
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
         goto the_end;
     }

@@ -2162,20 +2137,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
         return;
     }

-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            err = NULL;
-            bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
-            if (err) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on device '%s':"
-                               " %s\n",
-                               bdrv_get_device_name(bs),
-                               error_get_pretty(err));
-                error_free(err);
-            }
-        }
+    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs), error_get_pretty(err));
+        error_free(err);
     }
 }

-- 
2.5.0

[Qemu-devel] [PULL 07/14] snapshot: create bdrv_all_goto_snapshot helper

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

to switch to snapshot on all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 20 ++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 15 +++++----------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 61a6ad1..9f07a63 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -403,3 +403,23 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
     *first_bad_bs = bs;
     return ret;
 }
+
+
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            err = bdrv_snapshot_goto(bs, name);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index d02d2b1..0a176c7 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -84,5 +84,6 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);

 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index c52cbbe..254e51d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2093,16 +2093,11 @@ int load_vmstate(const char *name)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all();

-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            ret = bdrv_snapshot_goto(bs, name);
-            if (ret < 0) {
-                error_report("Error %d while activating snapshot '%s' on '%s'",
-                             ret, name, bdrv_get_device_name(bs));
-                return ret;
-            }
-        }
+    ret = bdrv_all_goto_snapshot(name, &bs);
+    if (ret < 0) {
+        error_report("Error %d while activating snapshot '%s' on '%s'",
+                     ret, name, bdrv_get_device_name(bs));
+        return ret;
     }

     /* restore the VM state */
-- 
2.5.0

[Qemu-devel] [PULL 08/14] migration: factor our snapshottability check in load_vmstate

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

We should check that all inserted and not read-only images support
snapshotting. This could be made using already invented helper
bdrv_all_can_snapshot().

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 254e51d..2ecc1b3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2051,6 +2051,12 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;

+    if (!bdrv_all_can_snapshot(&bs)) {
+        error_report("Device '%s' is writable but does not support snapshots.",
+                     bdrv_get_device_name(bs));
+        return -ENOTSUP;
+    }
+
     bs_vm_state = find_vmstate_bs();
     if (!bs_vm_state) {
         error_report("No block device supports snapshots");
@@ -2071,15 +2077,8 @@ int load_vmstate(const char *name)
     writable and check if the requested snapshot is available too. */
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-            continue;
-        }
-
         if (!bdrv_can_snapshot(bs)) {
-            error_report("Device '%s' is writable but does not support snapshots.",
-                               bdrv_get_device_name(bs));
-            return -ENOTSUP;
+            continue;
         }

         ret = bdrv_snapshot_find(bs, &sn, name);
-- 
2.5.0

[Qemu-devel] [PULL 09/14] snapshot: create bdrv_all_find_snapshot helper

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

to check that snapshot is available for all loaded block drivers.
The check bs != bs1 in hmp_info_snapshots is an optimization. The check
for availability of this snapshot will return always true as the list
of snapshots was collected from that image.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 20 ++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 42 +++++++++---------------------------------
 3 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..eae4730 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return err;
 }
+
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+    QEMUSnapshotInfo sn;
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            err = bdrv_snapshot_find(bs, &sn, name);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..10ee582 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,6 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);

 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 2ecc1b3..4e6d578 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2056,6 +2056,12 @@ int load_vmstate(const char *name)
                      bdrv_get_device_name(bs));
         return -ENOTSUP;
     }
+    ret = bdrv_all_find_snapshot(name, &bs);
+    if (ret < 0) {
+        error_report("Device '%s' does not have the requested snapshot '%s'",
+                     bdrv_get_device_name(bs), name);
+        return ret;
+    }

     bs_vm_state = find_vmstate_bs();
     if (!bs_vm_state) {
@@ -2073,22 +2079,6 @@ int load_vmstate(const char *name)
         return -EINVAL;
     }

-    /* Verify if there is any device that doesn't support snapshots and is
-    writable and check if the requested snapshot is available too. */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (!bdrv_can_snapshot(bs)) {
-            continue;
-        }
-
-        ret = bdrv_snapshot_find(bs, &sn, name);
-        if (ret < 0) {
-            error_report("Device '%s' does not have the requested snapshot '%s'",
-                           bdrv_get_device_name(bs), name);
-            return ret;
-        }
-    }
-
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all();

@@ -2142,8 +2132,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
-    QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
-    int nb_sns, i, ret, available;
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
     int total;
     int *available_snapshots;

@@ -2167,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     available_snapshots = g_new0(int, nb_sns);
     total = 0;
     for (i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        available = 1;
-        bs1 = NULL;
-
-        while ((bs1 = bdrv_next(bs1))) {
-            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-                if (ret < 0) {
-                    available = 0;
-                    break;
-                }
-            }
-        }
-
-        if (available) {
+        if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
             available_snapshots[total] = i;
             total++;
         }
-- 
2.5.0

[Qemu-devel] [PULL 10/14] migration: drop find_vmstate_bs check in hmp_delvm

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

There is no much sense to do the check and write warning.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 4e6d578..63c07e1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2116,11 +2116,6 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
     Error *err;
     const char *name = qdict_get_str(qdict, "name");

-    if (!find_vmstate_bs()) {
-        monitor_printf(mon, "No block device supports snapshots\n");
-        return;
-    }
-
     if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
         monitor_printf(mon,
                        "Error while deleting snapshot on device '%s': %s\n",
-- 
2.5.0

[Qemu-devel] [PULL 11/14] snapshot: create bdrv_all_create_snapshot helper

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

to create snapshot for all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 26 ++++++++++++++++++++++++++
 include/block/snapshot.h |  4 ++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index eae4730..75d0b96 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -443,3 +443,29 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return err;
 }
+
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+                             BlockDriverState *vm_state_bs,
+                             uint64_t vm_state_size,
+                             BlockDriverState **first_bad_bs)
+{
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bs == vm_state_bs) {
+            sn->vm_state_size = vm_state_size;
+            err = bdrv_snapshot_create(bs, sn);
+        } else if (bdrv_can_snapshot(bs)) {
+            sn->vm_state_size = 0;
+            err = bdrv_snapshot_create(bs, sn);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 10ee582..55e995a 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -86,5 +86,9 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+                             BlockDriverState *vm_state_bs,
+                             uint64_t vm_state_size,
+                             BlockDriverState **first_bad_bs);

 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 63c07e1..80107e3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1996,19 +1996,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         goto the_end;
     }

-    /* create the snapshots */
-
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
-        if (bdrv_can_snapshot(bs1)) {
-            /* Write VM state size only to the image that contains the state */
-            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
-            if (ret < 0) {
-                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
-            }
-        }
+    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
+    if (ret < 0) {
+        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
+                       bdrv_get_device_name(bs));
     }

  the_end:
-- 
2.5.0

[Qemu-devel] [PULL 12/14] migration: reorder processing in hmp_savevm

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

State deletion can be performed on running VM which reduces VM downtime
This approach looks a bit more natural.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 80107e3..98f9a8c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1935,6 +1935,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }

+    /* Delete old snapshots of the same name */
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
+        return;
+    }
+
     bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
@@ -1972,15 +1981,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }

-    /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
-        error_free(local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-- 
2.5.0

[Qemu-devel] [PULL 13/14] migration: implement bdrv_all_find_vmstate_bs helper

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

The patch also ensures proper locking for the operation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block/snapshot.c         | 15 +++++++++++++++
 include/block/snapshot.h |  2 ++
 migration/savevm.c       | 19 ++++---------------
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 75d0b96..6e9fa8d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -469,3 +469,18 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
     *first_bad_bs = bs;
     return err;
 }
+
+BlockDriverState *bdrv_all_find_vmstate_bs(void)
+{
+    bool not_found = true;
+    BlockDriverState *bs = NULL;
+
+    while (not_found && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        not_found = !bdrv_can_snapshot(bs);
+        aio_context_release(ctx);
+    }
+    return bs;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 55e995a..c6910da6 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -91,4 +91,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              uint64_t vm_state_size,
                              BlockDriverState **first_bad_bs);

+BlockDriverState *bdrv_all_find_vmstate_bs(void);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 98f9a8c..a845e69 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,17 +1905,6 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }

-static BlockDriverState *find_vmstate_bs(void)
-{
-    BlockDriverState *bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            return bs;
-        }
-    }
-    return NULL;
-}
-
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
@@ -1944,8 +1933,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }

-    bs = find_vmstate_bs();
-    if (!bs) {
+    bs = bdrv_all_find_vmstate_bs();
+    if (bs == NULL) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
@@ -2054,7 +2043,7 @@ int load_vmstate(const char *name)
         return ret;
     }

-    bs_vm_state = find_vmstate_bs();
+    bs_vm_state = bdrv_all_find_vmstate_bs();
     if (!bs_vm_state) {
         error_report("No block device supports snapshots");
         return -ENOTSUP;
@@ -2123,7 +2112,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     int total;
     int *available_snapshots;

-    bs = find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
-- 
2.5.0

[Qemu-devel] [PULL 14/14] migration: normalize locking in migration/savevm.c

[Juan Quintela]

From: "Denis V. Lunev" <den@openvz.org>

basically all bdrv_* operations must be called under aio_context_acquire
except ones with bdrv_all prefix.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index a845e69..0ad1b93 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1917,6 +1917,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
+    AioContext *aio_context;

     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
@@ -1938,6 +1939,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
+    aio_context = bdrv_get_aio_context(bs);

     saved_vm_running = runstate_is_running();

@@ -1948,6 +1950,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
     vm_stop(RUN_STATE_SAVE_VM);

+    aio_context_acquire(aio_context);
+
     memset(sn, 0, sizeof(*sn));

     /* fill auxiliary fields */
@@ -1992,6 +1996,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }

  the_end:
+    aio_context_release(aio_context);
     if (saved_vm_running) {
         vm_start();
     }
@@ -2030,6 +2035,7 @@ int load_vmstate(const char *name)
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
+    AioContext *aio_context;

     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
@@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
         error_report("No block device supports snapshots");
         return -ENOTSUP;
     }
+    aio_context = bdrv_get_aio_context(bs_vm_state);

     /* Don't even try to load empty VM states */
+    aio_context_acquire(aio_context);
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    aio_context_release(aio_context);
     if (ret < 0) {
         return ret;
     } else if (sn.vm_state_size == 0) {
@@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)

     qemu_system_reset(VMRESET_SILENT);
     migration_incoming_state_new(f);
+
+    aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);
-
     qemu_fclose(f);
+    aio_context_release(aio_context);
+
     migration_incoming_state_destroy();
     if (ret < 0) {
         error_report("Error %d while loading VM state", ret);
@@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     int nb_sns, i;
     int total;
     int *available_snapshots;
+    AioContext *aio_context;

     bs = bdrv_all_find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
     }
+    aio_context = bdrv_get_aio_context(bs);

+    aio_context_acquire(aio_context);
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    aio_context_release(aio_context);
+
     if (nb_sns < 0) {
         monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
         return;
-- 
2.5.0

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 19 November 2015 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> In thiss pull request:
> - snapshot fixes.  As it caused a crash are imprtant (Denis)
> - optimization that was lost on postcopy merge (Dave)
> - fix two very small issues discovered by coverity. (Dave)
>
> Apply, please.
>
>
> The following changes since commit 8f280309030331a912fd8924c129d8bd59e1bdc7:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-11-18 17:07:24 +0000)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20151119
>
> for you to fetch changes up to 79b3c12ac5714e036a16d1a163a3517d74504f87:
>
>   migration: normalize locking in migration/savevm.c (2015-11-19 11:50:00 +0100)
>
> ----------------------------------------------------------------
> migration/next for 20151119

Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):

GTESTER check-qtest-i386
blkdebug: Suspended request 'A'
blkdebug: Resuming request 'A'
**
ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
code should not be reached
GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
main-loop: WARNING: I/O thread spun for 1000 iterations
[vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
extension. Task offloads will be emulated.
make: *** [check-qtest-i386] Error 1

It might be an intermittent test fail from an earlier IDE pull?

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 November 2015 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
>> Hi
>>
>> In thiss pull request:
>> - snapshot fixes.  As it caused a crash are imprtant (Denis)
>> - optimization that was lost on postcopy merge (Dave)
>> - fix two very small issues discovered by coverity. (Dave)
>>
>> Apply, please.
>>
>>
>> The following changes since commit 8f280309030331a912fd8924c129d8bd59e1bdc7:
>>
>>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-11-18 17:07:24 +0000)
>>
>> are available in the git repository at:
>>
>>   git://github.com/juanquintela/qemu.git tags/migration/20151119
>>
>> for you to fetch changes up to 79b3c12ac5714e036a16d1a163a3517d74504f87:
>>
>>   migration: normalize locking in migration/savevm.c (2015-11-19 11:50:00 +0100)
>>
>> ----------------------------------------------------------------
>> migration/next for 20151119
>
> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>
> GTESTER check-qtest-i386
> blkdebug: Suspended request 'A'
> blkdebug: Resuming request 'A'
> **
> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
> code should not be reached
> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> main-loop: WARNING: I/O thread spun for 1000 iterations
> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
> extension. Task offloads will be emulated.
> make: *** [check-qtest-i386] Error 1
>
> It might be an intermittent test fail from an earlier IDE pull?

Yep, intermittent. Second run of the tests passed...

-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>
>> GTESTER check-qtest-i386
>> blkdebug: Suspended request 'A'
>> blkdebug: Resuming request 'A'
>> **
>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>> code should not be reached
>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>> main-loop: WARNING: I/O thread spun for 1000 iterations
>> main-loop: WARNING: I/O thread spun for 1000 iterations
>> main-loop: WARNING: I/O thread spun for 1000 iterations
>> main-loop: WARNING: I/O thread spun for 1000 iterations
>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>> extension. Task offloads will be emulated.
>> make: *** [check-qtest-i386] Error 1
>>
>> It might be an intermittent test fail from an earlier IDE pull?
>
> Yep, intermittent. Second run of the tests passed...

and repro'd on current master, so definitely not the fault of anything
in this pull.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>>
>>> GTESTER check-qtest-i386
>>> blkdebug: Suspended request 'A'
>>> blkdebug: Resuming request 'A'
>>> **
>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>>> code should not be reached
>>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>>> extension. Task offloads will be emulated.
>>> make: *** [check-qtest-i386] Error 1
>>>
>>> It might be an intermittent test fail from an earlier IDE pull?
>>
>> Yep, intermittent. Second run of the tests passed...
>
> and repro'd on current master, so definitely not the fault of anything
> in this pull.

while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
do true; done

will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
number of loops; when it works fine the test does not take an
appreciable amount of time). After a long time the timeout in the
test kicks in and the assert happens.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
> >>>
> >>> GTESTER check-qtest-i386
> >>> blkdebug: Suspended request 'A'
> >>> blkdebug: Resuming request 'A'
> >>> **
> >>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
> >>> code should not be reached
> >>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
> >>> main-loop: WARNING: I/O thread spun for 1000 iterations
> >>> main-loop: WARNING: I/O thread spun for 1000 iterations
> >>> main-loop: WARNING: I/O thread spun for 1000 iterations
> >>> main-loop: WARNING: I/O thread spun for 1000 iterations
> >>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
> >>> extension. Task offloads will be emulated.
> >>> make: *** [check-qtest-i386] Error 1
> >>>
> >>> It might be an intermittent test fail from an earlier IDE pull?
> >>
> >> Yep, intermittent. Second run of the tests passed...
> >
> > and repro'd on current master, so definitely not the fault of anything
> > in this pull.
> 
> while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
> QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
> do true; done
> 
> will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
> number of loops; when it works fine the test does not take an
> appreciable amount of time). After a long time the timeout in the
> test kicks in and the assert happens.

cc'ing in Peter Lieven given his recent IDE buffering changes.

Dave

> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 19 November 2015 at 11:20, Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> In thiss pull request:
> - snapshot fixes.  As it caused a crash are imprtant (Denis)
> - optimization that was lost on postcopy merge (Dave)
> - fix two very small issues discovered by coverity. (Dave)
>
> Apply, please.
>
>
> The following changes since commit 8f280309030331a912fd8924c129d8bd59e1bdc7:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-11-18 17:07:24 +0000)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20151119
>
> for you to fetch changes up to 79b3c12ac5714e036a16d1a163a3517d74504f87:
>
>   migration: normalize locking in migration/savevm.c (2015-11-19 11:50:00 +0100)
>
> ----------------------------------------------------------------
> migration/next for 20151119
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[John Snow]

On 11/19/2015 10:03 AM, Peter Maydell wrote:
> On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>>>
>>>> GTESTER check-qtest-i386
>>>> blkdebug: Suspended request 'A'
>>>> blkdebug: Resuming request 'A'
>>>> **
>>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>>>> code should not be reached
>>>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>>>> extension. Task offloads will be emulated.
>>>> make: *** [check-qtest-i386] Error 1
>>>>
>>>> It might be an intermittent test fail from an earlier IDE pull?
>>>
>>> Yep, intermittent. Second run of the tests passed...
>>
>> and repro'd on current master, so definitely not the fault of anything
>> in this pull.
> 
> while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
> QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
> do true; done
> 
> will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
> number of loops; when it works fine the test does not take an
> appreciable amount of time). After a long time the timeout in the
> test kicks in and the assert happens.
> 
> thanks
> -- PMM
> 

Alright, thanks. Will try to reproduce and investigate. It might
unfortunately be the recent ATAPI pull as a first guess. I'll try to
reproduce with and then without that set.

--js

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Lieven]

Am 19.11.2015 um 16:16 schrieb Dr. David Alan Gilbert:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>>>>
>>>>> GTESTER check-qtest-i386
>>>>> blkdebug: Suspended request 'A'
>>>>> blkdebug: Resuming request 'A'
>>>>> **
>>>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>>>>> code should not be reached
>>>>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>>>>> extension. Task offloads will be emulated.
>>>>> make: *** [check-qtest-i386] Error 1
>>>>>
>>>>> It might be an intermittent test fail from an earlier IDE pull?
>>>> Yep, intermittent. Second run of the tests passed...
>>> and repro'd on current master, so definitely not the fault of anything
>>> in this pull.
>> while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
>> QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
>> do true; done
>>
>> will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
>> number of loops; when it works fine the test does not take an
>> appreciable amount of time). After a long time the timeout in the
>> test kicks in and the assert happens.
> cc'ing in Peter Lieven given his recent IDE buffering changes.

Hi David,

John and I are looking at this at this moment. Thanks for notifying us.

Peter

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Lieven]

Am 19.11.2015 um 16:16 schrieb Dr. David Alan Gilbert:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 19 November 2015 at 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 19 November 2015 at 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 19 November 2015 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> Hi. Unfortunately this failed in 'make check' (x86-64 Linux, debug build):
>>>>>
>>>>> GTESTER check-qtest-i386
>>>>> blkdebug: Suspended request 'A'
>>>>> blkdebug: Resuming request 'A'
>>>>> **
>>>>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/ide-test.c:656:ide_wait_clear:
>>>>> code should not be reached
>>>>> GTester: last random seed: R02Sf1d98a814dee34e3985479976b17883c
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>>>> [vmxnet3][WR][vmxnet3_peer_has_vnet_hdr]: Peer has no virtio
>>>>> extension. Task offloads will be emulated.
>>>>> make: *** [check-qtest-i386] Error 1
>>>>>
>>>>> It might be an intermittent test fail from an earlier IDE pull?
>>>> Yep, intermittent. Second run of the tests passed...
>>> and repro'd on current master, so definitely not the fault of anything
>>> in this pull.
>> while QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
>> QTEST_QEMU_IMG=qemu-img gtester -k --verbose -m=quick tests/ide-test ;
>> do true; done
>>
>> will eventually hang on /i386/ide/cdrom/pio_large (may take a fair
>> number of loops; when it works fine the test does not take an
>> appreciable amount of time). After a long time the timeout in the
>> test kicks in and the assert happens.
> cc'ing in Peter Lieven given his recent IDE buffering changes.

I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
the test does not race:

diff --git a/tests/ide-test.c b/tests/ide-test.c
index d1014bb..ab0489e 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
     for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
         size_t offset = i * (limit / 2);
         size_t rem = (rxsize / 2) - offset;
+        ide_wait_clear(BSY);
         for (j = 0; j < MIN((limit / 2), rem); j++) {
             rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
         }

Note: in the old sync version of the ATAPI PIO implementation this could not happen.

Peter

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Maydell]

On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
> the test does not race:
>
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index d1014bb..ab0489e 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>          size_t offset = i * (limit / 2);
>          size_t rem = (rxsize / 2) - offset;
> +        ide_wait_clear(BSY);
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>          }
>
> Note: in the old sync version of the ATAPI PIO implementation this could not happen.

This certainly fixes the stalls for me, though I don't know enough
IDE to say whether it is the correct fix.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Lieven]

Am 20.11.2015 um 12:33 schrieb Peter Maydell:
> On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
>> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
>> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
>> the test does not race:
>>
>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>> index d1014bb..ab0489e 100644
>> --- a/tests/ide-test.c
>> +++ b/tests/ide-test.c
>> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
>>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>>          size_t offset = i * (limit / 2);
>>          size_t rem = (rxsize / 2) - offset;
>> +        ide_wait_clear(BSY);
>>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>>          }
>>
>> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
> This certainly fixes the stalls for me, though I don't know enough
> IDE to say whether it is the correct fix.
Thanks for testing.

I hope that John or Kevin can verify this fix?

Peter

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Kevin Wolf]

Am 20.11.2015 um 14:44 hat Peter Lieven geschrieben:
> Am 20.11.2015 um 12:33 schrieb Peter Maydell:
> > On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
> >> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
> >> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
> >> the test does not race:
> >>
> >> diff --git a/tests/ide-test.c b/tests/ide-test.c
> >> index d1014bb..ab0489e 100644
> >> --- a/tests/ide-test.c
> >> +++ b/tests/ide-test.c
> >> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
> >>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
> >>          size_t offset = i * (limit / 2);
> >>          size_t rem = (rxsize / 2) - offset;
> >> +        ide_wait_clear(BSY);
> >>          for (j = 0; j < MIN((limit / 2), rem); j++) {
> >>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> >>          }
> >>
> >> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
> > This certainly fixes the stalls for me, though I don't know enough
> > IDE to say whether it is the correct fix.
> Thanks for testing.
> 
> I hope that John or Kevin can verify this fix?

The fix looks correct to me.

While you're improving the test, you could also add an assertion that
DRQ is set after BSY has been cleared.

Kevin

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Peter Lieven]

Am 20.11.2015 um 14:53 schrieb Kevin Wolf:
> Am 20.11.2015 um 14:44 hat Peter Lieven geschrieben:
>> Am 20.11.2015 um 12:33 schrieb Peter Maydell:
>>> On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
>>>> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
>>>> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
>>>> the test does not race:
>>>>
>>>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>>>> index d1014bb..ab0489e 100644
>>>> --- a/tests/ide-test.c
>>>> +++ b/tests/ide-test.c
>>>> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
>>>>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>>>>          size_t offset = i * (limit / 2);
>>>>          size_t rem = (rxsize / 2) - offset;
>>>> +        ide_wait_clear(BSY);
>>>>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>>>>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>>>>          }
>>>>
>>>> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
>>> This certainly fixes the stalls for me, though I don't know enough
>>> IDE to say whether it is the correct fix.
>> Thanks for testing.
>>
>> I hope that John or Kevin can verify this fix?
> The fix looks correct to me.
>
> While you're improving the test, you could also add an assertion that
> DRQ is set after BSY has been cleared.

I would actually move the check (which is already there) into the loop:

diff --git a/tests/ide-test.c b/tests/ide-test.c
index d1014bb..d7ee376 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -712,11 +712,6 @@ static void cdrom_pio_impl(int nblocks)
     /* HPD3: INTRQ_Wait */
     ide_wait_intr(IDE_PRIMARY_IRQ);
 
-    /* HPD2: Check_Status_B */
-    data = ide_wait_clear(BSY);
-    assert_bit_set(data, DRQ | DRDY);
-    assert_bit_clear(data, ERR | DF | BSY);
-
     /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
      * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
      * We allow an odd limit only when the remaining transfer size is
@@ -728,6 +723,10 @@ static void cdrom_pio_impl(int nblocks)
     for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
         size_t offset = i * (limit / 2);
         size_t rem = (rxsize / 2) - offset;
+        /* HPD2: Check_Status_B */
+        data = ide_wait_clear(BSY);
+        assert_bit_set(data, DRQ | DRDY);
+        assert_bit_clear(data, ERR | DF | BSY);
         for (j = 0; j < MIN((limit / 2), rem); j++) {
             rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
         }

Are you okay with that? @John, you also?

Peter

Re: [Qemu-devel] [PULL 00/14] Migration pull request

[Kevin Wolf]

Am 20.11.2015 um 15:00 hat Peter Lieven geschrieben:
> Am 20.11.2015 um 14:53 schrieb Kevin Wolf:
> > Am 20.11.2015 um 14:44 hat Peter Lieven geschrieben:
> >> Am 20.11.2015 um 12:33 schrieb Peter Maydell:
> >>> On 20 November 2015 at 09:38, Peter Lieven <pl@kamp.de> wrote:
> >>>> I wonder if there is a glitch in the PIO implementation of test-ide.c. As far as I understand the specs
> >>>> it is not allowed to read data while the BSY flag is set. With the following change to the test-ide script
> >>>> the test does not race:
> >>>>
> >>>> diff --git a/tests/ide-test.c b/tests/ide-test.c
> >>>> index d1014bb..ab0489e 100644
> >>>> --- a/tests/ide-test.c
> >>>> +++ b/tests/ide-test.c
> >>>> @@ -728,6 +728,7 @@ static void cdrom_pio_impl(int nblocks)
> >>>>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
> >>>>          size_t offset = i * (limit / 2);
> >>>>          size_t rem = (rxsize / 2) - offset;
> >>>> +        ide_wait_clear(BSY);
> >>>>          for (j = 0; j < MIN((limit / 2), rem); j++) {
> >>>>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> >>>>          }
> >>>>
> >>>> Note: in the old sync version of the ATAPI PIO implementation this could not happen.
> >>> This certainly fixes the stalls for me, though I don't know enough
> >>> IDE to say whether it is the correct fix.
> >> Thanks for testing.
> >>
> >> I hope that John or Kevin can verify this fix?
> > The fix looks correct to me.
> >
> > While you're improving the test, you could also add an assertion that
> > DRQ is set after BSY has been cleared.
> 
> I would actually move the check (which is already there) into the loop:
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index d1014bb..d7ee376 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -712,11 +712,6 @@ static void cdrom_pio_impl(int nblocks)
>      /* HPD3: INTRQ_Wait */
>      ide_wait_intr(IDE_PRIMARY_IRQ);
>  
> -    /* HPD2: Check_Status_B */
> -    data = ide_wait_clear(BSY);
> -    assert_bit_set(data, DRQ | DRDY);
> -    assert_bit_clear(data, ERR | DF | BSY);
> -
>      /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
>       * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
>       * We allow an odd limit only when the remaining transfer size is
> @@ -728,6 +723,10 @@ static void cdrom_pio_impl(int nblocks)
>      for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>          size_t offset = i * (limit / 2);
>          size_t rem = (rxsize / 2) - offset;
> +        /* HPD2: Check_Status_B */
> +        data = ide_wait_clear(BSY);
> +        assert_bit_set(data, DRQ | DRDY);
> +        assert_bit_clear(data, ERR | DF | BSY);
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
>              rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
>          }
> 
> Are you okay with that? @John, you also?

Oh, yes, I missed that the check was already there, just in the wrong
place. I agree that this is better.

I also see that we have the state names from the ATA spec in a comment,
so getting that part right might be nice, too. For a start, HPD* are the
wrong states (they are for DMA transfers), it should be HP* everywhere.
And for the part that your patch touches, the loop that actually
transfers data is part of "HP4: Transfer_Data", so we might add a
comment right before the (nested) for.

Kevin