diff mbox series

[v3,19/33] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake()

Message ID 20210416080911.83197-20-vsementsov@virtuozzo.com
State New
Headers show
Series block/nbd: rework client connection | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:08 a.m. UTC 
To be reused in the following patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 42 deletions(-)

Comments

Roman Kagan May 12, 2021, 8:40 a.m. UTC | #1 
On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be reused in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 57 insertions(+), 42 deletions(-)

Reviewed-by: Roman Kagan <rvkagan@yandex-team.ru>
Eric Blake June 3, 2021, 4:29 p.m. UTC | #2 
On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be reused in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 57 insertions(+), 42 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5e63caaf4b..03ffe95231 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -318,6 +318,50 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
>      return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
>  }
>  
> +/*
> + * Check s->info updated by negotiation process.

The parameter name is bs, not s; so this comment is a bit confusing...

> + * Update @bs correspondingly to new options.
> + */
> +static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

...until here.  Maybe rewrite the entire comment as:

Update @bs with information learned during a completed negotiation
process.  Return failure if the server's advertised options are
incompatible with the client's needs.

> +    int ret;
> +
> +    if (s->x_dirty_bitmap) {
> +        if (!s->info.base_allocation) {
> +            error_setg(errp, "requested x-dirty-bitmap %s not found",
> +                       s->x_dirty_bitmap);
> +            return -EINVAL;
> +        }
> +        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
> +            s->alloc_depth = true;
> +        }
> +    }
> +
> +    if (s->info.flags & NBD_FLAG_READ_ONLY) {
> +        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    if (s->info.flags & NBD_FLAG_SEND_FUA) {
> +        bs->supported_write_flags = BDRV_REQ_FUA;
> +        bs->supported_zero_flags |= BDRV_REQ_FUA;

Code motion, so it is correct, but it looks odd to use = for one
assignment and |= for the other.  Using |= in both places would be
more consistent.

> +    }
> +
> +    if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
> +        bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
> +        if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
> +            bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
> +        }
> +    }
> +
> +    trace_nbd_client_handshake_success(s->export);
> +
> +    return 0;
> +}
> +
>  static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>  {
>      int ret;
> @@ -1579,49 +1623,13 @@ static int nbd_client_handshake(BlockDriverState *bs, Error **errp)

As updating the comment doesn't affect code correctness,
Reviewed-by: Eric Blake <eblake@redhat.com>
Vladimir Sementsov-Ogievskiy June 9, 2021, 5:23 p.m. UTC | #3 
03.06.2021 19:29, Eric Blake wrote:
> On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> To be reused in the following patch.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
>>   1 file changed, 57 insertions(+), 42 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 5e63caaf4b..03ffe95231 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -318,6 +318,50 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
>>       return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
>>   }
>>   
>> +/*
>> + * Check s->info updated by negotiation process.
> 
> The parameter name is bs, not s; so this comment is a bit confusing...
> 
>> + * Update @bs correspondingly to new options.
>> + */
>> +static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
>> +{
>> +    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> 
> ...until here.  Maybe rewrite the entire comment as:
> 
> Update @bs with information learned during a completed negotiation
> process.  Return failure if the server's advertised options are
> incompatible with the client's needs.
> 
>> +    int ret;
>> +
>> +    if (s->x_dirty_bitmap) {
>> +        if (!s->info.base_allocation) {
>> +            error_setg(errp, "requested x-dirty-bitmap %s not found",
>> +                       s->x_dirty_bitmap);
>> +            return -EINVAL;
>> +        }
>> +        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
>> +            s->alloc_depth = true;
>> +        }
>> +    }
>> +
>> +    if (s->info.flags & NBD_FLAG_READ_ONLY) {
>> +        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    if (s->info.flags & NBD_FLAG_SEND_FUA) {
>> +        bs->supported_write_flags = BDRV_REQ_FUA;
>> +        bs->supported_zero_flags |= BDRV_REQ_FUA;
> 
> Code motion, so it is correct, but it looks odd to use = for one
> assignment and |= for the other.  Using |= in both places would be
> more consistent.

Actually I see bugs here:

1. we should do =, not |=, as on reconnect info changes, so we should reset supported flags.

2. in-fligth requests that are in retying loops are not prepared to flags changing. I afraid, that some malicious server may even do some bad thing

Still, let's fix it after these series. To avoid more conflicts.

> 
>> +    }
>> +
>> +    if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
>> +        bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
>> +        if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
>> +            bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
>> +        }
>> +    }
>> +
>> +    trace_nbd_client_handshake_success(s->export);
>> +
>> +    return 0;
>> +}
>> +
>>   static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>>   {
>>       int ret;
>> @@ -1579,49 +1623,13 @@ static int nbd_client_handshake(BlockDriverState *bs, Error **errp)
> 
> As updating the comment doesn't affect code correctness,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Eric Blake June 9, 2021, 6:28 p.m. UTC | #4 
On Wed, Jun 09, 2021 at 08:23:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > +    if (s->x_dirty_bitmap) {
> > > +        if (!s->info.base_allocation) {
> > > +            error_setg(errp, "requested x-dirty-bitmap %s not found",
> > > +                       s->x_dirty_bitmap);
> > > +            return -EINVAL;
> > > +        }
> > > +        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
> > > +            s->alloc_depth = true;
> > > +        }
> > > +    }
> > > +
> > > +    if (s->info.flags & NBD_FLAG_READ_ONLY) {
> > > +        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
> > > +        if (ret < 0) {
> > > +            return ret;
> > > +        }
> > > +    }
> > > +
> > > +    if (s->info.flags & NBD_FLAG_SEND_FUA) {
> > > +        bs->supported_write_flags = BDRV_REQ_FUA;
> > > +        bs->supported_zero_flags |= BDRV_REQ_FUA;
> > 
> > Code motion, so it is correct, but it looks odd to use = for one
> > assignment and |= for the other.  Using |= in both places would be
> > more consistent.
> 
> Actually I see bugs here:
> 
> 1. we should do =, not |=, as on reconnect info changes, so we should reset supported flags.
> 
> 2. in-fligth requests that are in retying loops are not prepared to flags changing. I afraid, that some malicious server may even do some bad thing
> 
> Still, let's fix it after these series. To avoid more conflicts.

Oh, you raise some good points.  And it's not just bs->*flags; qemu as
server uses constant metacontext ids (base:allocation is always
context 0), but even that might not be stable across reconnect.  For
example, with my proposed patch of adding qemu:joint-allocation
metacontext, if the reason we have to reconnect is because the server
is upgrading from qemu 6.0 to 6.1 temporarily bouncing the server, and
the client was paying attention to qemu:dirty-bitmap:FOO, that context
would now have a different id.

Yeah, making this code safer across potential changes in server
information (either to fail the reconnect because the reconnected
server dropped something we were previously depending on, or
gracefully handling the downgrade, or ...) is worth leaving for a
later series while we focus on the more immediate issue of making
reconnect itself stable.
diff mbox series

Patch

diff --git a/block/nbd.c b/block/nbd.c
index 5e63caaf4b..03ffe95231 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -318,6 +318,50 @@  static bool nbd_client_connecting_wait(BDRVNBDState *s)
     return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
 }
 
