Re: [PATCH v2 28/29] migration: Add direct-io parameter

[Daniel P . Berrangé]

On Wed, Oct 25, 2023 at 02:30:01PM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Oct 25, 2023 at 11:32:00AM -0300, Fabiano Rosas wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Tue, Oct 24, 2023 at 04:32:10PM -0300, Fabiano Rosas wrote:
> >> >> Markus Armbruster <armbru@redhat.com> writes:
> >> >> 
> >> >> > Fabiano Rosas <farosas@suse.de> writes:
> >> >> >
> >> >> >> Add the direct-io migration parameter that tells the migration code 
> >> >> >> to
> >> >> >> use O_DIRECT when opening the migration stream file whenever 
> >> >> >> possible.
> >> >> >>
> >> >> >> This is currently only used for the secondary channels of fixed-ram
> >> >> >> migration, which can guarantee that writes are page aligned.
> >> >> >>
> >> >> >> However the parameter could be made to affect other types of
> >> >> >> file-based migrations in the future.
> >> >> >>
> >> >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> >> >
> >> >> > When would you want to enable @direct-io, and when would you want to
> >> >> > leave it disabled?
> >> >> 
> >> >> That depends on a performance analysis. You'd generally leave it
> >> >> disabled unless there's some indication that the operating system is
> >> >> having trouble draining the page cache.
> >> >
> >> > That's not the usage model I would suggest.
> >> >
> >> 
> >> Hehe I took a shot at answering but I really wanted to say "ask Daniel".
> >> 
> >> > The biggest value of the page cache comes when it holds data that
> >> > will be repeatedly accessed.
> >> >
> >> > When you are saving/restoring a guest to file, that data is used
> >> > once only (assuming there's a large gap between save & restore).
> >> > By using the page cache to save a big guest we essentially purge
> >> > the page cache of most of its existing data that is likely to be
> >> > reaccessed, to fill it up with data never to be reaccessed.
> >> >
> >> > I usually describe save/restore operations as trashing the page
> >> > cache.
> >> >
> >> > IMHO, mgmt apps should request O_DIRECT always unless they expect
> >> > the save/restore operation to run in quick succession, or if they
> >> > know that the host has oodles of free RAM such that existing data
> >> > in the page cache won't be trashed, or
> >> 
> >> Thanks, I'll try to incorporate this to some kind of doc in the next
> >> version.
> >> 
> >> > if the host FS does not support O_DIRECT of course.
> >> 
> >> Should we try to probe for this and inform the user?
> >
> > qemu_open_internall will already do a nice error message. If it gets
> > EINVAL when using O_DIRECT, it'll retry without O_DIRECT and if that
> > works, it'll reoprt "filesystem does not support O_DIRECT"
> >
> > Having said that I see a problem with /dev/fdset handling, because
> > we're only validating O_ACCMODE and that excludes O_DIRECT.
> >
> > If the mgmt apps passes an FD with O_DIRECT already set, then it
> > won't work for VMstate saving which is unaligned.
> >
> > If the mgmt app passes an FD without O_DIRECT set, then we are
> > not setting O_DIRECT for the multifd RAM threads.
> 
> Worse, the fds get dup'ed so even without O_DIRECT, we we enable it for
> the secondary channels the main channel will break on unaligned writes.
> 
> For now I can only think of requiring two fds. One for the main channel
> and a second one for the rest of the channels. And validating the fd
> flags to make sure O_DIRECT is only allowed to be set in the second fd.

In this new model I think there's no reason for libvirt to set O_DIRECT
for its own initial I/O. So we could just totally ignore O_DIRECT when
initially opening the QIOCHannelFile.

Then provide a method on QIOCHannelFile to enable O_DIRECT on the fly
which can be called for the multifd threads setup ?

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 :|