[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown()
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown() |
Date: |
Fri, 23 Sep 2022 08:14:25 +0100 |
User-agent: |
Mutt/2.2.6 (2022-06-05) |
On Thu, Sep 22, 2022 at 03:37:11PM -0400, Peter Xu wrote:
> On Thu, Sep 22, 2022 at 05:58:25PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Sep 20, 2022 at 06:37:57PM -0400, Peter Xu wrote:
> > > In qemu_file_shutdown(), there's a possible race if with current order of
> > > operation. There're two major things to do:
> > >
> > > (1) Do real shutdown() (e.g. shutdown() syscall on socket)
> > > (2) Update qemufile's last_error
> > >
> > > We must do (2) before (1) otherwise there can be a race condition like:
> > >
> > > page receiver other thread
> > > ------------- ------------
> > > qemu_get_buffer()
> > > do shutdown()
> > > returns 0 (buffer all zero)
> > > (meanwhile we didn't check this retcode)
> > > try to detect IO error
> > > last_error==NULL, IO okay
> > > install ALL-ZERO page
> > > set last_error
> > > --> guest crash!
> > >
> > > To fix this, we can also check retval of qemu_get_buffer(), but not all
> > > APIs can be properly checked and ultimately we still need to go back to
> > > qemu_file_get_error(). E.g. qemu_get_byte() doesn't return error.
> > >
> > > Maybe some day a rework of qemufile API is really needed, but for now keep
> > > using qemu_file_get_error() and fix it by not allowing that race condition
> > > to happen. Here shutdown() is indeed special because the last_error was
> > > emulated. For real -EIO errors it'll always be set when e.g. sendmsg()
> > > error triggers so we won't miss those ones, only shutdown() is a bit
> > > tricky
> > > here.
> >
> > The ultimate answer really is to stop using QEMUFile entirely and just
> > do migration with the QIOChannel objects directly. The work I did in the
> > last cycle to remove the QEMUFileOps callbacks was another piece of the
> > puzzle in moving in that direction, by ensuring that every QEMUFile is
> > now backed by a QIOChannel. All QEMUFile is doing now is adding the I/O
> > caching layer and some convenience APIs for I/O operations.
> >
> > So the next step would be to add a QIOChannelCached class that can wrap
> > over another QIOChannel, to add the I/O buffering functionality that
> > currently exists in QEMUFile. Once that's done, it'd be at the stage
> > where we could look at how to use QIOChannel APIs for VMstate. It would
> > likely involve wiring up an Error **errp parameter into a great many
> > places so we get synchronous error propagation instead of out-of-band
> > error checking, so a phased transition would need to be figured out.
>
> Yes, Error** sounds very good to have.
>
> So far I'm not satisfied with qemufile api majorly because of that error
> handling, especially on *get() interfaces.
>
> Besides that, do you have anything else in mind that would make
> QIOChannelCached better than qemufile (e.g. on how we do caching)?
Depends what you mean by better ? I think the caching code would be
a bit easier to understand, because QEMUFile gets a bit confusing
about which logic is used for read side and which is used for the
write side.
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 :|
- [PATCH 4/5] migration: Disallow postcopy preempt to be used with compress, (continued)
[PATCH 1/5] migration: Fix possible deadloop of ram save process, Peter Xu, 2022/09/20