Re: [PATCH v11 2/7] block/nbd.c: Add yank feature

[Eric Blake]

On 11/15/20 5:36 AM, 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.

occurred

> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/nbd.c | 154 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 93 insertions(+), 61 deletions(-)
> 

> @@ -166,12 +168,12 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
>  static void nbd_channel_error(BDRVNBDState *s, int ret)
>  {
>      if (ret == -EIO) {
> -        if (s->state == NBD_CLIENT_CONNECTED) {
> +        if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {

This may have interesting interactions with Vladimir's latest patches to
make NBD connection re-startable, but we'll sort that out as needed.
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg07012.html

The patch seems big; I might have broken it into two pieces (conversion
of existing logic to use qatomic_*() accesses instead of direct s->state
manipulation, and then adding new logic).  But I'm not going to hold up
the series demanding for a split at this time.


> @@ -424,12 +427,12 @@ static void *connect_thread_func(void *opaque)
>      return NULL;
>  }
> 
> -static QIOChannelSocket *coroutine_fn
> +static int coroutine_fn
>  nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
>  {
> +    int ret;
>      QemuThread thread;
>      BDRVNBDState *s = bs->opaque;
> -    QIOChannelSocket *res;
>      NBDConnectThread *thr = s->connect_thread;
> 
>      qemu_mutex_lock(&thr->mutex);
> @@ -446,10 +449,12 @@ nbd_co_establish_connection(BlockDriverState *bs, Error 
> **errp)
>      case CONNECT_THREAD_SUCCESS:
>          /* Previous attempt finally succeeded in background */
>          thr->state = CONNECT_THREAD_NONE;
> -        res = thr->sioc;
> +        s->sioc = thr->sioc;
>          thr->sioc = NULL;
> +        yank_register_function(BLOCKDEV_YANK_INSTANCE(bs->node_name),
> +                               nbd_yank, bs);
>          qemu_mutex_unlock(&thr->mutex);
> -        return res;
> +        return 0;

Looks sensible.

> @@ -1745,6 +1759,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;
> +
> +    qatomic_store_release(&s->state, NBD_CLIENT_QUIT);
> +    qio_channel_shutdown(QIO_CHANNEL(s->sioc), QIO_CHANNEL_SHUTDOWN_BOTH, 
> NULL);
> +}
> +

Yep, that does indeed tell qemu to give up on the NBD socket right away.

Reviewed-by: Eric Blake <eblake@redhat.com>

And sorry it's taken me so long to actually stare at this series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org