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

[Lukas Straub]

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.

I prefer option 3 :)

The subclass can live in yank.c. There'd have to be 2 yank functions again but 
a comment in yank_nbd should suffice.

Regards,
Lukas Straub

pgpNwv7KB4do9.pgp
Description: OpenPGP digital signature