| Message ID | 20171012095319.136610-8-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd minimal structured read | expand |
On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: > Send qiov via qio_channel_writev_all instead of calling nbd_write twice > with a cork. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/server.c | 50 ++++++++++++++++++++++++-------------------------- > 1 file changed, 24 insertions(+), 26 deletions(-) > > @@ -1203,36 +1220,17 @@ static int nbd_co_send_simple_reply(NBDClient *client, > size_t len, > Error **errp) > { > - NBDSimpleReply simple_reply; > - int ret; > - > - g_assert(qemu_in_coroutine()); > + NBDSimpleReply reply; Why the rename from simple_reply to reply? > + struct iovec iov[] = { > + {.iov_base = &reply, .iov_len = sizeof(reply)}, I guess it made this line shorter. But we could reduce some churn by naming it 'reply' in the first place, back in earlier patches. At the same time, I'm not going to bother if there's not a reason to respin the series (or at least the first half). > + {.iov_base = data, .iov_len = len} > + }; > > trace_nbd_co_send_simple_reply(handle, error, len); > > - set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(error), > - handle); > - > - qemu_co_mutex_lock(&client->send_lock); > - client->send_coroutine = qemu_coroutine_self(); > - > - if (!len) { > - ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); > - } else { > - qio_channel_set_cork(client->ioc, true); > - ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); > - if (ret == 0) { > - ret = nbd_write(client->ioc, data, len, errp); > - if (ret < 0) { > - ret = -EIO; > - } > - } > - qio_channel_set_cork(client->ioc, false); > - } > + set_be_simple_reply(&reply, system_errno_to_nbd_errno(error), handle); > > - client->send_coroutine = NULL; > - qemu_co_mutex_unlock(&client->send_lock); > - return ret; > + return nbd_co_send_iov(client, iov, len ? 2 : 1, errp); This part is definitely nicer! Thanks for splitting the v2 patch, it made review a lot more pleasant. Reviewed-by: Eric Blake <eblake@redhat.com>
On 10/12/2017 05:27 PM, Eric Blake wrote: > On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: >> Send qiov via qio_channel_writev_all instead of calling nbd_write twice >> with a cork. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> nbd/server.c | 50 ++++++++++++++++++++++++-------------------------- >> 1 file changed, 24 insertions(+), 26 deletions(-) >> Subject-line consistency: elsewhere you used nbd/server instead of nbd-server. I'll make the obvious tweak.
On 10/12/2017 05:27 PM, Eric Blake wrote: > On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote: >> Send qiov via qio_channel_writev_all instead of calling nbd_write twice >> with a cork. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> nbd/server.c | 50 ++++++++++++++++++++++++-------------------------- >> 1 file changed, 24 insertions(+), 26 deletions(-) >> > >> @@ -1203,36 +1220,17 @@ static int nbd_co_send_simple_reply(NBDClient *client, >> size_t len, >> Error **errp) >> { >> - NBDSimpleReply simple_reply; >> - int ret; >> - >> - g_assert(qemu_in_coroutine()); >> + NBDSimpleReply reply; > > Why the rename from simple_reply to reply? > >> + struct iovec iov[] = { >> + {.iov_base = &reply, .iov_len = sizeof(reply)}, > > I guess it made this line shorter. But we could reduce some churn by > naming it 'reply' in the first place, back in earlier patches. At the > same time, I'm not going to bother if there's not a reason to respin the > series (or at least the first half). Answering myself - you couldn't use the name 'reply' until 5/13 removed it as a parameter name, even though you introduced the name 'simple_reply' in 4/13. Okay, the rename is fine here.
diff --git a/nbd/server.c b/nbd/server.c index afc127bbd9..c1bbe8d2d1 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1188,6 +1188,23 @@ void nbd_export_close_all(void) } } +static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec *iov, + unsigned niov, Error **errp) +{ + int ret; + + g_assert(qemu_in_coroutine()); + qemu_co_mutex_lock(&client->send_lock); + client->send_coroutine = qemu_coroutine_self(); + + ret = qio_channel_writev_all(client->ioc, iov, niov, errp) < 0 ? -EIO : 0; + + client->send_coroutine = NULL; + qemu_co_mutex_unlock(&client->send_lock); + + return ret; +} + static inline void set_be_simple_reply(NBDSimpleReply *reply, uint64_t error, uint64_t handle) { @@ -1203,36 +1220,17 @@ static int nbd_co_send_simple_reply(NBDClient *client, size_t len, Error **errp) { - NBDSimpleReply simple_reply; - int ret; - - g_assert(qemu_in_coroutine()); + NBDSimpleReply reply; + struct iovec iov[] = { + {.iov_base = &reply, .iov_len = sizeof(reply)}, + {.iov_base = data, .iov_len = len} + }; trace_nbd_co_send_simple_reply(handle, error, len); - set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(error), - handle); - - qemu_co_mutex_lock(&client->send_lock); - client->send_coroutine = qemu_coroutine_self(); - - if (!len) { - ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); - } else { - qio_channel_set_cork(client->ioc, true); - ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL); - if (ret == 0) { - ret = nbd_write(client->ioc, data, len, errp); - if (ret < 0) { - ret = -EIO; - } - } - qio_channel_set_cork(client->ioc, false); - } + set_be_simple_reply(&reply, system_errno_to_nbd_errno(error), handle); - client->send_coroutine = NULL; - qemu_co_mutex_unlock(&client->send_lock); - return ret; + return nbd_co_send_iov(client, iov, len ? 2 : 1, errp); } /* nbd_co_receive_request
Send qiov via qio_channel_writev_all instead of calling nbd_write twice with a cork. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- nbd/server.c | 50 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 26 deletions(-)