Re: [PATCH 3/5] block/nbd.c: Add yank feature

[Daniel P . Berrangé]

On Fri, May 15, 2020 at 03:03:30PM +0200, Lukas Straub wrote:
> On Fri, 15 May 2020 11:26:13 +0100
> Daniel P. Berrangé <address@hidden> wrote:
> 
> > On Fri, May 15, 2020 at 12:14:47PM +0200, Lukas Straub wrote:
> > > On Fri, 15 May 2020 11:04:13 +0100
> > > Daniel P. Berrangé <address@hidden> wrote:
> > >   
> > > > On Fri, May 15, 2020 at 11:48:18AM +0200, Lukas Straub wrote:  
> > > > > On Tue, 12 May 2020 09:54:58 +0100
> > > > > Daniel P. Berrangé <address@hidden> wrote:
> > > > >     
> > > > > > On Mon, May 11, 2020 at 07:05:24PM +0200, Lukas Straub wrote:    
> > > > > > > On Mon, 11 May 2020 17:19:09 +0100
> > > > > > > "Dr. David Alan Gilbert" <address@hidden> wrote:
> > > > > > >       
> > > > > > > > * Lukas Straub (address@hidden) wrote:      
> > > > > > > > > Add yank option, pass it to the socket-channel and register a 
> > > > > > > > > yank
> > > > > > > > > function which sets s->state = NBD_CLIENT_QUIT. This is the 
> > > > > > > > > same
> > > > > > > > > behaviour as if an error occured.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Lukas Straub <address@hidden>        
> > > > > > > >       
> > > > > > > > > +static void nbd_yank(void *opaque)
> > > > > > > > > +{
> > > > > > > > > +    BlockDriverState *bs = opaque;
> > > > > > > > > +    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > > > > > > > > +
> > > > > > > > > +    atomic_set(&s->state, NBD_CLIENT_QUIT);        
> > > > > > > > 
> > > > > > > > I think I was expecting a shutdown on the socket here - why 
> > > > > > > > doesn't it
> > > > > > > > have one?      
> > > > > > > 
> > > > > > > For nbd, we register two yank functions: This one and we enable
> > > > > > > the yank feature on the qio channel (see function
> > > > > > > nbd_establish_connection below).      
> > > > > > 
> > > > > > As mentioned on the earlier patch, I don't want to see any yank
> > > > > > code in the QIOChannel object directly. This nbd_yank function
> > > > > > can simply call the qio_channel_shutdown() function directly
> > > > > > and avoid need for modifying the QIOChannel object with yank
> > > > > > support.    
> > > > > 
> > > > > Hi,
> > > > > Looking at it again, the problem is not with registering the yank 
> > > > > functions, but with tracking the lifetime of it. Suppose we add 
> > > > > qio_channel_shutdown to the yank_nbd function. Then we need to 
> > > > > unregister it whenever the QIOChannel object is freed.
> > > > > 
> > > > > In the code that would lead to the following constructs in a lot of 
> > > > > places:
> > > > >      if (local_err) {
> > > > >          yank_unregister_function(s->yank_name, yank_nbd, bs);
> > > > >          object_unref(OBJECT(sioc));
> > > > >          error_propagate(errp, local_err);
> > > > >          return NULL;
> > > > >      }    
> > > > 
> > > > The nbd patch here already has a yank_unregister_function() so I'm
> > > > not seeing anything changes in that respect. The "yank_nbd" function
> > > > should check that the I/O channel is non-NULL before calling the
> > > > qio_channel_shutdown method.  
> > > 
> > > Hmm, but if object_unref frees the object, it doesn't set the
> > > pointer to NULL does it?  
> > 
> > So set  "ioc = NULL" after calling object_unref. AFAICT, nbd already
> > does exactly this.
> 
> I see 3 options to do that in a thread-safe manner:
> 1. Introduce a mutex here.
> 2. Use atomics to check for NULL and increase the reference count at the same 
> time in yank_nbd so it isn't freed while calling qio_channel_shutdown on it. 
> (I'm unsure how to do that)
> 3. Do it like it is currently done (but with the new subclass). We get thread 
> safety for free trough the mutex in yank.c.

Oh, so the problem is that  the yank function can be invoked concurrently
with the object being unreffed.

In normal object finalizers, we just have to order things such that
yank_unregister_function() is called before object_unref(ioc) is
called.

The NBD code is slightly harder because we can close & re-open the
IO separately from the finalizer. eg in nbd_reconnect_attempt we
have

    if (s->ioc) {
        nbd_client_detach_aio_context(s->bs);
        object_unref(OBJECT(s->sioc));
        s->sioc = NULL;
        object_unref(OBJECT(s->ioc));
        s->ioc = NULL;
    }

    s->connect_status = nbd_client_connect(s->bs, &local_err);

If the io channel is not open, then we don't need a yank function
registered. So this code would changed to


    if (s->ioc) {
        nbd_client_detach_aio_context(s->bs);
        yank_unregister_function(...);
        object_unref(OBJECT(s->sioc));
        s->sioc = NULL;
        object_unref(OBJECT(s->ioc));
        s->ioc = NULL;
    }

    s->connect_status = nbd_client_connect(s->bs, &local_err);

The locking in yank_unregister_function() should ensure that any
currently executing yank callback has completed before the
yank_unregister_function() call returns. Thus it should be safe
to unref the cannel.

nbd_client_connect() will call yank_register_function() once it
has successfully started a new connection, which your patch already
handles IIUC.

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