[PATCH 00/16] Next round of migration atomic counters

[PATCH 00/16] Next round of migration atomic counters

[Juan Quintela]

Hi

On this series:

- Make sure that qemu_file_transferred() make sense and its used
  coherently

- Use stat64 for qemu_file_transferred(), so we can call the function
  from any thread.

- Don't account for the same transfer twice (i.e. it is multifd_bytes,
  rdma_bytes or qemu_file_bytes) qemu_file_transferred() just sums all
  of them.

- Use this new counter for rate_limit()

- Remove old trasferred stat64 (now we use the real thing)

- Simplify qemu_file_get_error(): see where next cleanups are coming

- As an example, qemu_fflush() now return errors.

Please review.

Later, Juan.

Based-on: Message-Id: <20230530115429.1998-1-quintela@redhat.com>
Subject: [PULL 00/21] Migration 20230530 patches

Juan Quintela (16):
  qemu-file: Rename qemu_file_transferred_ fast -> noflush
  migration: Change qemu_file_transferred to noflush
  migration: Use qemu_file_transferred_noflush() for block migration.
  qemu-file: Don't call qemu_fflush() for read only files
  qemu-file: We only call qemu_file_transferred_* on the sending side
  qemu_file: Use a stat64 for qemu_file_transferred
  qemu_file: total_transferred is not used anymore
  migration: Use the number of transferred bytes directly
  qemu_file: Remove unused qemu_file_transferred()
  qemu-file: Remove _noflush from qemu_file_transferred_noflush()
  migration: migration_transferred_bytes() don't need the QEMUFile
  migration: migration_rate_limit_reset() don't need the QEMUFile
  qemu-file: Simplify qemu_file_get_error()
  migration: Use migration_transferred_bytes()
  migration: Remove transferred atomic counter
  qemu-file: Make qemu_fflush() return errors

 migration/migration-stats.h | 16 ++++++----------
 migration/qemu-file.h       | 29 +++++------------------------
 migration/colo.c            | 11 +++--------
 migration/migration-stats.c | 10 +++++-----
 migration/migration.c       | 17 ++++++-----------
 migration/multifd.c         |  3 ---
 migration/qemu-file.c       | 35 ++++++++++++++---------------------
 migration/ram.c             | 29 ++++++++++-------------------
 migration/rdma.c            |  4 +---
 migration/savevm.c          |  7 +++----
 migration/vmstate.c         |  4 ++--
 11 files changed, 55 insertions(+), 110 deletions(-)

-- 
2.40.1

[PATCH 01/16] qemu-file: Rename qemu_file_transferred_ fast -> noflush

[Juan Quintela]

Fast don't say much.  Noflush indicates more clearly that it is like
qemu_file_transferred but without the flush.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 11 +++++------
 migration/qemu-file.c |  2 +-
 migration/savevm.c    |  4 ++--
 migration/vmstate.c   |  4 ++--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 83b8fb10de..21c3f34b84 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -52,16 +52,15 @@ int qemu_fclose(QEMUFile *f);
 uint64_t qemu_file_transferred(QEMUFile *f);
 
 /*
- * qemu_file_transferred_fast:
+ * qemu_file_transferred_noflush:
  *
- * As qemu_file_transferred except for writable
- * files, where no flush is performed and the reported
- * amount will include the size of any queued buffers,
- * on top of the amount actually transferred.
+ * As qemu_file_transferred except for writable files, where no flush
+ * is performed and the reported amount will include the size of any
+ * queued buffers, on top of the amount actually transferred.
  *
  * Returns: the total bytes transferred and queued
  */
