Commit bc38feddeb ("io: fsync before closing a file channel") added a
fsync/fdatasync at the closing point of the QIOChannelFile to ensure
integrity of the migration stream in case of QEMU crash.

The decision to do the sync at qio_channel_close() was not the best
since that function runs in the main thread and the fsync can cause
QEMU to hang for several minutes, depending on the migration size and
disk speed.

To fix the hang, remove the fsync from qio_channel_file_close().

At this moment, the migration code is the only user of the fsync and
we're taking the tradeoff of not having a sync at all, leaving the
responsibility to the upper layers.

Fixes: bc38feddeb ("io: fsync before closing a file channel")
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
v2:
- kept the qio_channel_close
---
 docs/devel/migration/main.rst |  3 ++-
 io/channel-file.c             |  5 -----
 migration/multifd.c           | 14 ++++----------
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 8024275d6d..54385a23e5 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -44,7 +44,8 @@ over any transport.
 - file migration: do the migration using a file that is passed to QEMU
   by path. A file offset option is supported to allow a management
   application to add its own metadata to the start of the file without
-  QEMU interference.
+  QEMU interference. Note that QEMU does not flush cached file
+  data/metadata at the end of migration.
 
 In addition, support is included for migration using RDMA, which
 transports the page data using ``RDMA``, where the hardware takes care of
diff --git a/io/channel-file.c b/io/channel-file.c
index d4706fa592..a6ad7770c6 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -242,11 +242,6 @@ static int qio_channel_file_close(QIOChannel *ioc,
 {
     QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc);
 
-    if (qemu_fdatasync(fioc->fd) < 0) {
-        error_setg_errno(errp, errno,
-                         "Unable to synchronize file data with storage device");
-        return -1;
-    }
     if (qemu_close(fioc->fd) < 0) {
         error_setg_errno(errp, errno,
                          "Unable to close file");
diff --git a/migration/multifd.c b/migration/multifd.c
index d4a44da559..0a8fef046b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -710,16 +710,10 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
     if (p->c) {
         migration_ioc_unregister_yank(p->c);
         /*
-         * An explicit close() on the channel here is normally not
-         * required, but can be helpful for "file:" iochannels, where it
-         * will include fdatasync() to make sure the data is flushed to the
-         * disk backend.
-         *
-         * The object_unref() cannot guarantee that because: (1) finalize()
-         * of the iochannel is only triggered on the last reference, and
-         * it's not guaranteed that we always hold the last refcount when
-         * reaching here, and, (2) even if finalize() is invoked, it only
-         * does a close(fd) without data flush.
+         * The object_unref() cannot guarantee the fd will always be
+         * released because finalize() of the iochannel is only
+         * triggered on the last reference and it's not guaranteed
+         * that we always hold the last refcount when reaching here.
          */
         qio_channel_close(p->c, &error_abort);
         object_unref(OBJECT(p->c));
-- 
2.35.3

05.03.2024 22:56, Fabiano Rosas :
> Commit bc38feddeb ("io: fsync before closing a file channel") added a

> Fixes: bc38feddeb ("io: fsync before closing a file channel")

FWIW, this is commit c05dfcb7f2 in master, not bc38feddeb.
It's too late to change that now though.

/mjt

On Tue, Mar 05, 2024 at 04:56:29PM -0300, Fabiano Rosas wrote:
> Commit bc38feddeb ("io: fsync before closing a file channel") added a
> fsync/fdatasync at the closing point of the QIOChannelFile to ensure
> integrity of the migration stream in case of QEMU crash.
> 
> The decision to do the sync at qio_channel_close() was not the best
> since that function runs in the main thread and the fsync can cause
> QEMU to hang for several minutes, depending on the migration size and
> disk speed.
> 
> To fix the hang, remove the fsync from qio_channel_file_close().
> 
> At this moment, the migration code is the only user of the fsync and
> we're taking the tradeoff of not having a sync at all, leaving the
> responsibility to the upper layers.
> 
> Fixes: bc38feddeb ("io: fsync before closing a file channel")
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Since 9.0 is reaching and it's important we avoid such hang, I queued this
version.

However to make sure we can still remember why we do this after a few
years, I added a rich comment and will squash into this patch:

=======

diff --git a/migration/multifd.c b/migration/multifd.c
index 0a8fef046b..bf9d483f7a 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -714,6 +714,22 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
          * released because finalize() of the iochannel is only
          * triggered on the last reference and it's not guaranteed
          * that we always hold the last refcount when reaching here.
+         *
+         * Closing the fd explicitly has the benefit that if there is any
+         * registered I/O handler callbacks on such fd, that will get a
+         * POLLNVAL event and will further trigger the cleanup to finally
+         * release the IOC.
+         *
+         * FIXME: It should logically be guaranteed that all multifd
+         * channels have no I/O handler callback registered when reaching
+         * here, because migration thread will wait for all multifd channel
+         * establishments to complete during setup.  Since
+         * migrate_fd_cleanup() will be scheduled in main thread too, all
+         * previous callbacks should guarantee to be completed when
+         * reaching here.  See multifd_send_state.channels_created and its
+         * usage.  In the future, we could replace this with an assert
+         * making sure we're the last reference, or simply drop it if above
+         * is more clear to be justified.
          */
         qio_channel_close(p->c, &error_abort);
         object_unref(OBJECT(p->c));

========

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 05, 2024 at 04:56:29PM -0300, Fabiano Rosas wrote:
>> Commit bc38feddeb ("io: fsync before closing a file channel") added a
>> fsync/fdatasync at the closing point of the QIOChannelFile to ensure
>> integrity of the migration stream in case of QEMU crash.
>> 
>> The decision to do the sync at qio_channel_close() was not the best
>> since that function runs in the main thread and the fsync can cause
>> QEMU to hang for several minutes, depending on the migration size and
>> disk speed.
>> 
>> To fix the hang, remove the fsync from qio_channel_file_close().
>> 
>> At this moment, the migration code is the only user of the fsync and
>> we're taking the tradeoff of not having a sync at all, leaving the
>> responsibility to the upper layers.
>> 
>> Fixes: bc38feddeb ("io: fsync before closing a file channel")
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Since 9.0 is reaching and it's important we avoid such hang, I queued this
> version.
>
> However to make sure we can still remember why we do this after a few
> years, I added a rich comment and will squash into this patch:
>
> =======
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0a8fef046b..bf9d483f7a 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -714,6 +714,22 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>           * released because finalize() of the iochannel is only
>           * triggered on the last reference and it's not guaranteed
>           * that we always hold the last refcount when reaching here.
> +         *
> +         * Closing the fd explicitly has the benefit that if there is any
> +         * registered I/O handler callbacks on such fd, that will get a
> +         * POLLNVAL event and will further trigger the cleanup to finally
> +         * release the IOC.
> +         *
> +         * FIXME: It should logically be guaranteed that all multifd
> +         * channels have no I/O handler callback registered when reaching
> +         * here, because migration thread will wait for all multifd channel
> +         * establishments to complete during setup.  Since
> +         * migrate_fd_cleanup() will be scheduled in main thread too, all
> +         * previous callbacks should guarantee to be completed when
> +         * reaching here.  See multifd_send_state.channels_created and its
> +         * usage.  In the future, we could replace this with an assert
> +         * making sure we're the last reference, or simply drop it if above
> +         * is more clear to be justified.
>           */
>          qio_channel_close(p->c, &error_abort);
>          object_unref(OBJECT(p->c));
>
> ========

Ack. Thanks Peter!