| Message ID | 20230608135653.2918540-16-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On 08.06.23 16:56, Eric Blake wrote: > Although extended mode is not yet enabled, once we do turn it on, we > need to reply with extended headers to all messages. Update the low > level entry points necessary so that all other callers automatically > get the right header based on the current mode. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > v4: new patch, split out from v3 9/14 > --- > nbd/server.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 119ac765f09..84c848a31d3 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, > size_t niov, uint16_t flags, uint16_t type, > NBDRequest *request) > { > - /* TODO - handle structured vs. extended replies */ > - NBDStructuredReplyChunk *chunk = iov->iov_base; > size_t i, length = 0; > > for (i = 1; i < niov; i++) { > @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, > } > assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)); > > - iov[0].iov_len = sizeof(*chunk); > - 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->cookie, request->cookie); > - stl_be_p(&chunk->length, length); > + if (client->mode >= NBD_MODE_EXTENDED) { > + NBDExtendedReplyChunk *chunk = iov->iov_base; > + > + iov->iov_len = sizeof(*chunk); I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC); > + stw_be_p(&chunk->flags, flags); > + stw_be_p(&chunk->type, type); > + stq_be_p(&chunk->cookie, request->cookie); Hm. Not about this patch: we now moved to simple cookies. And it seems that actually, 64bit is too much for number of request. > + stq_be_p(&chunk->offset, request->from); > + stq_be_p(&chunk->length, length); > + } else { > + NBDStructuredReplyChunk *chunk = iov->iov_base; > + > + iov->iov_len = sizeof(*chunk); > + 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->cookie, request->cookie); > + stl_be_p(&chunk->length, length); > + } > } > > static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client, > @@ -2478,6 +2490,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, > { > if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) { > return nbd_co_send_chunk_error(client, request, -ret, error_msg, errp); > + } else if (client->mode >= NBD_MODE_EXTENDED) { > + return nbd_co_send_chunk_done(client, request, errp); > } else { > return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0, > NULL, 0, errp);
On Fri, Jun 16, 2023 at 09:48:18PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 08.06.23 16:56, Eric Blake wrote: > > Although extended mode is not yet enabled, once we do turn it on, we > > need to reply with extended headers to all messages. Update the low > > level entry points necessary so that all other callers automatically > > get the right header based on the current mode. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > > > v4: new patch, split out from v3 9/14 > > --- > > nbd/server.c | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > diff --git a/nbd/server.c b/nbd/server.c > > index 119ac765f09..84c848a31d3 100644 > > --- a/nbd/server.c > > +++ b/nbd/server.c > > @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, > > size_t niov, uint16_t flags, uint16_t type, > > NBDRequest *request) > > { > > - /* TODO - handle structured vs. extended replies */ > > - NBDStructuredReplyChunk *chunk = iov->iov_base; > > size_t i, length = 0; > > > > for (i = 1; i < niov; i++) { > > @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, > > } > > assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)); > > > > - iov[0].iov_len = sizeof(*chunk); > > - 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->cookie, request->cookie); > > - stl_be_p(&chunk->length, length); > > + if (client->mode >= NBD_MODE_EXTENDED) { > > + NBDExtendedReplyChunk *chunk = iov->iov_base; > > + > > + iov->iov_len = sizeof(*chunk); > > I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array > > anyway: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> I can make that change, and keep your R-b. > > > + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC); > > + stw_be_p(&chunk->flags, flags); > > + stw_be_p(&chunk->type, type); > > + stq_be_p(&chunk->cookie, request->cookie); > > Hm. Not about this patch: > > we now moved to simple cookies. And it seems that actually, 64bit is too much for number of request. You're right that it's more than qemu cared about. But there may be other implementations that really do store a 64-bit pointer as their opaque cookie, for ease of reverse-lookup on which command the server's reply corresponds to, so I don't see it changing any time soon in the NBD protocol.
On 04.08.23 22:28, Eric Blake wrote: > On Fri, Jun 16, 2023 at 09:48:18PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 08.06.23 16:56, Eric Blake wrote: >>> Although extended mode is not yet enabled, once we do turn it on, we >>> need to reply with extended headers to all messages. Update the low >>> level entry points necessary so that all other callers automatically >>> get the right header based on the current mode. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> >>> v4: new patch, split out from v3 9/14 >>> --- >>> nbd/server.c | 30 ++++++++++++++++++++++-------- >>> 1 file changed, 22 insertions(+), 8 deletions(-) >>> >>> diff --git a/nbd/server.c b/nbd/server.c >>> index 119ac765f09..84c848a31d3 100644 >>> --- a/nbd/server.c >>> +++ b/nbd/server.c >>> @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, >>> size_t niov, uint16_t flags, uint16_t type, >>> NBDRequest *request) >>> { >>> - /* TODO - handle structured vs. extended replies */ >>> - NBDStructuredReplyChunk *chunk = iov->iov_base; >>> size_t i, length = 0; >>> >>> for (i = 1; i < niov; i++) { >>> @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, >>> } >>> assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)); >>> >>> - iov[0].iov_len = sizeof(*chunk); >>> - 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->cookie, request->cookie); >>> - stl_be_p(&chunk->length, length); >>> + if (client->mode >= NBD_MODE_EXTENDED) { >>> + NBDExtendedReplyChunk *chunk = iov->iov_base; >>> + >>> + iov->iov_len = sizeof(*chunk); >> >> I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array >> >> anyway: >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > I can make that change, and keep your R-b. OK > >> >>> + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC); >>> + stw_be_p(&chunk->flags, flags); >>> + stw_be_p(&chunk->type, type); >>> + stq_be_p(&chunk->cookie, request->cookie); >> >> Hm. Not about this patch: >> >> we now moved to simple cookies. And it seems that actually, 64bit is too much for number of request. > > You're right that it's more than qemu cared about. But there may be > other implementations that really do store a 64-bit pointer as their > opaque cookie, for ease of reverse-lookup on which command the > server's reply corresponds to, so I don't see it changing any time > soon in the NBD protocol. > Agree
diff --git a/nbd/server.c b/nbd/server.c index 119ac765f09..84c848a31d3 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, size_t niov, uint16_t flags, uint16_t type, NBDRequest *request) { - /* TODO - handle structured vs. extended replies */ - NBDStructuredReplyChunk *chunk = iov->iov_base; size_t i, length = 0; for (i = 1; i < niov; i++) { @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client, struct iovec *iov, } assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)); - iov[0].iov_len = sizeof(*chunk); - 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->cookie, request->cookie); - stl_be_p(&chunk->length, length); + if (client->mode >= NBD_MODE_EXTENDED) { + NBDExtendedReplyChunk *chunk = iov->iov_base; + + iov->iov_len = sizeof(*chunk); + stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC); + stw_be_p(&chunk->flags, flags); + stw_be_p(&chunk->type, type); + stq_be_p(&chunk->cookie, request->cookie); + stq_be_p(&chunk->offset, request->from); + stq_be_p(&chunk->length, length); + } else { + NBDStructuredReplyChunk *chunk = iov->iov_base; + + iov->iov_len = sizeof(*chunk); + 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->cookie, request->cookie); + stl_be_p(&chunk->length, length); + } } static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client, @@ -2478,6 +2490,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, { if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) { return nbd_co_send_chunk_error(client, request, -ret, error_msg, errp); + } else if (client->mode >= NBD_MODE_EXTENDED) { + return nbd_co_send_chunk_done(client, request, errp); } else { return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0, NULL, 0, errp);
Although extended mode is not yet enabled, once we do turn it on, we need to reply with extended headers to all messages. Update the low level entry points necessary so that all other callers automatically get the right header based on the current mode. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch, split out from v3 9/14 --- nbd/server.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)