Re: [PATCH] QIO: Add force_quit to the QIOChannel to ensure QIO exits cleanly in some corner case

[Daniel P . Berrangé]

On Wed, Dec 01, 2021 at 09:48:31AM +0000, Rao, Lei wrote:
> 
> 
> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com> 
> Sent: Wednesday, December 1, 2021 5:11 PM
> To: Rao, Lei <lei.rao@intel.com>
> Cc: Zhang, Chen <chen.zhang@intel.com>; eblake@redhat.com; 
> vsementsov@virtuozzo.com; kwolf@redhat.com; hreitz@redhat.com; 
> qemu-block@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH] QIO: Add force_quit to the QIOChannel to ensure QIO 
> exits cleanly in some corner case
> 
> > 
> > Signed-off-by: Lei Rao <lei.rao@intel.com>
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  block/nbd.c          |  5 +++++
> >  include/io/channel.h | 19 +++++++++++++++++++
> >  io/channel.c         | 22 ++++++++++++++++++++++
> >  3 files changed, 46 insertions(+)
> > 
> > diff --git a/block/nbd.c b/block/nbd.c index 5ef462db1b..5ee4eaaf57 
> > 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -208,6 +208,8 @@ static void nbd_teardown_connection(BlockDriverState 
> > *bs)
> >      assert(!s->in_flight);
> >  
> >      if (s->ioc) {
> > +        qio_channel_set_force_quit(s->ioc, true);
> > +        qio_channel_coroutines_wake(s->ioc);
> >          qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, 
> > NULL);
> 
> Calling shutdown here should already be causing the qio_chanel_readv_all to 
> wakeup and break out of its
> poll() loop. We shouldn't need to add a second way to break out of the poll().
> 
> Calling shutdown can wake up the coroutines which is polling. But I think 
> it's not enough. I tried to forcibly wake up the NBD coroutine,
> It may cause segment fault. The root cause is that it will continue to access 
> ioc->xxx in qio_channel_yield(), but the ioc has been released and set it 
> NULL such as in
> nbd_co_do_establish_connection(); I think call shutdown will have the same 
> result. So, I add the force_quit, once set it true, it will immediately exit 
> without accessing IOC.

That's a bug in the NBD code then. NBD must not be freeing the IO channel
object while it is performing read/write API calls on it.

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