Make sure the data is flushed to disk before closing file
channels. This will ensure data is on disk at the end of a migration
to file.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 io/channel-file.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/io/channel-file.c b/io/channel-file.c
index a6ad7770c6..d4706fa592 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -242,6 +242,11 @@ 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");
-- 
2.35.3

On Mon, Nov 27, 2023 at 05:25:46PM -0300, Fabiano Rosas wrote:
> Make sure the data is flushed to disk before closing file
> channels. This will ensure data is on disk at the end of a migration
> to file.

Looks reasonable, but just two (possibly naive) questions:

(1) Does this apply to all io channel users, or only migration?

(2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Nov 27, 2023 at 05:25:46PM -0300, Fabiano Rosas wrote:
>> Make sure the data is flushed to disk before closing file
>> channels. This will ensure data is on disk at the end of a migration
>> to file.
>
> Looks reasonable, but just two (possibly naive) questions:
>
> (1) Does this apply to all io channel users, or only migration?

All file channel users.

>
> (2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?

Syncing the inode information is not critical, it's mostly timestamp
information (man inode). And fdatasync makes sure to sync any metadata
that would be relevant for the retrieval of the data.

>
> Thanks,

On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > (2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?
> 
> Syncing the inode information is not critical, it's mostly timestamp
> information (man inode). And fdatasync makes sure to sync any metadata
> that would be relevant for the retrieval of the data.

I forgot to reply to this one in the previous reply..

Timestamps look all fine to be old.  What about file size?  That's also in
"man inode" as metadata, but I'm not sure whether data will be fully valid
if e.g. size enlarged but not flushed along with the page caches.

-- 
Peter Xu

On Mon, Jan 15, 2024 at 04:57:42PM +0800, Peter Xu wrote:
> On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > > (2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?
> > 
> > Syncing the inode information is not critical, it's mostly timestamp
> > information (man inode). And fdatasync makes sure to sync any metadata
> > that would be relevant for the retrieval of the data.
> 
> I forgot to reply to this one in the previous reply..
> 
> Timestamps look all fine to be old.  What about file size?  That's also in
> "man inode" as metadata, but I'm not sure whether data will be fully valid
> if e.g. size enlarged but not flushed along with the page caches.

If the size wasn't updated, then syncing of the data would be pointless.
The man page confirms that size is synced:

[quote]
       fdatasync() is similar to fsync(), but does not flush modified metadata
       unless  that metadata is needed in order to allow a subsequent data re‐
       trieval to be correctly handled.  For example, changes to  st_atime  or
       st_mtime  (respectively, time of last access and time of last modifica‐
       tion; see inode(7)) do not require flushing because they are not neces‐
       sary for a subsequent data read to be handled correctly.  On the  other
       hand, a change to the file size (st_size, as made by say ftruncate(2)),
       would require a metadata flush.
[/quote]

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Jan 15, 2024 at 09:03:20AM +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 15, 2024 at 04:57:42PM +0800, Peter Xu wrote:
> > On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > > > (2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?
> > > 
> > > Syncing the inode information is not critical, it's mostly timestamp
> > > information (man inode). And fdatasync makes sure to sync any metadata
> > > that would be relevant for the retrieval of the data.
> > 
> > I forgot to reply to this one in the previous reply..
> > 
> > Timestamps look all fine to be old.  What about file size?  That's also in
> > "man inode" as metadata, but I'm not sure whether data will be fully valid
> > if e.g. size enlarged but not flushed along with the page caches.
> 
> If the size wasn't updated, then syncing of the data would be pointless.
> The man page confirms that size is synced:
> 
> [quote]
>        fdatasync() is similar to fsync(), but does not flush modified metadata
>        unless  that metadata is needed in order to allow a subsequent data re‐
>        trieval to be correctly handled.  For example, changes to  st_atime  or
>        st_mtime  (respectively, time of last access and time of last modifica‐
>        tion; see inode(7)) do not require flushing because they are not neces‐
>        sary for a subsequent data read to be handled correctly.  On the  other
>        hand, a change to the file size (st_size, as made by say ftruncate(2)),
>        would require a metadata flush.
> [/quote]

I should have read more carefully, sorry for the noise.

-- 
Peter Xu

On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > (1) Does this apply to all io channel users, or only migration?
> 
> All file channel users.

I meant the whole idea of flushing on close, on whether there will be
iochannel users that will prefer not do so?  It's a matter of where to put
this best.

-- 
Peter Xu

On Fri, Jan 12, 2024 at 08:01:36AM +0800, Peter Xu wrote:
> On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > > (1) Does this apply to all io channel users, or only migration?
> > 
> > All file channel users.
> 
> I meant the whole idea of flushing on close, on whether there will be
> iochannel users that will prefer not do so?  It's a matter of where to put
> this best.

IMHO, all users of QIOChannelFile will benefit from flushing, to ensure
integrity of their saved file upon host crash.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, Jan 12, 2024 at 10:40:28AM +0000, Daniel P. Berrangé wrote:
> On Fri, Jan 12, 2024 at 08:01:36AM +0800, Peter Xu wrote:
> > On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > > > (1) Does this apply to all io channel users, or only migration?
> > > 
> > > All file channel users.
> > 
> > I meant the whole idea of flushing on close, on whether there will be
> > iochannel users that will prefer not do so?  It's a matter of where to put
> > this best.
> 
> IMHO, all users of QIOChannelFile will benefit from flushing, to ensure
> integrity of their saved file upon host crash.

Thanks.  It might be nice to also mention that is for all purpose then in
the commit message (currently, it only mentions "for migration only"), some
description would be even nicer on why it services all purposes.

For example, I think it applies to all as long as we don't have use case
for frequent open/close of iochannels which may require a fast close(), so
all use cases will hope the sync to happen.

Then we could optionally also document above QIOChannelClass.io_close() on
the implied behavior.

When looking at that header, I noticed we used to have io_flush() for
zerocopy.  I'm wondering whether we should also do that for close() when
zerocopy is enabled.  This may mean that file sockets can also implement
io_flush(), then we call io_flush() in close() if ever existed.

Thanks,

-- 
Peter Xu

On Mon, Nov 27, 2023 at 05:25:46PM -0300, Fabiano Rosas wrote:
> Make sure the data is flushed to disk before closing file
> channels. This will ensure data is on disk at the end of a migration
> to file.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  io/channel-file.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/io/channel-file.c b/io/channel-file.c
> index a6ad7770c6..d4706fa592 100644
> --- a/io/channel-file.c
> +++ b/io/channel-file.c
> @@ -242,6 +242,11 @@ 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");
> -- 
> 2.35.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|