Re: [Qemu-devel] [PATCH] migration: Cleanup during exit

[Peter Xu]

On Thu, Feb 28, 2019 at 12:28:22PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (address@hidden) wrote:
> > On Thu, Feb 28, 2019 at 11:40:19AM +0000, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (address@hidden) wrote:
> > > > On Wed, Feb 27, 2019 at 04:49:00PM +0000, Dr. David Alan Gilbert (git) 
> > > > wrote:
> > > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > > > 
> > > > > Currently we cleanup the migration object as we exit main after the
> > > > > main_loop finishes; however if there's a migration running things
> > > > > get messy and we can end up with the migration thread still trying
> > > > > to access freed structures.
> > > > > 
> > > > > We now take a ref to the object around the migration thread itself,
> > > > > so the act of dropping the ref during exit doesn't cause us to lose
> > > > > the state until the thread quits.
> > > > > 
> > > > > Cancelling the migration during migration also tries to get the thread
> > > > > to quit.
> > > > > 
> > > > > We do this a bit earlier; so hopefully migration gets out of the way
> > > > > before all the devices etc are freed.
> > > > 
> > > > So does it mean that even with the patch it's still possible the
> > > > migration thread will be accessing device structs that have already
> > > > been freed which can still crash QEMU?
> > > 
> > > Possibly yes; I'm not sure how to go to the next stage and stop that
> > > case; the consensus seems to be we don't want to explicitly block
> > > during the exit process, so doing a join on the migration thread doesn't
> > > seem to be wanted.
> > 
> > Essentially the many  *_cleanup() calls at the end of main() in vl.c
> > are only ever safe if all background threads have stopped using the
> > resources that are being freed. This isn't the case with migration
> > currently. I also worry about other threads that might be running
> > in QEMU, SPICE in particular as it touchs many device backends.
> > 
> > Aborting the migration & joining the migration threads is the natural
> > way to address this. Cancelling appears to require the main loop to
> > still be running, so would require main_loop_should_exit() to issue
> > the cancel & return false unless it has completed. This would delay
> > shutdown for as long as it takes migration to abort.
> 
> ish - cancelling performs a shutdown(2) on the fd, that should be enough
> in most cases to kick the migration thread into falling out without
> main loop interaction; I think...

Dave, could you hint me on when shutdown() will not suffice (say,
we'll hang death if we do join() upon the migration thread)?

> 
> > FWIW, even the bdrv_close_all() call during shutdown can delay things
> > for a considerable time if draining the backends isn't a quick op,
> > which could be the case if there are storage problems (blocked local
> > I/O, or interrupted network - rbd/ceph/nfs clients). So I'm not sure
> > that stopping migration is inherantly worse than what's already
> > possible with block cleanup, in terms of delaying things.
> > 
> > A slightly more hacky approach would be to pthread_cancel() all the
> > migration threads. Normally we'd never use pthread_cancel() as it
> > is incredibly hard to ensure all memory used by the threads is
> > freed. Since we're about to exit the process though, this cleanup
> > does not matter. The risk, however, is that one of the threads is
> > holding a mutex which then blocks the rest of the *cleanup functions,
> > so this still feels dangerous.
> > 
> > Overall to me a clean cancel & join of the migration threads feels
> > like the only safe option, unless we just remove all notion of
> > cleanup from QEMU & delete all the _cleanup functions in main()
> > and let the OS free all memory.
> 
> That's actually my preference; I think trying to do clean tidy ups
> here is very racy and doesn't gain much.  However, for things like
> storage there may be locks that have to be properly dropped and
> bitmaps and things to be stored/sync'd - so just exiting probably
> isn't safe either.

I'm unsure about whether I caught the whole idea but I feel like we
can't drop all the cleanup hooks since what if we want to do something
else than "freeing memories"?  Or anything that the OS can't do for us
but we want to try to do before the process quits.  If that operation
hangs we'll probably face a similar hang issue.

Regarding pthread_cancel() - it will only work if the thread reaches
cancellation points, right?  Then does it mean that it still cannot
guarantee the thread will quit very soon and we still have chance that
we'll wait forever when join()?  One major reason that the thread will
be waiting should be the migration streams but AFAIU shutdown() (as
Dave has mentioned) should solve this problem properly, then I'm
unsuer how pthread_cancel() can help much comparing to shutdown()...

My understanding (probably not correct, but I'll just speak loud...)
is that we will never have a way to guarantee a process to quit
cleanly all the time because there're really many things that can hang
(I'm assuming we've already solved the MigrationState refcounting
issue by this patch, so I'm assuming we're having a "hang QEMU" rather
than crashing issue).  Then IMHO we could simply do whatever we can do
to cleanup everything assuming no hang will happen, and if it really
happens we use SIGKILL which will be the last thing we can do by which
we might lost many things (e.g., unflushed caches in block layer) but
we've tried our best.  For migration, it'll be (1) cancel (not
pthread_cancel, but cancel the migration to trigger shutdown() on fds
or whatever) (2) join thread (3) finalize/cleanup data structures.

IMHO SIGKILL is the real de-facto solution for all these issues to me.
And even if with SIGKILL we can still hang.... but I'll assume those
hangs will be kernel problems not QEMU since SIGKILL should be
designed to kill a process without a question.

Thanks,

-- 
Peter Xu