-uint64_t qemu_file_transferred_fast(QEMUFile *f);
+uint64_t qemu_file_transferred_noflush(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index c94b667726..7a5c1a5e32 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -627,7 +627,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-uint64_t qemu_file_transferred_fast(QEMUFile *f)
+uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
     uint64_t ret = f->total_transferred;
     int i;
diff --git a/migration/savevm.c b/migration/savevm.c
index bc284087f9..f26b455764 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    uint64_t old_offset = qemu_file_transferred_fast(f);
+    uint64_t old_offset = qemu_file_transferred_noflush(f);
     se->ops->save_state(f, se->opaque);
-    uint64_t size = qemu_file_transferred_fast(f) - old_offset;
+    uint64_t size = qemu_file_transferred_noflush(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index af01d54b6f..31842c3afb 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-                old_offset = qemu_file_transferred_fast(f);
+                old_offset = qemu_file_transferred_noflush(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -391,7 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_transferred_fast(f) - old_offset;
+                written_bytes = qemu_file_transferred_noflush(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
                 /* Compressed arrays only care about the first element */
-- 
2.40.1

[PATCH 02/16] migration: Change qemu_file_transferred to noflush

[Juan Quintela]

We do a qemu_fclose() just after that, that also does a qemu_fflush(),
so remove one qemu_fflush().

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

diff --git a/migration/savevm.c b/migration/savevm.c
index f26b455764..b2199d1039 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2952,7 +2952,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
         goto the_end;
     }
     ret = qemu_savevm_state(f, errp);
-    vm_state_size = qemu_file_transferred(f);
+    vm_state_size = qemu_file_transferred_noflush(f);
     ret2 = qemu_fclose(f);
     if (ret < 0) {
         goto the_end;
-- 
2.40.1

[PATCH 03/16] migration: Use qemu_file_transferred_noflush() for block migration.

[Juan Quintela]

We only care about the amount of bytes transferred.  Flushing is done
by the system somewhere else.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index b9580a6c7e..b29e80bdc4 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -748,7 +748,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    uint64_t last_bytes = qemu_file_transferred(f);
+    uint64_t last_bytes = qemu_file_transferred_noflush(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -800,7 +800,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_transferred_noflush(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
-- 
2.40.1

[PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Juan Quintela]

This was the only caller for read only files.  So change the test for
an assert in qemu_fflush().

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 7a5c1a5e32..38984504ba 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -272,9 +272,7 @@ static void qemu_iovec_release_ram(QEMUFile *f)
  */
 void qemu_fflush(QEMUFile *f)
 {
-    if (!qemu_file_is_writable(f)) {
-        return;
-    }
+    g_assert(qemu_file_is_writable(f));
 
     if (qemu_file_get_error(f)) {
         return;
@@ -363,7 +361,9 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
 int qemu_fclose(QEMUFile *f)
 {
     int ret, ret2;
-    qemu_fflush(f);
+    if (qemu_file_is_writable(f)) {
+        qemu_fflush(f);
+    }
     ret = qemu_file_get_error(f);
 
     ret2 = qio_channel_close(f->ioc, NULL);
-- 
2.40.1

[PATCH 05/16] qemu-file: We only call qemu_file_transferred_* on the sending side

[Juan Quintela]

Remove the increase in qemu_file_fill_buffer() and add asserts to
qemu_file_transferred* functions.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 38984504ba..5ff3bc40fd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -340,7 +340,6 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
 
     if (len > 0) {
         f->buf_size += len;
-        f->total_transferred += len;
     } else if (len == 0) {
         qemu_file_set_error_obj(f, -EIO, local_error);
     } else {
@@ -632,6 +631,8 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
     uint64_t ret = f->total_transferred;
     int i;
 
+    g_assert(qemu_file_is_writable(f));
+
     for (i = 0; i < f->iovcnt; i++) {
         ret += f->iov[i].iov_len;
     }
@@ -641,6 +642,7 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 
 uint64_t qemu_file_transferred(QEMUFile *f)
 {
+    g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
     return f->total_transferred;
 }
-- 
2.40.1

[PATCH 06/16] qemu_file: Use a stat64 for qemu_file_transferred

[Juan Quintela]

This way we can read it from any thread.
I checked that it gives the same value than the current one.  We never
use to qemu_files at the same time.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 ++++
 migration/qemu-file.c       | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 2358caad63..b7795e7914 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -81,6 +81,10 @@ typedef struct {
      * Number of bytes sent during precopy stage.
      */
     Stat64 precopy_bytes;
+    /*
+     * Number of bytes transferred with QEMUFile.
+     */
+    Stat64 qemu_file_transferred;
     /*
      * Amount of transferred data at the start of current cycle.
      */
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 5ff3bc40fd..ac7a303142 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -286,6 +286,7 @@ void qemu_fflush(QEMUFile *f)
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
             f->total_transferred += size;
+            stat64_add(&mig_stats.qemu_file_transferred, size);
         }
 
         qemu_iovec_release_ram(f);
@@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
 
 uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
-    uint64_t ret = f->total_transferred;
+    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;
 
     g_assert(qemu_file_is_writable(f));
@@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
 {
     g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
-    return f->total_transferred;
+    return stat64_get(&mig_stats.qemu_file_transferred);
 }
 
 void qemu_put_be16(QEMUFile *f, unsigned int v)
-- 
2.40.1

[PATCH 07/16] qemu_file: total_transferred is not used anymore

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index ac7a303142..a8420dd734 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -41,9 +41,6 @@ struct QEMUFile {
     QIOChannel *ioc;
     bool is_writable;
 
-    /* The sum of bytes transferred on the wire */
-    uint64_t total_transferred;
-
     int buf_index;
     int buf_size; /* 0 when writing */
     uint8_t buf[IO_BUF_SIZE];
@@ -285,7 +282,6 @@ void qemu_fflush(QEMUFile *f)
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
-            f->total_transferred += size;
             stat64_add(&mig_stats.qemu_file_transferred, size);
         }
 
-- 
2.40.1

[PATCH 08/16] migration: Use the number of transferred bytes directly

[Juan Quintela]

We only use migration_transferred_bytes() to calculate the rate_limit,
for that we don't need to flush whatever is on the qemu_file buffer.
Remember that the buffer is really small (normal case is 32K if we use
iov's can be 64 * TARGET_PAGE_SIZE), so this is not relevant to
calculations.

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

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 79eea8d865..1696185694 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
-    uint64_t qemu_file = qemu_file_transferred(f);
+    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
 
     trace_migration_transferred_bytes(qemu_file, multifd, rdma);
     return qemu_file + multifd + rdma;
-- 
2.40.1

[PATCH 09/16] qemu_file: Remove unused qemu_file_transferred()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 18 ------------------
 migration/qemu-file.c |  7 -------
 2 files changed, 25 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 21c3f34b84..b4fb872018 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -33,24 +33,6 @@ QEMUFile *qemu_file_new_input(QIOChannel *ioc);
 QEMUFile *qemu_file_new_output(QIOChannel *ioc);
 int qemu_fclose(QEMUFile *f);
 
-/*
- * qemu_file_transferred:
- *
- * Report the total number of bytes transferred with
- * this file.
- *
- * For writable files, any pending buffers will be
- * flushed, so the reported value will be equal to
- * the number of bytes transferred on the wire.
- *
- * For readable files, the reported value will be
- * equal to the number of bytes transferred on the
- * wire.
- *
- * Returns: the total bytes transferred
- */
-uint64_t qemu_file_transferred(QEMUFile *f);
-
 /*
  * qemu_file_transferred_noflush:
  *
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index a8420dd734..2da092ab77 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -637,13 +637,6 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
     return ret;
 }
 
-uint64_t qemu_file_transferred(QEMUFile *f)
-{
-    g_assert(qemu_file_is_writable(f));
-    qemu_fflush(f);
-    return stat64_get(&mig_stats.qemu_file_transferred);
-}
-
 void qemu_put_be16(QEMUFile *f, unsigned int v)
 {
     qemu_put_byte(f, v >> 8);
-- 
2.40.1

[PATCH 10/16] qemu-file: Remove _noflush from qemu_file_transferred_noflush()

[Juan Quintela]

qemu_file_transferred() don't exist anymore, so we can reuse the name.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 4 ++--
 migration/block.c     | 4 ++--
 migration/qemu-file.c | 2 +-
 migration/savevm.c    | 6 +++---
 migration/vmstate.c   | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index b4fb872018..3575dfa5ff 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -34,7 +34,7 @@ QEMUFile *qemu_file_new_output(QIOChannel *ioc);
 int qemu_fclose(QEMUFile *f);
 
 /*
- * qemu_file_transferred_noflush:
+ * qemu_file_transferred:
  *
  * As qemu_file_transferred except for writable files, where no flush
  * is performed and the reported amount will include the size of any
@@ -42,7 +42,7 @@ int qemu_fclose(QEMUFile *f);
  *
  * Returns: the total bytes transferred and queued
  */
-uint64_t qemu_file_transferred_noflush(QEMUFile *f);
+uint64_t qemu_file_transferred(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/block.c b/migration/block.c
index b29e80bdc4..b9580a6c7e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -748,7 +748,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    uint64_t last_bytes = qemu_file_transferred_noflush(f);
+    uint64_t last_bytes = qemu_file_transferred(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -800,7 +800,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    uint64_t delta_bytes = qemu_file_transferred_noflush(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 2da092ab77..a3d0412b5f 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -623,7 +623,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-uint64_t qemu_file_transferred_noflush(QEMUFile *f)
+uint64_t qemu_file_transferred(QEMUFile *f)
 {
     uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;
diff --git a/migration/savevm.c b/migration/savevm.c
index b2199d1039..751b4a1e31 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    uint64_t old_offset = qemu_file_transferred_noflush(f);
+    uint64_t old_offset = qemu_file_transferred(f);
     se->ops->save_state(f, se->opaque);
-    uint64_t size = qemu_file_transferred_noflush(f) - old_offset;
+    uint64_t size = qemu_file_transferred(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
@@ -2952,7 +2952,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
         goto the_end;
     }
     ret = qemu_savevm_state(f, errp);
-    vm_state_size = qemu_file_transferred_noflush(f);
+    vm_state_size = qemu_file_transferred(f);
     ret2 = qemu_fclose(f);
     if (ret < 0) {
         goto the_end;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..fd8035ea32 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-                old_offset = qemu_file_transferred_noflush(f);
+                old_offset = qemu_file_transferred(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -391,7 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_transferred_noflush(f) - old_offset;
+                written_bytes = qemu_file_transferred(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
                 /* Compressed arrays only care about the first element */
-- 
2.40.1

[PATCH 11/16] migration: migration_transferred_bytes() don't need the QEMUFile

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 +---
 migration/migration-stats.c | 6 +++---
 migration/migration.c       | 6 +++---
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index b7795e7914..e3863bf9bb 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -137,11 +137,9 @@ void migration_rate_set(uint64_t new_rate);
 /**
  * migration_transferred_bytes: Return number of bytes transferred
  *
- * @f: QEMUFile used for main migration channel
- *
  * Returns how many bytes have we transferred since the beginning of
  * the migration.  It accounts for bytes sent through any migration
  * channel, multifd, qemu_file, rdma, ....
  */
-uint64_t migration_transferred_bytes(QEMUFile *f);
+uint64_t migration_transferred_bytes(void);
 #endif
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 1696185694..a5f3fcc5ae 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -25,7 +25,7 @@ bool migration_rate_exceeded(QEMUFile *f)
     }
 
     uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
-    uint64_t rate_limit_current = migration_transferred_bytes(f);
+    uint64_t rate_limit_current = migration_transferred_bytes();
     uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
     uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);
 
@@ -55,10 +55,10 @@ void migration_rate_set(uint64_t limit)
 
 void migration_rate_reset(QEMUFile *f)
 {
-    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
+    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes());
 }
 
-uint64_t migration_transferred_bytes(QEMUFile *f)
+uint64_t migration_transferred_bytes(void)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
diff --git a/migration/migration.c b/migration/migration.c
index 31d30a2ac5..24e55c5bf3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2628,7 +2628,7 @@ static MigThrError migration_detect_error(MigrationState *s)
 
 static void migration_calculate_complete(MigrationState *s)
 {
-    uint64_t bytes = migration_transferred_bytes(s->to_dst_file);
+    uint64_t bytes = migration_transferred_bytes();
     int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     int64_t transfer_time;
 
@@ -2654,7 +2654,7 @@ static void update_iteration_initial_status(MigrationState *s)
      * wrong speed calculation.
      */
     s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    s->iteration_initial_bytes = migration_transferred_bytes(s->to_dst_file);
+    s->iteration_initial_bytes = migration_transferred_bytes();
     s->iteration_initial_pages = ram_get_total_transferred_pages();
 }
 
@@ -2669,7 +2669,7 @@ static void migration_update_counters(MigrationState *s,
         return;
     }
 
-    current_bytes = migration_transferred_bytes(s->to_dst_file);
+    current_bytes = migration_transferred_bytes();
     transferred = current_bytes - s->iteration_initial_bytes;
     time_spent = current_time - s->iteration_start_time;
     bandwidth = (double)transferred / time_spent;
-- 
2.40.1

[PATCH 12/16] migration: migration_rate_limit_reset() don't need the QEMUFile

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 +---
 migration/migration-stats.c | 2 +-
 migration/migration.c       | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index e3863bf9bb..68f3939188 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -120,10 +120,8 @@ uint64_t migration_rate_get(void);
  * migration_rate_reset: Reset the rate limit counter.
  *
  * This is called when we know we start a new transfer cycle.
- *
- * @f: QEMUFile used for main migration channel
  */
-void migration_rate_reset(QEMUFile *f);
+void migration_rate_reset(void);
 
 /**
  * migration_rate_set: Set the maximum amount that can be transferred.
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index a5f3fcc5ae..df541fa904 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -53,7 +53,7 @@ void migration_rate_set(uint64_t limit)
     stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
 }
 
-void migration_rate_reset(QEMUFile *f)
+void migration_rate_reset(void)
 {
     stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes());
 }
diff --git a/migration/migration.c b/migration/migration.c
index 24e55c5bf3..ab8254e433 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2693,7 +2693,7 @@ static void migration_update_counters(MigrationState *s,
             stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
     }
 
-    migration_rate_reset(s->to_dst_file);
+    migration_rate_reset();
 
     update_iteration_initial_status(s);
 
-- 
2.40.1

[PATCH 13/16] qemu-file: Simplify qemu_file_get_error()

[Juan Quintela]

If we pass a NULL error is the same that returning dirrectly the value.

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

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index a3d0412b5f..7cf457a981 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -209,7 +209,7 @@ void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
  */
 int qemu_file_get_error(QEMUFile *f)
 {
-    return qemu_file_get_error_obj(f, NULL);
+    return f->last_error;
 }
 
 /*
-- 
2.40.1

[PATCH 14/16] migration: Use migration_transferred_bytes()

[Juan Quintela]

There are only two differnces with the old value:

- the amount of QEMUFile that hasn't yet been flushed.  It can be
  discussed what is more exact, the new or the old one.
- the amount of transferred bytes that we forgot to account for (the
  newer is better, i.e. exact).

Notice that this two values are used to:
a - present to the user
b - calculate the rate_limit

So a few KB here and there is not going to make a difference.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 2 +-
 migration/ram.c       | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ab8254e433..6ed55d618a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -912,7 +912,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     size_t page_size = qemu_target_page_size();
 
     info->ram = g_malloc0(sizeof(*info->ram));
-    info->ram->transferred = stat64_get(&mig_stats.transferred);
+    info->ram->transferred = migration_transferred_bytes();
     info->ram->total = ram_bytes_total();
     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
     /* legacy value.  It is not used anymore */
diff --git a/migration/ram.c b/migration/ram.c
index 87fc504674..0675421f4e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -552,7 +552,7 @@ void mig_throttle_counter_reset(void)
 
     rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     rs->num_dirty_pages_period = 0;
-    rs->bytes_xfer_prev = stat64_get(&mig_stats.transferred);
+    rs->bytes_xfer_prev = migration_transferred_bytes();
 }
 
 /**
@@ -988,7 +988,7 @@ static void migration_trigger_throttle(RAMState *rs)
 {
     uint64_t threshold = migrate_throttle_trigger_threshold();
     uint64_t bytes_xfer_period =
-        stat64_get(&mig_stats.transferred) - rs->bytes_xfer_prev;
+        migration_transferred_bytes() - rs->bytes_xfer_prev;
     uint64_t bytes_dirty_period = rs->num_dirty_pages_period * TARGET_PAGE_SIZE;
     uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100;
 
@@ -1051,7 +1051,7 @@ static void migration_bitmap_sync(RAMState *rs, bool last_stage)
         /* reset period counters */
         rs->time_last_bitmap_sync = end_time;
         rs->num_dirty_pages_period = 0;
-        rs->bytes_xfer_prev = stat64_get(&mig_stats.transferred);
+        rs->bytes_xfer_prev = migration_transferred_bytes();
     }
     if (migrate_events()) {
         uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
-- 
2.40.1

[PATCH 15/16] migration: Remove transferred atomic counter

[Juan Quintela]

After last commit, it is a write only variable.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 ----
 migration/multifd.c         | 3 ---
 migration/ram.c             | 1 -
 3 files changed, 8 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 68f3939188..05290ade76 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -97,10 +97,6 @@ typedef struct {
      * Number of bytes sent through RDMA.
      */
     Stat64 rdma_bytes;
-    /*
-     * Total number of bytes transferred.
-     */
-    Stat64 transferred;
     /*
      * Number of pages transferred that were full of zeros.
      */
diff --git a/migration/multifd.c b/migration/multifd.c
index 0bf5958a9c..b297d1cb62 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -188,7 +188,6 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
         return -1;
     }
     stat64_add(&mig_stats.multifd_bytes, size);
-    stat64_add(&mig_stats.transferred, size);
     return 0;
 }
 
@@ -715,7 +714,6 @@ static void *multifd_send_thread(void *opaque)
                     break;
                 }
                 stat64_add(&mig_stats.multifd_bytes, p->packet_len);
-                stat64_add(&mig_stats.transferred, p->packet_len);
             } else {
                 /* Send header using the same writev call */
                 p->iov[0].iov_len = p->packet_len;
@@ -729,7 +727,6 @@ static void *multifd_send_thread(void *opaque)
             }
 
             stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
-            stat64_add(&mig_stats.transferred, p->next_packet_size);
             qemu_mutex_lock(&p->mutex);
             p->pending_job--;
             qemu_mutex_unlock(&p->mutex);
diff --git a/migration/ram.c b/migration/ram.c
index 0675421f4e..cdf7783d65 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -443,7 +443,6 @@ void ram_transferred_add(uint64_t bytes)
     } else {
         stat64_add(&mig_stats.downtime_bytes, bytes);
     }
-    stat64_add(&mig_stats.transferred, bytes);
 }
 
 struct MigrationOps {
-- 
2.40.1

[PATCH 16/16] qemu-file: Make qemu_fflush() return errors

[Juan Quintela]

This let us simplify code of this shape.

   qemu_fflush(f);
   int ret = qemu_file_get_error(f);
   if (ret) {
      return ret;
   }

into:

   int ret = qemu_fflush(f);
   if (ret) {
      return ret;
   }

I updated all callers where there is any error check.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h |  2 +-
 migration/colo.c      | 11 +++--------
 migration/migration.c |  7 +------
 migration/qemu-file.c |  7 ++++---
 migration/ram.c       | 22 +++++++---------------
 migration/rdma.c      |  4 +---
 migration/savevm.c    |  3 +--
 7 files changed, 18 insertions(+), 38 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 3575dfa5ff..bc9128d333 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -75,7 +75,7 @@ void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
 void qemu_file_set_error(QEMUFile *f, int ret);
 int qemu_file_shutdown(QEMUFile *f);
 QEMUFile *qemu_file_get_return_path(QEMUFile *f);
-void qemu_fflush(QEMUFile *f);
+int qemu_fflush(QEMUFile *f);
 void qemu_file_set_blocking(QEMUFile *f, bool block);
 int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
 
diff --git a/migration/colo.c b/migration/colo.c
index 72f4f7b37e..4447e34914 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -314,9 +314,7 @@ static void colo_send_message(QEMUFile *f, COLOMessage msg,
         return;
     }
     qemu_put_be32(f, msg);
-    qemu_fflush(f);
-
-    ret = qemu_file_get_error(f);
+    ret = qemu_fflush(f);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Can't send COLO message");
     }
@@ -335,9 +333,7 @@ static void colo_send_message_value(QEMUFile *f, COLOMessage msg,
         return;
     }
     qemu_put_be64(f, value);
-    qemu_fflush(f);
-
-    ret = qemu_file_get_error(f);
+    ret = qemu_fflush(f);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed to send value for message:%s",
                          COLOMessage_str(msg));
@@ -483,8 +479,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
-    qemu_fflush(s->to_dst_file);
-    ret = qemu_file_get_error(s->to_dst_file);
+    ret = qemu_fflush(s->to_dst_file);
     if (ret < 0) {
         goto out;
     }
diff --git a/migration/migration.c b/migration/migration.c
index 6ed55d618a..0195105c59 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -297,12 +297,7 @@ static int migrate_send_rp_message(MigrationIncomingState *mis,
     qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
     qemu_put_be16(mis->to_src_file, len);
     qemu_put_buffer(mis->to_src_file, data, len);
-    qemu_fflush(mis->to_src_file);
-
-    /* It's possible that qemu file got error during sending */
-    ret = qemu_file_get_error(mis->to_src_file);
-
-    return ret;
+    return qemu_fflush(mis->to_src_file);
 }
 
 /* Request one page from the source VM at the given start address.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 7cf457a981..3e8df20c55 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -267,12 +267,12 @@ static void qemu_iovec_release_ram(QEMUFile *f)
  * This will flush all pending data. If data was only partially flushed, it
  * will set an error state.
  */
-void qemu_fflush(QEMUFile *f)
+int qemu_fflush(QEMUFile *f)
 {
     g_assert(qemu_file_is_writable(f));
 
-    if (qemu_file_get_error(f)) {
-        return;
+    if (f->last_error) {
+        return f->last_error;
     }
     if (f->iovcnt > 0) {
         Error *local_error = NULL;
@@ -290,6 +290,7 @@ void qemu_fflush(QEMUFile *f)
 
     f->buf_index = 0;
     f->iovcnt = 0;
+    return f->last_error;
 }
 
 /*
diff --git a/migration/ram.c b/migration/ram.c
index cdf7783d65..26baa54d98 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -301,17 +301,15 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
 
     qemu_put_be64(file, size);
     qemu_put_buffer(file, (const uint8_t *)le_bitmap, size);
+    g_free(le_bitmap);
     /*
      * Mark as an end, in case the middle part is screwed up due to
      * some "mysterious" reason.
      */
     qemu_put_be64(file, RAMBLOCK_RECV_BITMAP_ENDING);
-    qemu_fflush(file);
-
-    g_free(le_bitmap);
-
-    if (qemu_file_get_error(file)) {
-        return qemu_file_get_error(file);
+    int ret = qemu_fflush(file);
+    if (ret) {
+        return ret;
     }
 
     return size + sizeof(size);
@@ -3034,9 +3032,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 /**
@@ -3155,10 +3151,8 @@ out:
         }
 
         qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-        qemu_fflush(f);
         ram_transferred_add(8);
-
-        ret = qemu_file_get_error(f);
+        ret = qemu_fflush(f);
     }
     if (ret < 0) {
         return ret;
@@ -3235,9 +3229,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
     }
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
diff --git a/migration/rdma.c b/migration/rdma.c
index f912a31b45..3ef35fc635 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3886,9 +3886,7 @@ int rdma_registration_start(QEMUFile *f, uint64_t flags)
 
     trace_rdma_registration_start(flags);
     qemu_put_be64(f, RAM_SAVE_FLAG_HOOK);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 /*
diff --git a/migration/savevm.c b/migration/savevm.c
index 751b4a1e31..1dc46b0967 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1536,8 +1536,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     }
 
 flush:
-    qemu_fflush(f);
-    return 0;
+    return qemu_fflush(f);
 }
 
 /* Give an estimate of the amount left to be transferred,
-- 
2.40.1

Re: [PATCH 11/16] migration: migration_transferred_bytes() don't need the QEMUFile

[Philippe Mathieu-Daudé]

On 30/5/23 14:28, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/migration-stats.h | 4 +---
>   migration/migration-stats.c | 6 +++---
>   migration/migration.c       | 6 +++---
>   3 files changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 12/16] migration: migration_rate_limit_reset() don't need the QEMUFile

[Philippe Mathieu-Daudé]

On 30/5/23 14:28, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/migration-stats.h | 4 +---
>   migration/migration-stats.c | 2 +-
>   migration/migration.c       | 2 +-
>   3 files changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/16] qemu-file: Rename qemu_file_transferred_ fast -> noflush

[Philippe Mathieu-Daudé]

On 30/5/23 14:27, Juan Quintela wrote:
> Fast don't say much.  Noflush indicates more clearly that it is like
> qemu_file_transferred but without the flush.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/qemu-file.h | 11 +++++------
>   migration/qemu-file.c |  2 +-
>   migration/savevm.c    |  4 ++--
>   migration/vmstate.c   |  4 ++--
>   4 files changed, 10 insertions(+), 11 deletions(-)

To clarify further, qemu_file_transferred() could be
renamed as qemu_file_transferred_flush[ed]().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/16] migration: Change qemu_file_transferred to noflush

[Philippe Mathieu-Daudé]

On 30/5/23 14:27, Juan Quintela wrote:
> We do a qemu_fclose() just after that, that also does a qemu_fflush(),
> so remove one qemu_fflush().
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/savevm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/16] qemu-file: Rename qemu_file_transferred_ fast -> noflush

[Philippe Mathieu-Daudé]

On 30/5/23 14:36, Philippe Mathieu-Daudé wrote:
> On 30/5/23 14:27, Juan Quintela wrote:
>> Fast don't say much.  Noflush indicates more clearly that it is like
>> qemu_file_transferred but without the flush.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/qemu-file.h | 11 +++++------
>>   migration/qemu-file.c |  2 +-
>>   migration/savevm.c    |  4 ++--
>>   migration/vmstate.c   |  4 ++--
>>   4 files changed, 10 insertions(+), 11 deletions(-)
> 
> To clarify further, qemu_file_transferred() could be
> renamed as qemu_file_transferred_flush[ed]().

Never mind, it gets removed later in this series.

Re: [PATCH 00/16] Next round of migration atomic counters

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Hi

Hi

Fiona, I forgot to put your on the CC'd list.  This series should reget
the speed problem that you find.

Later, Juan.

> On this series:
>
> - Make sure that qemu_file_transferred() make sense and its used
>   coherently
>
> - Use stat64 for qemu_file_transferred(), so we can call the function
>   from any thread.
>
> - Don't account for the same transfer twice (i.e. it is multifd_bytes,
>   rdma_bytes or qemu_file_bytes) qemu_file_transferred() just sums all
>   of them.
>
> - Use this new counter for rate_limit()
>
> - Remove old trasferred stat64 (now we use the real thing)
>
> - Simplify qemu_file_get_error(): see where next cleanups are coming
>
> - As an example, qemu_fflush() now return errors.
>
> Please review.
>
> Later, Juan.
>
> Based-on: Message-Id: <20230530115429.1998-1-quintela@redhat.com>
> Subject: [PULL 00/21] Migration 20230530 patches
>
> Juan Quintela (16):
>   qemu-file: Rename qemu_file_transferred_ fast -> noflush
>   migration: Change qemu_file_transferred to noflush
>   migration: Use qemu_file_transferred_noflush() for block migration.
>   qemu-file: Don't call qemu_fflush() for read only files
>   qemu-file: We only call qemu_file_transferred_* on the sending side
>   qemu_file: Use a stat64 for qemu_file_transferred
>   qemu_file: total_transferred is not used anymore
>   migration: Use the number of transferred bytes directly
>   qemu_file: Remove unused qemu_file_transferred()
>   qemu-file: Remove _noflush from qemu_file_transferred_noflush()
>   migration: migration_transferred_bytes() don't need the QEMUFile
>   migration: migration_rate_limit_reset() don't need the QEMUFile
>   qemu-file: Simplify qemu_file_get_error()
>   migration: Use migration_transferred_bytes()
>   migration: Remove transferred atomic counter
>   qemu-file: Make qemu_fflush() return errors
>
>  migration/migration-stats.h | 16 ++++++----------
>  migration/qemu-file.h       | 29 +++++------------------------
>  migration/colo.c            | 11 +++--------
>  migration/migration-stats.c | 10 +++++-----
>  migration/migration.c       | 17 ++++++-----------
>  migration/multifd.c         |  3 ---
>  migration/qemu-file.c       | 35 ++++++++++++++---------------------
>  migration/ram.c             | 29 ++++++++++-------------------
>  migration/rdma.c            |  4 +---
>  migration/savevm.c          |  7 +++----
>  migration/vmstate.c         |  4 ++--
>  11 files changed, 55 insertions(+), 110 deletions(-)

Re: [PATCH 16/16] qemu-file: Make qemu_fflush() return errors

[Philippe Mathieu-Daudé]

On 30/5/23 14:28, Juan Quintela wrote:
> This let us simplify code of this shape.
> 
>     qemu_fflush(f);
>     int ret = qemu_file_get_error(f);
>     if (ret) {
>        return ret;
>     }
> 
> into:
> 
>     int ret = qemu_fflush(f);
>     if (ret) {
>        return ret;
>     }
> 
> I updated all callers where there is any error check.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/qemu-file.h |  2 +-
>   migration/colo.c      | 11 +++--------
>   migration/migration.c |  7 +------
>   migration/qemu-file.c |  7 ++++---
>   migration/ram.c       | 22 +++++++---------------
>   migration/rdma.c      |  4 +---
>   migration/savevm.c    |  3 +--
>   7 files changed, 18 insertions(+), 38 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/16] qemu-file: Remove _noflush from qemu_file_transferred_noflush()

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> qemu_file_transferred() don't exist anymore, so we can reuse the name.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.h | 4 ++--
>  migration/block.c     | 4 ++--
>  migration/qemu-file.c | 2 +-
>  migration/savevm.c    | 6 +++---
>  migration/vmstate.c   | 4 ++--
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index b4fb872018..3575dfa5ff 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -34,7 +34,7 @@ QEMUFile *qemu_file_new_output(QIOChannel *ioc);
>  int qemu_fclose(QEMUFile *f);
>  
>  /*
> - * qemu_file_transferred_noflush:
> + * qemu_file_transferred:
>   *
>   * As qemu_file_transferred except for writable files, where no flush

Docs need updating^

>   * is performed and the reported amount will include the size of any
> @@ -42,7 +42,7 @@ int qemu_fclose(QEMUFile *f);
>   *
>   * Returns: the total bytes transferred and queued
>   */
> -uint64_t qemu_file_transferred_noflush(QEMUFile *f);
> +uint64_t qemu_file_transferred(QEMUFile *f);

Re: [PATCH 13/16] qemu-file: Simplify qemu_file_get_error()

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> If we pass a NULL error is the same that returning dirrectly the value.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 03/16] migration: Use qemu_file_transferred_noflush() for block migration.

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> We only care about the amount of bytes transferred.  Flushing is done
> by the system somewhere else.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> This was the only caller for read only files.  So change the test for
> an assert in qemu_fflush().
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Richard Henderson]

On 5/30/23 05:28, Juan Quintela wrote:
> This was the only caller for read only files.  So change the test for
> an assert in qemu_fflush().


Not a fan, as fflush(stdin) is well-defined.


r~

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 5/30/23 05:28, Juan Quintela wrote:
>> This was the only caller for read only files.  So change the test for
>> an assert in qemu_fflush().
>
>
> Not a fan, as fflush(stdin) is well-defined.

I guess you mean this:

       For input streams associated with seekable files (e.g., disk files, but
       not pipes or terminals), fflush() discards any buffered data  that  has
       been fetched from the underlying file, but has not been consumed by the
       application.

Two things:
- Current code just do nothing for imput streams
- We only call it from qemu_fclose()
- If we drop anything from the input stream, migration get broken.

If it makes you feel better, I can rename the function to
qemu_file_write_buffer() or whatever name your preffer.

Later, Juan.

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Richard Henderson]

On 5/30/23 10:06, Juan Quintela wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
>> On 5/30/23 05:28, Juan Quintela wrote:
>>> This was the only caller for read only files.  So change the test for
>>> an assert in qemu_fflush().
>>
>>
>> Not a fan, as fflush(stdin) is well-defined.
> 
> I guess you mean this:
> 
>         For input streams associated with seekable files (e.g., disk files, but
>         not pipes or terminals), fflush() discards any buffered data  that  has
>         been fetched from the underlying file, but has not been consumed by the
>         application.

Yes, in that, importantly, it does not assert.

> 
> Two things:
> - Current code just do nothing for imput streams
> - We only call it from qemu_fclose()

Pardon?  There are nearly 30 calls to qemu_fflush.

> - If we drop anything from the input stream, migration get broken.

I'm not talking about dropping anything.  Obviously QEMUFile works different from stdio, 
and therefore resetting the file state to that of the unbuffered data is not relevant.



r~

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 5/30/23 10:06, Juan Quintela wrote:
>> Richard Henderson <richard.henderson@linaro.org> wrote:
>>> On 5/30/23 05:28, Juan Quintela wrote:
>>>> This was the only caller for read only files.  So change the test for
>>>> an assert in qemu_fflush().
>>>
>>>
>>> Not a fan, as fflush(stdin) is well-defined.
>> I guess you mean this:
>>         For input streams associated with seekable files (e.g., disk
>> files, but
>>         not pipes or terminals), fflush() discards any buffered data  that  has
>>         been fetched from the underlying file, but has not been consumed by the
>>         application.
>
> Yes, in that, importantly, it does not assert.

I can let it do nothing if it makes you feel better.

>> Two things:
>> - Current code just do nothing for imput streams
>> - We only call it from qemu_fclose()
>
> Pardon?  There are nearly 30 calls to qemu_fflush.

None of them in a file that is not open for writting.

>> - If we drop anything from the input stream, migration get broken.
>
> I'm not talking about dropping anything.  Obviously QEMUFile works
> different from stdio, and therefore resetting the file state to that
> of the unbuffered data is not relevant.

Ok, if you feel so strong, I will change the assert() to a check that
does nothing.

Later, Juan.

Re: [PATCH 04/16] qemu-file: Don't call qemu_fflush() for read only files

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> This was the only caller for read only files.  So change the test for
> an assert in qemu_fflush().
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Drop this patch.

As Richard has strong opinions about calling fflush() on input streams.
I still think that it makes zero sense to call it on a migration input
stream.

But I don't care enough to fight this battle O:-)

Later, Juan.

Re: [PATCH 10/16] qemu-file: Remove _noflush from qemu_file_transferred_noflush()

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> qemu_file_transferred() don't exist anymore, so we can reuse the name.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/qemu-file.h | 4 ++--
>>  migration/block.c     | 4 ++--
>>  migration/qemu-file.c | 2 +-
>>  migration/savevm.c    | 6 +++---
>>  migration/vmstate.c   | 4 ++--
>>  5 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
>> index b4fb872018..3575dfa5ff 100644
>> --- a/migration/qemu-file.h
>> +++ b/migration/qemu-file.h
>> @@ -34,7 +34,7 @@ QEMUFile *qemu_file_new_output(QIOChannel *ioc);
>>  int qemu_fclose(QEMUFile *f);
>>  
>>  /*
>> - * qemu_file_transferred_noflush:
>> + * qemu_file_transferred:
>>   *
>>   * As qemu_file_transferred except for writable files, where no flush
>
> Docs need updating^

Fixed, thanks.

>>   * is performed and the reported amount will include the size of any
>> @@ -42,7 +42,7 @@ int qemu_fclose(QEMUFile *f);
>>   *
>>   * Returns: the total bytes transferred and queued
>>   */
>> -uint64_t qemu_file_transferred_noflush(QEMUFile *f);
>> +uint64_t qemu_file_transferred(QEMUFile *f);