+/*
+ * Check s->info updated by negotiation process.
+ * Update @bs correspondingly to new options.
+ */
+static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
+{
+    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+    int ret;
+
+    if (s->x_dirty_bitmap) {
+        if (!s->info.base_allocation) {
+            error_setg(errp, "requested x-dirty-bitmap %s not found",
+                       s->x_dirty_bitmap);
+            return -EINVAL;
+        }
+        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
+            s->alloc_depth = true;
+        }
+    }
+
+    if (s->info.flags & NBD_FLAG_READ_ONLY) {
+        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if (s->info.flags & NBD_FLAG_SEND_FUA) {
+        bs->supported_write_flags = BDRV_REQ_FUA;
+        bs->supported_zero_flags |= BDRV_REQ_FUA;
+    }
+
+    if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
+        bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
+        if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
+            bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
+        }
+    }
+
+    trace_nbd_client_handshake_success(s->export);
+
+    return 0;
+}
+
 static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
 {
     int ret;
@@ -1579,49 +1623,13 @@  static int nbd_client_handshake(BlockDriverState *bs, Error **errp)
         s->sioc = NULL;
         return ret;
     }
-    if (s->x_dirty_bitmap) {
-        if (!s->info.base_allocation) {
-            error_setg(errp, "requested x-dirty-bitmap %s not found",
-                       s->x_dirty_bitmap);
-            ret = -EINVAL;
-            goto fail;
-        }
-        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
-            s->alloc_depth = true;
-        }
-    }
-    if (s->info.flags & NBD_FLAG_READ_ONLY) {
-        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
-        if (ret < 0) {
-            goto fail;
-        }
-    }
-    if (s->info.flags & NBD_FLAG_SEND_FUA) {
-        bs->supported_write_flags = BDRV_REQ_FUA;
-        bs->supported_zero_flags |= BDRV_REQ_FUA;
-    }
-    if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
-        bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
-        if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
-            bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
-        }
-    }
 
-    if (!s->ioc) {
-        s->ioc = QIO_CHANNEL(s->sioc);
-        object_ref(OBJECT(s->ioc));
-    }
-
-    trace_nbd_client_handshake_success(s->export);
-
-    return 0;
-
- fail:
-    /*
-     * We have connected, but must fail for other reasons.
-     * Send NBD_CMD_DISC as a courtesy to the server.
-     */
-    {
+    ret = nbd_handle_updated_info(bs, errp);
+    if (ret < 0) {
+        /*
+         * We have connected, but must fail for other reasons.
+         * Send NBD_CMD_DISC as a courtesy to the server.
+         */
         NBDRequest request = { .type = NBD_CMD_DISC };
 
         nbd_send_request(s->ioc ?: QIO_CHANNEL(s->sioc), &request);
@@ -1635,6 +1643,13 @@  static int nbd_client_handshake(BlockDriverState *bs, Error **errp)
 
         return ret;
     }
+
+    if (!s->ioc) {
+        s->ioc = QIO_CHANNEL(s->sioc);
+        object_ref(OBJECT(s->ioc));
+    }
+
+    return 0;
 }
 
 /*