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. > + 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().
signature.asc
Description: PGP signature
