[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature |
Date: |
Wed, 17 Jun 2020 16:09:09 +0100 |
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