Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
On Wed, 17 Jun 2020 16:09:09 +0100
Stefan Hajnoczi 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
pgpaWZt0OZPuh.pgp
Description: OpenPGP digital signature
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
On Tue, 16 Jun 2020 15:44:06 +0100 Daniel P. Berrangé wrote: > On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote: > > Register a yank function which shuts down the socket and sets > > s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an > > error occured. > > > > Signed-off-by: Lukas Straub > > --- > > Makefile.objs | 1 + > > block/nbd.c | 101 -- > > 2 files changed, 65 insertions(+), 37 deletions(-) > > > > diff --git a/Makefile.objs b/Makefile.objs > > index a7c967633a..8e403b81f3 100644 > > --- a/Makefile.objs > > +++ b/Makefile.objs > > @@ -18,6 +18,7 @@ block-obj-y += block.o blockjob.o job.o > > block-obj-y += block/ scsi/ > > block-obj-y += qemu-io-cmds.o > > block-obj-$(CONFIG_REPLICATION) += replication.o > > +block-obj-y += yank.o > > Oh, I see this is repeated for migration and chardev code too. > > Instead of doing this and relying on linker to merge duplicates, > I think we should put yank.c into util/ and built it into util-obj-y, > so it gets added to everything. Ok, I will do this in the next version. Thanks, Lukas Straub > Regards, > Daniel pgpcCKUVeaDXO.pgp Description: OpenPGP digital signature
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
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
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote: > Register a yank function which shuts down the socket and sets > s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an > error occured. > > Signed-off-by: Lukas Straub > --- > Makefile.objs | 1 + > block/nbd.c | 101 -- > 2 files changed, 65 insertions(+), 37 deletions(-) > > diff --git a/Makefile.objs b/Makefile.objs > index a7c967633a..8e403b81f3 100644 > --- a/Makefile.objs > +++ b/Makefile.objs > @@ -18,6 +18,7 @@ block-obj-y += block.o blockjob.o job.o > block-obj-y += block/ scsi/ > block-obj-y += qemu-io-cmds.o > block-obj-$(CONFIG_REPLICATION) += replication.o > +block-obj-y += yank.o Oh, I see this is repeated for migration and chardev code too. Instead of doing this and relying on linker to merge duplicates, I think we should put yank.c into util/ and built it into util-obj-y, so it gets added to everything. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 2/4] block/nbd.c: Add yank feature
On Mon, May 25, 2020 at 05:44:26PM +0200, Lukas Straub wrote: > Register a yank function which shuts down the socket and sets > s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an > error occured. > > Signed-off-by: Lukas Straub > --- > Makefile.objs | 1 + > block/nbd.c | 101 -- > 2 files changed, 65 insertions(+), 37 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH v4 2/4] block/nbd.c: Add yank feature
Register a yank function which shuts down the socket and sets
s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
error occured.
Signed-off-by: Lukas Straub
---
Makefile.objs | 1 +
block/nbd.c | 101 --
2 files changed, 65 insertions(+), 37 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index a7c967633a..8e403b81f3 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -18,6 +18,7 @@ block-obj-y += block.o blockjob.o job.o
block-obj-y += block/ scsi/
block-obj-y += qemu-io-cmds.o
block-obj-$(CONFIG_REPLICATION) += replication.o
+block-obj-y += yank.o
block-obj-m = block/
diff --git a/block/nbd.c b/block/nbd.c
index 2160859f64..3a41749f1b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,6 +35,7 @@
#include "qemu/option.h"
#include "qemu/cutils.h"
#include "qemu/main-loop.h"
+#include "qemu/atomic.h"
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qmp/qstring.h"
@@ -43,6 +44,8 @@
#include "block/nbd.h"
#include "block/block_int.h"
+#include "yank.h"
+
#define EN_OPTSTR ":exportname="
#define MAX_NBD_REQUESTS16
@@ -84,6 +87,8 @@ typedef struct BDRVNBDState {
NBDReply reply;
BlockDriverState *bs;
+char *yank_name;
+
/* Connection parameters */
uint32_t reconnect_delay;
SocketAddress *saddr;
@@ -94,6 +99,7 @@ typedef struct BDRVNBDState {
} BDRVNBDState;
static int nbd_client_connect(BlockDriverState *bs, Error **errp);
+static void nbd_yank(void *opaque);
static void nbd_clear_bdrvstate(BDRVNBDState *s)
{
@@ -106,17 +112,19 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
s->tlscredsid = NULL;
g_free(s->x_dirty_bitmap);
s->x_dirty_bitmap = NULL;
+g_free(s->yank_name);
+s->yank_name = NULL;
}
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
if (ret == -EIO) {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
NBD_CLIENT_CONNECTING_NOWAIT;
}
} else {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
s->state = NBD_CLIENT_QUIT;
@@ -167,7 +175,7 @@ static void nbd_client_attach_aio_context(BlockDriverState
*bs,
* s->connection_co is either yielded from nbd_receive_reply or from
* nbd_co_reconnect_loop()
*/
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
}
@@ -206,7 +214,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
{
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTED) {
/* finish any pending coroutines */
assert(s->ioc);
qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
@@ -230,13 +238,14 @@ static void nbd_teardown_connection(BlockDriverState *bs)
static bool nbd_client_connecting(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT ||
-s->state == NBD_CLIENT_CONNECTING_NOWAIT;
+NBDClientState state = atomic_read(&s->state);
+return state == NBD_CLIENT_CONNECTING_WAIT ||
+state == NBD_CLIENT_CONNECTING_NOWAIT;
}
static bool nbd_client_connecting_wait(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT;
+return atomic_read(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
}
static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
@@ -274,6 +283,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState
*s)
/* Finalize previous connection if any */
if (s->ioc) {
nbd_client_detach_aio_context(s->bs);
+yank_unregister_function(s->yank_name, nbd_yank, s->bs);
object_unref(OBJECT(s->sioc));
s->sioc = NULL;
object_unref(OBJECT(s->ioc));
@@ -305,7 +315,7 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState
*s)
nbd_reconnect_attempt(s);
while (nbd_client_connecting(s)) {
-if (s->state == NBD_CLIENT_CONNECTING_WAIT &&
+if (atomic_read(&s->state) == NBD_CLIENT_CONNECTING_WAIT &&
qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start_time_ns > delay_ns)
{
s->state = NBD_CLIENT_CONNECTING_NOWAIT;
@@ -341,7 +351,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
int ret = 0;
Error *local_err = NULL;
-while (s->state != NBD_CLIENT_QUIT) {
+while (atomic_read(&s->state) != NBD_CLIENT_QUIT) {
/*
* The NBD client can only really be considered idle when it has
* yielded from qio_channel_readv_all_eof(), waiting for data. This is
@@ -356,7 +366,7
