[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 03/18] nbd: Minimal structured read for server
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 03/18] nbd: Minimal structured read for server |
Date: |
Tue, 7 Feb 2017 18:44:30 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
On 07/02/2017 00:01, Eric Blake wrote:
> 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
Wouter's slides from FOSDEM said the state is "discussion complete, not
yet implemented".
Paolo
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>> 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.
>
signature.asc
Description: OpenPGP digital signature