Re: [Qemu-devel] [PATCH 03/18] nbd: Minimal structured read for server

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6868 bytes --]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation of structured read: one data chunk + finishing
> none chunk. No segmentation.
> Minimal structured error implementation: no text message.
> Support DF flag, but just ignore it, as there is no segmentation any
> way.

Might be worth adding that this is still an experimental extension to
the NBD spec, and therefore that this implementation serves as proof of
concept and may still need tweaking if anything major turns up before
promoting it to stable.  It might also be worth a link to:

https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h |  31 +++++++++++++
>  nbd/nbd-internal.h  |   2 +
>  nbd/server.c        | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 3c65cf8d87..58b864f145 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -70,6 +70,25 @@ struct NBDSimpleReply {
>  };
>  typedef struct NBDSimpleReply NBDSimpleReply;
>  
> +typedef struct NBDStructuredReplyChunk {
> +    uint32_t magic;
> +    uint16_t flags;
> +    uint16_t type;
> +    uint64_t handle;
> +    uint32_t length;
> +} QEMU_PACKED NBDStructuredReplyChunk;
> +
> +typedef struct NBDStructuredRead {
> +    NBDStructuredReplyChunk h;
> +    uint64_t offset;
> +} QEMU_PACKED NBDStructuredRead;
> +
> +typedef struct NBDStructuredError {
> +    NBDStructuredReplyChunk h;
> +    uint32_t error;
> +    uint16_t message_length;
> +} QEMU_PACKED NBDStructuredError;

Definitely a subset of all types added in the NBD protocol extension,
but reasonable for a minimal implementation.  Might be worth adding
comments to the types...

>  
> +/* Structured reply flags */
> +#define NBD_REPLY_FLAG_DONE 1
> +
> +/* Structured reply types */
> +#define NBD_REPLY_TYPE_NONE 0
> +#define NBD_REPLY_TYPE_OFFSET_DATA 1
> +#define NBD_REPLY_TYPE_OFFSET_HOLE 2
> +#define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
> +#define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)

...that correspond to these constants that will be used in the [h.]type
field.

Also, it's a bit odd that you are defining constants that aren't
implemented here; I don't know if it is any cleaner to save the
definition for the unimplemented types until you actually implement them
(NBD_REPLY_TYPE_OFFSET_HOLE, NBD_REPLY_TYPE_ERROR_OFFSET).

> +++ b/nbd/nbd-internal.h
> @@ -60,6 +60,7 @@
>  #define NBD_REPLY_SIZE          (4 + 4 + 8)
>  #define NBD_REQUEST_MAGIC       0x25609513
>  #define NBD_SIMPLE_REPLY_MAGIC  0x67446698
> +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef
>  #define NBD_OPTS_MAGIC          0x49484156454F5054LL
>  #define NBD_CLIENT_MAGIC        0x0000420281861253LL
>  #define NBD_REP_MAGIC           0x0003e889045565a9LL

I would not be bothered if you wanted to reindent the other lines by 3
spaces so that all the macro definitions start on the same column.  But
I also won't require it.

> @@ -81,6 +82,7 @@
>  #define NBD_OPT_LIST            (3)
>  #define NBD_OPT_PEEK_EXPORT     (4)
>  #define NBD_OPT_STARTTLS        (5)
> +#define NBD_OPT_STRUCTURED_REPLY (8)

Similar comments about consistency in the definition column.

> +++ b/nbd/server.c
> @@ -100,6 +100,8 @@ struct NBDClient {
>      QTAILQ_ENTRY(NBDClient) next;
>      int nb_requests;
>      bool closing;
> +
> +    bool structured_reply;
>  };
>  
>  /* That's all folks */
> @@ -573,6 +575,16 @@ static int nbd_negotiate_options(NBDClient *client)
>                      return ret;
>                  }
>                  break;
> +
> +            case NBD_OPT_STRUCTURED_REPLY:
> +                client->structured_reply = true;
> +                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> +                                             clientflags);
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +                break;
> +

As written, you allow the client to negotiate this more than once.  On
the one hand, we are idempotent, so it doesn't hurt if they do so; on
the other hand, it is a waste of bandwidth, and a client could abuse it
by sending an infinite stream of NBD_OPT_STRUCTURED_REPLY requests and
never moving into transmission phase, which is a mild form of
Denial-of-Service (they're hogging a socket from accomplishing useful
work for some other client).  It would be acceptable if we wanted to
disconnect any client that sends this option more than once, although
the NBD spec does not require us to do so.  Up to you if you think
that's worth adding.

>  
> +static void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags,
> +                         uint16_t type, uint64_t handle, uint32_t length)

I'm not sure I like the name of this helper. I know what you are doing:
go from native-endian local variables into network-byte-order storage in
preparation for transmission, done at the last possible moment.  But I
also don't know if I have a good suggestion for a better name off hand.

> +{
> +    stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC);
> +    stw_be_p(&chunk->flags, flags);
> +    stw_be_p(&chunk->type, type);
> +    stq_be_p(&chunk->handle, handle);
> +    stl_be_p(&chunk->length, length);
> +}
> +
> +static int nbd_co_send_iov(NBDClient *client, struct iovec *iov, unsigned niov)

Probably should add the coroutine_fn annotation to this function and its
friends (yeah, the NBD code doesn't consistently use it yet, but it should).

> @@ -1147,7 +1239,8 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
>          rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
>          goto out;
>      }
> -    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
> +    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE |
> +                           NBD_CMD_FLAG_DF)) {
>          LOG("unsupported flags (got 0x%x)", request->flags);
>          rc = -EINVAL;
>          goto out;

Missing a check that NBD_CMD_FLAG_DF is only set for NBD_CMD_READ (it is
not valid on any other command, at least in the current version of the
extension specification).

> @@ -1444,6 +1559,8 @@ void nbd_client_new(NBDExport *exp,
>      client->can_read = true;
>      client->close = close_fn;
>  
> +    client->structured_reply = false;

Dead assignment, since we used 'client = g_malloc0()' above.

Overall looks like it matches the spec.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]