Re: [PATCH v3 09/14] nbd/server: Initial support for extended headers
On Wed, May 31, 2023 at 05:46:55PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
> > Time to support clients that request extended headers. Now we can
> > finally reach the code added across several previous patches.
> >
> > Even though the NBD spec has been altered to allow us to accept
> > NBD_CMD_READ larger than the max payload size (provided our response
> > is a hole or broken up over more than one data chunk), we are not
> > planning to take advantage of that, and continue to cap NBD_CMD_READ
> > to 32M regardless of header size.
> >
> > For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already
> > supports 64-bit operations without any effort on our part. For
> > NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous
> > patch took care of implementing the required
> > NBD_REPLY_TYPE_BLOCK_STATUS_EXT.
> >
> > Signed-off-by: Eric Blake
> > ---
> > nbd/nbd-internal.h | 5 +-
>
> [..]
>
> >
> > static inline void set_be_simple_reply(NBDClient *client, struct iovec
> > *iov,
> > - uint64_t error, NBDRequest *request)
> > + uint32_t error, NBDStructuredError
> > *err,
> > + NBDRequest *request)
> > {
> > -NBDSimpleReply *reply = iov->iov_base;
> > +if (client->header_style >= NBD_HEADER_EXTENDED) {
> > +NBDExtendedReplyChunk *chunk = iov->iov_base;
> >
> > -iov->iov_len = sizeof(*reply);
> > -stl_be_p(>magic, NBD_SIMPLE_REPLY_MAGIC);
> > -stl_be_p(>error, error);
> > -stq_be_p(>handle, request->handle);
> > +iov->iov_len = sizeof(*chunk);
> > +stl_be_p(>magic, NBD_EXTENDED_REPLY_MAGIC);
> > +stw_be_p(>flags, NBD_REPLY_FLAG_DONE);
> > +stq_be_p(>handle, request->handle);
> > +stq_be_p(>offset, request->from);
> > +if (error) {
> > +assert(!iov[1].iov_base);
> > +iov[1].iov_base = err;
> > +iov[1].iov_len = sizeof(*err);
> > +stw_be_p(>type, NBD_REPLY_TYPE_ERROR);
> > +stq_be_p(>length, sizeof(*err));
> > +stl_be_p(>error, error);
> > +stw_be_p(>message_length, 0);
> > +} else {
> > +stw_be_p(>type, NBD_REPLY_TYPE_NONE);
> > +stq_be_p(>length, 0);
> > +}
> > +} else {
> > +NBDSimpleReply *reply = iov->iov_base;
> > +
> > +iov->iov_len = sizeof(*reply);
> > +stl_be_p(>magic, NBD_SIMPLE_REPLY_MAGIC);
> > +stl_be_p(>error, error);
> > +stq_be_p(>handle, request->handle);
> > +}
> > }
> >
> > static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client,
> > @@ -1906,30 +1966,44 @@ static int coroutine_fn
> > nbd_co_send_simple_reply(NBDClient *client,
>
> So, that's not _simple_ now.. The function should be renamed. As well as
> set_be_simple_reply(). _simple_or_extended_ ? a bit too long. But continuing
> to use "simple" is in bad relation with use of "simple" word in specification.
In fact, I added an assertion that set_be_simple_reply() can only be
reached when extended replies are not in use, so none of this
complexity here was needed after all.
>
> Probably better to update callers? The only caller isi
> nbd_send_generic_reply(). So, could we just add
> nbd_co_send_single_extended_reply() to call from nbd_send_generic_reply() in
> case of EXTENDED?
>
> Also, transformation of set_be_simple_reply() do look like it should be two
> separate functions.
>
> > {
> > NBDReply hdr;
> > int nbd_err = system_errno_to_nbd_errno(error);
> > +NBDStructuredError err;
> > struct iovec iov[] = {
> > {.iov_base = },
> > {.iov_base = data, .iov_len = len}
> > };
> >
> > +assert(!len || !nbd_err);
> > trace_nbd_co_send_simple_reply(request->handle, nbd_err,
> > nbd_err_lookup(nbd_err), len);
> > -set_be_simple_reply(client, [0], nbd_err, request);
> > +set_be_simple_reply(client, [0], nbd_err, , request);
> >
> > -return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
> > +return nbd_co_send_iov(client, iov, iov[1].iov_len ? 2 : 1, errp);
Not introduced in this patch, but it turns out that when
iov[1].iov_len == 0, blindly passing niov==2 to nbd_co_send_iov()
still does the right thing, so I can lose the conditional here.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 09/14] nbd/server: Initial support for extended headers
On 15.05.23 22:53, Eric Blake wrote:
Time to support clients that request extended headers. Now we can
finally reach the code added across several previous patches.
Even though the NBD spec has been altered to allow us to accept
NBD_CMD_READ larger than the max payload size (provided our response
is a hole or broken up over more than one data chunk), we are not
planning to take advantage of that, and continue to cap NBD_CMD_READ
to 32M regardless of header size.
For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already
supports 64-bit operations without any effort on our part. For
NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous
patch took care of implementing the required
NBD_REPLY_TYPE_BLOCK_STATUS_EXT.
Signed-off-by: Eric Blake
---
nbd/nbd-internal.h | 5 +-
[..]
static inline void set_be_simple_reply(NBDClient *client, struct iovec *iov,
- uint64_t error, NBDRequest *request)
+ uint32_t error, NBDStructuredError *err,
+ NBDRequest *request)
{
-NBDSimpleReply *reply = iov->iov_base;
+if (client->header_style >= NBD_HEADER_EXTENDED) {
+NBDExtendedReplyChunk *chunk = iov->iov_base;
-iov->iov_len = sizeof(*reply);
-stl_be_p(>magic, NBD_SIMPLE_REPLY_MAGIC);
-stl_be_p(>error, error);
-stq_be_p(>handle, request->handle);
+iov->iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_EXTENDED_REPLY_MAGIC);
+stw_be_p(>flags, NBD_REPLY_FLAG_DONE);
+stq_be_p(>handle, request->handle);
+stq_be_p(>offset, request->from);
+if (error) {
+assert(!iov[1].iov_base);
+iov[1].iov_base = err;
+iov[1].iov_len = sizeof(*err);
+stw_be_p(>type, NBD_REPLY_TYPE_ERROR);
+stq_be_p(>length, sizeof(*err));
+stl_be_p(>error, error);
+stw_be_p(>message_length, 0);
+} else {
+stw_be_p(>type, NBD_REPLY_TYPE_NONE);
+stq_be_p(>length, 0);
+}
+} else {
+NBDSimpleReply *reply = iov->iov_base;
+
+iov->iov_len = sizeof(*reply);
+stl_be_p(>magic, NBD_SIMPLE_REPLY_MAGIC);
+stl_be_p(>error, error);
+stq_be_p(>handle, request->handle);
+}
}
static int coroutine_fn nbd_co_send_simple_reply(NBDClient *client,
@@ -1906,30 +1966,44 @@ static int coroutine_fn
nbd_co_send_simple_reply(NBDClient *client,
So, that's not _simple_ now.. The function should be renamed. As well as set_be_simple_reply().
_simple_or_extended_ ? a bit too long. But continuing to use "simple" is in bad relation
with use of "simple" word in specification.
Probably better to update callers? The only caller isi
nbd_send_generic_reply(). So, could we just add
nbd_co_send_single_extended_reply() to call from nbd_send_generic_reply() in
case of EXTENDED?
Also, transformation of set_be_simple_reply() do look like it should be two
separate functions.
{
NBDReply hdr;
int nbd_err = system_errno_to_nbd_errno(error);
+NBDStructuredError err;
struct iovec iov[] = {
{.iov_base = },
{.iov_base = data, .iov_len = len}
};
+assert(!len || !nbd_err);
trace_nbd_co_send_simple_reply(request->handle, nbd_err,
nbd_err_lookup(nbd_err), len);
-set_be_simple_reply(client, [0], nbd_err, request);
+set_be_simple_reply(client, [0], nbd_err, , request);
-return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
+return nbd_co_send_iov(client, iov, iov[1].iov_len ? 2 : 1, errp);
}
static inline void set_be_chunk(NBDClient *client, struct iovec *iov,
uint16_t flags, uint16_t type,
NBDRequest *request, uint32_t length)
{
-NBDStructuredReplyChunk *chunk = iov->iov_base;
+if (client->header_style >= NBD_HEADER_EXTENDED) {
+NBDExtendedReplyChunk *chunk = iov->iov_base;
-iov->iov_len = sizeof(*chunk);
-stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
-stw_be_p(>flags, flags);
-stw_be_p(>type, type);
-stq_be_p(>handle, request->handle);
-stl_be_p(>length, length);
+iov->iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_EXTENDED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>handle, request->handle);
+stq_be_p(>offset, request->from);
+stq_be_p(>length, length);
+} else {
+NBDStructuredReplyChunk *chunk = iov->iov_base;
+
+iov->iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>handle, request->handle);
+stl_be_p(>length, length);
+}
}
static int coroutine_fn nbd_co_send_structured_done(NBDClient *client,
--
Best regards,
Vladimir
