On Wed, 17 Jun 2020 16:09:09 +0100 Stefan Hajnoczi <stefa...@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
pgpnek0Wr004q.pgp
Description: OpenPGP digital signature
