Re: [PATCH v4 2/4] block/nbd.c: Add yank feature

[Lukas Straub]

On Wed, 17 Jun 2020 16:09:09 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote:
> > @@ -1395,6 +1407,15 @@ static int nbd_client_reopen_prepare(BDRVReopenState 
> > *state,
> >      return 0;
> >  }
> > 
> > +static void nbd_yank(void *opaque)
> > +{
> > +    BlockDriverState *bs = opaque;
> > +    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > +
> > +    qio_channel_shutdown(QIO_CHANNEL(s->sioc), QIO_CHANNEL_SHUTDOWN_BOTH, 
> > NULL);  
> 
> qio_channel_shutdown() is not guaranteed to be thread-safe. Please
> document new assumptions that are being introduced.
> 
> Today we can more or less get away with it (although TLS sockets are a
> little iffy) because it boils down the a shutdown(2) system call. I
> think it would be okay to update the qio_channel_shutdown() and
> .io_shutdown() documentation to clarify that this is thread-safe.

Good idea, especially since the migration code already assumes this. I will do 
this in the next version.

> > +    atomic_set(&s->state, NBD_CLIENT_QUIT);  
> 
> docs/devel/atomics.rst says:
> 
>   No barriers are implied by ``atomic_read`` and ``atomic_set`` in either 
> Linux
>   or QEMU.
> 
> Other threads might not see the latest value of s->state because this is
> a weakly ordered memory access.
> 
> I haven't audited the NBD code in detail, but if you want the other
> threads to always see NBD_CLIENT_QUIT then s->state should be set before
> calling qio_channel_shutdown() using a stronger atomics API like
> atomic_load_acquire()/atomic_store_release().

You are right, I will change that in the next version.

Thanks,
Lukas Straub

pgptDW3wdksfa.pgp
Description: OpenPGP digital signature