* [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring @ 2018-05-07 15:44 Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 1/5] block/nbd-client: split channel errors from export errors Vladimir Sementsov-Ogievskiy ` (5 more replies) 0 siblings, 6 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den Hi all! Here are some preliminary refactoring patches, before NBD reconnect series. Vladimir Sementsov-Ogievskiy (5): block/nbd-client: split channel errors from export errors block/nbd: move connection code from block/nbd to block/nbd-client block/nbd-client: split connection from initialization block/nbd-client: fix nbd_reply_chunk_iter_receive block/nbd-client: don't check ioc block/nbd-client.h | 2 +- block/nbd-client.c | 163 ++++++++++++++++++++++++++++++++++------------------- block/nbd.c | 41 +------------- 3 files changed, 107 insertions(+), 99 deletions(-) -- 2.11.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/5] block/nbd-client: split channel errors from export errors 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 ` Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 2/5] block/nbd: move connection code from block/nbd to block/nbd-client Vladimir Sementsov-Ogievskiy ` (4 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den To implement nbd reconnect in further patches, we need to distinguish error codes, returned by nbd server, from channel errors, to reconnect only in the latter case. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.c | 83 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index 8d69eaaa32..9b9a82fef1 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -501,11 +501,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( */ static coroutine_fn int nbd_co_receive_one_chunk( NBDClientSession *s, uint64_t handle, bool only_structured, - QEMUIOVector *qiov, NBDReply *reply, void **payload, Error **errp) + int *request_ret, QEMUIOVector *qiov, NBDReply *reply, void **payload, + Error **errp) { - int request_ret; int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured, - &request_ret, qiov, payload, errp); + request_ret, qiov, payload, errp); if (ret < 0) { s->quit = true; @@ -515,7 +515,6 @@ static coroutine_fn int nbd_co_receive_one_chunk( *reply = s->reply; } s->reply.handle = 0; - ret = request_ret; } if (s->read_reply_co) { @@ -527,22 +526,17 @@ static coroutine_fn int nbd_co_receive_one_chunk( typedef struct NBDReplyChunkIter { int ret; - bool fatal; + int request_ret; Error *err; bool done, only_structured; } NBDReplyChunkIter; -static void nbd_iter_error(NBDReplyChunkIter *iter, bool fatal, - int ret, Error **local_err) +static void nbd_iter_channel_error(NBDReplyChunkIter *iter, + int ret, Error **local_err) { assert(ret < 0); - if ((fatal && !iter->fatal) || iter->ret == 0) { - if (iter->ret != 0) { - error_free(iter->err); - iter->err = NULL; - } - iter->fatal = fatal; + if (!iter->ret) { iter->ret = ret; error_propagate(&iter->err, *local_err); } else { @@ -552,6 +546,15 @@ static void nbd_iter_error(NBDReplyChunkIter *iter, bool fatal, *local_err = NULL; } +static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret) +{ + assert(ret < 0); + + if (!iter->request_ret) { + iter->request_ret = ret; + } +} + /* NBD_FOREACH_REPLY_CHUNK */ #define NBD_FOREACH_REPLY_CHUNK(s, iter, handle, structured, \ @@ -567,13 +570,13 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s, QEMUIOVector *qiov, NBDReply *reply, void **payload) { - int ret; + int ret, request_ret; NBDReply local_reply; NBDStructuredReplyChunk *chunk; Error *local_err = NULL; if (s->quit) { error_setg(&local_err, "Connection closed"); - nbd_iter_error(iter, true, -EIO, &local_err); + nbd_iter_channel_error(iter, -EIO, &local_err); goto break_loop; } @@ -587,10 +590,12 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s, } ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured, - qiov, reply, payload, &local_err); + &request_ret, qiov, reply, payload, + &local_err); if (ret < 0) { - /* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */ - nbd_iter_error(iter, s->quit, ret, &local_err); + nbd_iter_channel_error(iter, ret, &local_err); + } else if (request_ret < 0) { + nbd_iter_request_error(iter, request_ret); } /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */ @@ -627,7 +632,7 @@ break_loop: } static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle, - Error **errp) + int *request_ret, Error **errp) { NBDReplyChunkIter iter; @@ -636,12 +641,13 @@ static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle, } error_propagate(errp, iter.err); + *request_ret = iter.request_ret; return iter.ret; } static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle, uint64_t offset, QEMUIOVector *qiov, - Error **errp) + int *request_ret, Error **errp) { NBDReplyChunkIter iter; NBDReply reply; @@ -666,7 +672,7 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle, offset, qiov, &local_err); if (ret < 0) { s->quit = true; - nbd_iter_error(&iter, true, ret, &local_err); + nbd_iter_channel_error(&iter, ret, &local_err); } break; default: @@ -676,7 +682,7 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle, error_setg(&local_err, "Unexpected reply type: %d (%s) for CMD_READ", chunk->type, nbd_reply_type_lookup(chunk->type)); - nbd_iter_error(&iter, true, -EINVAL, &local_err); + nbd_iter_channel_error(&iter, -EINVAL, &local_err); } } @@ -685,12 +691,14 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle, } error_propagate(errp, iter.err); + *request_ret = iter.request_ret; return iter.ret; } static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, uint64_t handle, uint64_t length, - NBDExtent *extent, Error **errp) + NBDExtent *extent, + int *request_ret, Error **errp) { NBDReplyChunkIter iter; NBDReply reply; @@ -712,7 +720,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, if (received) { s->quit = true; error_setg(&local_err, "Several BLOCK_STATUS chunks in reply"); - nbd_iter_error(&iter, true, -EINVAL, &local_err); + nbd_iter_channel_error(&iter, -EINVAL, &local_err); } received = true; @@ -721,7 +729,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, &local_err); if (ret < 0) { s->quit = true; - nbd_iter_error(&iter, true, ret, &local_err); + nbd_iter_channel_error(&iter, ret, &local_err); } break; default: @@ -731,7 +739,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, "Unexpected reply type: %d (%s) " "for CMD_BLOCK_STATUS", chunk->type, nbd_reply_type_lookup(chunk->type)); - nbd_iter_error(&iter, true, -EINVAL, &local_err); + nbd_iter_channel_error(&iter, -EINVAL, &local_err); } } @@ -746,14 +754,16 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, iter.ret = -EIO; } } + error_propagate(errp, iter.err); + *request_ret = iter.request_ret; return iter.ret; } static int nbd_co_request(BlockDriverState *bs, NBDRequest *request, QEMUIOVector *write_qiov) { - int ret; + int ret, request_ret; Error *local_err = NULL; NBDClientSession *client = nbd_get_client_session(bs); @@ -769,17 +779,18 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request, return ret; } - ret = nbd_co_receive_return_code(client, request->handle, &local_err); + ret = nbd_co_receive_return_code(client, request->handle, + &request_ret, &local_err); if (local_err) { error_report_err(local_err); } - return ret; + return ret ? ret : request_ret; } int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { - int ret; + int ret, request_ret; Error *local_err = NULL; NBDClientSession *client = nbd_get_client_session(bs); NBDRequest request = { @@ -800,11 +811,11 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset, } ret = nbd_co_receive_cmdread_reply(client, request.handle, offset, qiov, - &local_err); + &request_ret, &local_err); if (local_err) { error_report_err(local_err); } - return ret; + return ret ? ret : request_ret; } int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset, @@ -898,7 +909,7 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs, int64_t *pnum, int64_t *map, BlockDriverState **file) { - int64_t ret; + int ret, request_ret; NBDExtent extent = { 0 }; NBDClientSession *client = nbd_get_client_session(bs); Error *local_err = NULL; @@ -923,12 +934,12 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs, } ret = nbd_co_receive_blockstatus_reply(client, request.handle, bytes, - &extent, &local_err); + &extent, &request_ret, &local_err); if (local_err) { error_report_err(local_err); } - if (ret < 0) { - return ret; + if (ret < 0 || request_ret < 0) { + return ret ? ret : request_ret; } assert(extent.length); -- 2.11.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/5] block/nbd: move connection code from block/nbd to block/nbd-client 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 1/5] block/nbd-client: split channel errors from export errors Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 ` Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 3/5] block/nbd-client: split connection from initialization Vladimir Sementsov-Ogievskiy ` (3 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den Keep all connection code in one file, to be able to implement reconnect in further patches. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.h | 2 +- block/nbd-client.c | 37 +++++++++++++++++++++++++++++++++++-- block/nbd.c | 41 ++--------------------------------------- 3 files changed, 38 insertions(+), 42 deletions(-) diff --git a/block/nbd-client.h b/block/nbd-client.h index 0ece76e5af..a93f2114b9 100644 --- a/block/nbd-client.h +++ b/block/nbd-client.h @@ -41,7 +41,7 @@ typedef struct NBDClientSession { NBDClientSession *nbd_get_client_session(BlockDriverState *bs); int nbd_client_init(BlockDriverState *bs, - QIOChannelSocket *sock, + SocketAddress *saddr, const char *export_name, QCryptoTLSCreds *tlscreds, const char *hostname, diff --git a/block/nbd-client.c b/block/nbd-client.c index 9b9a82fef1..6ff505c4b8 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -976,8 +976,31 @@ void nbd_client_close(BlockDriverState *bs) nbd_teardown_connection(bs); } +static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, + Error **errp) +{ + QIOChannelSocket *sioc; + Error *local_err = NULL; + + sioc = qio_channel_socket_new(); + qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client"); + + qio_channel_socket_connect_sync(sioc, + saddr, + &local_err); + if (local_err) { + object_unref(OBJECT(sioc)); + error_propagate(errp, local_err); + return NULL; + } + + qio_channel_set_delay(QIO_CHANNEL(sioc), false); + + return sioc; +} + int nbd_client_init(BlockDriverState *bs, - QIOChannelSocket *sioc, + SocketAddress *saddr, const char *export, QCryptoTLSCreds *tlscreds, const char *hostname, @@ -986,6 +1009,15 @@ int nbd_client_init(BlockDriverState *bs, NBDClientSession *client = nbd_get_client_session(bs); int ret; + /* establish TCP connection, return error if it fails + * TODO: Configurable retry-until-timeout behaviour. + */ + QIOChannelSocket *sioc = nbd_establish_connection(saddr, errp); + + if (!sioc) { + return -ECONNREFUSED; + } + /* NBD handshake */ logout("session init %s\n", export); qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL); @@ -998,12 +1030,14 @@ int nbd_client_init(BlockDriverState *bs, &client->ioc, &client->info, errp); if (ret < 0) { logout("Failed to negotiate with the NBD server\n"); + object_unref(OBJECT(sioc)); return ret; } if (client->info.flags & NBD_FLAG_READ_ONLY && !bdrv_is_read_only(bs)) { error_setg(errp, "request for write access conflicts with read-only export"); + object_unref(OBJECT(sioc)); return -EACCES; } if (client->info.flags & NBD_FLAG_SEND_FUA) { @@ -1017,7 +1051,6 @@ int nbd_client_init(BlockDriverState *bs, qemu_co_mutex_init(&client->send_mutex); qemu_co_queue_init(&client->free_sema); client->sioc = sioc; - object_ref(OBJECT(client->sioc)); if (!client->ioc) { client->ioc = QIO_CHANNEL(sioc); diff --git a/block/nbd.c b/block/nbd.c index 3e1693cc55..5d9faea18f 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -305,30 +305,6 @@ NBDClientSession *nbd_get_client_session(BlockDriverState *bs) return &s->client; } -static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, - Error **errp) -{ - QIOChannelSocket *sioc; - Error *local_err = NULL; - - sioc = qio_channel_socket_new(); - qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client"); - - qio_channel_socket_connect_sync(sioc, - saddr, - &local_err); - if (local_err) { - object_unref(OBJECT(sioc)); - error_propagate(errp, local_err); - return NULL; - } - - qio_channel_set_delay(QIO_CHANNEL(sioc), false); - - return sioc; -} - - static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) { Object *obj; @@ -398,7 +374,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, BDRVNBDState *s = bs->opaque; QemuOpts *opts = NULL; Error *local_err = NULL; - QIOChannelSocket *sioc = NULL; QCryptoTLSCreds *tlscreds = NULL; const char *hostname = NULL; int ret = -EINVAL; @@ -438,22 +413,10 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, hostname = s->saddr->u.inet.host; } - /* establish TCP connection, return error if it fails - * TODO: Configurable retry-until-timeout behaviour. - */ - sioc = nbd_establish_connection(s->saddr, errp); - if (!sioc) { - ret = -ECONNREFUSED; - goto error; - } - /* NBD handshake */ - ret = nbd_client_init(bs, sioc, s->export, - tlscreds, hostname, errp); + ret = nbd_client_init(bs, s->saddr, s->export, tlscreds, hostname, errp); + error: - if (sioc) { - object_unref(OBJECT(sioc)); - } if (tlscreds) { object_unref(OBJECT(tlscreds)); } -- 2.11.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/5] block/nbd-client: split connection from initialization 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 1/5] block/nbd-client: split channel errors from export errors Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 2/5] block/nbd: move connection code from block/nbd to block/nbd-client Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 ` Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 4/5] block/nbd-client: fix nbd_reply_chunk_iter_receive Vladimir Sementsov-Ogievskiy ` (2 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den Split connection code to reuse it for reconnect. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index 6ff505c4b8..14b42f31df 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -999,12 +999,12 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, return sioc; } -int nbd_client_init(BlockDriverState *bs, - SocketAddress *saddr, - const char *export, - QCryptoTLSCreds *tlscreds, - const char *hostname, - Error **errp) +static int nbd_client_connect(BlockDriverState *bs, + SocketAddress *saddr, + const char *export, + QCryptoTLSCreds *tlscreds, + const char *hostname, + Error **errp) { NBDClientSession *client = nbd_get_client_session(bs); int ret; @@ -1048,8 +1048,6 @@ int nbd_client_init(BlockDriverState *bs, bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; } - qemu_co_mutex_init(&client->send_mutex); - qemu_co_queue_init(&client->free_sema); client->sioc = sioc; if (!client->ioc) { @@ -1066,3 +1064,18 @@ int nbd_client_init(BlockDriverState *bs, logout("Established connection with NBD server\n"); return 0; } + +int nbd_client_init(BlockDriverState *bs, + SocketAddress *saddr, + const char *export, + QCryptoTLSCreds *tlscreds, + const char *hostname, + Error **errp) +{ + NBDClientSession *client = nbd_get_client_session(bs); + + qemu_co_mutex_init(&client->send_mutex); + qemu_co_queue_init(&client->free_sema); + + return nbd_client_connect(bs, saddr, export, tlscreds, hostname, errp); +} -- 2.11.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 4/5] block/nbd-client: fix nbd_reply_chunk_iter_receive 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy ` (2 preceding siblings ...) 2018-05-07 15:44 ` [Qemu-devel] [PATCH 3/5] block/nbd-client: split connection from initialization Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 ` Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc Vladimir Sementsov-Ogievskiy 2018-05-17 9:54 ` [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 5 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den Use exported report, not the variable to be reused (should not really matter). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index 14b42f31df..dd712c59b3 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -599,7 +599,7 @@ static bool nbd_reply_chunk_iter_receive(NBDClientSession *s, } /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */ - if (nbd_reply_is_simple(&s->reply) || s->quit) { + if (nbd_reply_is_simple(reply) || s->quit) { goto break_loop; } -- 2.11.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy ` (3 preceding siblings ...) 2018-05-07 15:44 ` [Qemu-devel] [PATCH 4/5] block/nbd-client: fix nbd_reply_chunk_iter_receive Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 ` Vladimir Sementsov-Ogievskiy 2018-05-07 18:08 ` Eric Blake 2018-05-17 9:54 ` [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 5 siblings, 1 reply; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-07 15:44 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, vsementsov, den We have several paranoiac checks for ioc != NULL. But ioc may become NULL only on close, which should not happen during requests handling. Also, we check ioc only sometimes, not after each yield, which is inconsistent. Let's drop these checks. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/nbd-client.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/block/nbd-client.c b/block/nbd-client.c index dd712c59b3..87d90d9026 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -51,10 +51,6 @@ static void nbd_teardown_connection(BlockDriverState *bs) { NBDClientSession *client = nbd_get_client_session(bs); - if (!client->ioc) { /* Already closed */ - return; - } - /* finish any pending coroutines */ qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, @@ -150,10 +146,6 @@ static int nbd_co_send_request(BlockDriverState *bs, rc = -EIO; goto err; } - if (!s->ioc) { - rc = -EPIPE; - goto err; - } if (qiov) { qio_channel_set_cork(s->ioc, true); @@ -426,7 +418,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk( s->requests[i].receiving = true; qemu_coroutine_yield(); s->requests[i].receiving = false; - if (!s->ioc || s->quit) { + if (s->quit) { error_setg(errp, "Connection closed"); return -EIO; } @@ -967,10 +959,6 @@ void nbd_client_close(BlockDriverState *bs) NBDClientSession *client = nbd_get_client_session(bs); NBDRequest request = { .type = NBD_CMD_DISC }; - if (client->ioc == NULL) { - return; - } - nbd_send_request(client->ioc, &request); nbd_teardown_connection(bs); -- 2.11.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc 2018-05-07 15:44 ` [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc Vladimir Sementsov-Ogievskiy @ 2018-05-07 18:08 ` Eric Blake 2018-05-08 6:36 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2018-05-07 18:08 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block Cc: mreitz, kwolf, pbonzini, den On 05/07/2018 10:44 AM, Vladimir Sementsov-Ogievskiy wrote: > We have several paranoiac checks for ioc != NULL. But ioc may become > NULL only on close, which should not happen during requests handling. > Also, we check ioc only sometimes, not after each yield, which is > inconsistent. Let's drop these checks. Can (or even should) any of these be replaced by asserts that ioc is not NULL? > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/nbd-client.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc 2018-05-07 18:08 ` Eric Blake @ 2018-05-08 6:36 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-08 6:36 UTC (permalink / raw) To: Eric Blake, qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, den 07.05.2018 21:08, Eric Blake wrote: > On 05/07/2018 10:44 AM, Vladimir Sementsov-Ogievskiy wrote: >> We have several paranoiac checks for ioc != NULL. But ioc may become >> NULL only on close, which should not happen during requests handling. >> Also, we check ioc only sometimes, not after each yield, which is >> inconsistent. Let's drop these checks. > > Can (or even should) any of these be replaced by asserts that ioc is > not NULL? > >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/nbd-client.c | 14 +------------- >> 1 file changed, 1 insertion(+), 13 deletions(-) > No problem, I can add them. Actually in most of cases we will crash very soon on next QIO_CHANNEL_GET_CLASS(ioc). The exclusions (looked through) are: - "if (!s->ioc || s->quit) {" case, if reply is not simple. - zero-length io requests in other cases, if they are possible -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy ` (4 preceding siblings ...) 2018-05-07 15:44 ` [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc Vladimir Sementsov-Ogievskiy @ 2018-05-17 9:54 ` Vladimir Sementsov-Ogievskiy 2018-05-17 13:48 ` Eric Blake 5 siblings, 1 reply; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-17 9:54 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, eblake, den What about patches 1-4? 07.05.2018 18:44, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > Here are some preliminary refactoring patches, before NBD reconnect > series. > > Vladimir Sementsov-Ogievskiy (5): > block/nbd-client: split channel errors from export errors > block/nbd: move connection code from block/nbd to block/nbd-client > block/nbd-client: split connection from initialization > block/nbd-client: fix nbd_reply_chunk_iter_receive > block/nbd-client: don't check ioc > > block/nbd-client.h | 2 +- > block/nbd-client.c | 163 ++++++++++++++++++++++++++++++++++------------------- > block/nbd.c | 41 +------------- > 3 files changed, 107 insertions(+), 99 deletions(-) > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring 2018-05-17 9:54 ` [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy @ 2018-05-17 13:48 ` Eric Blake 2018-05-18 11:32 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2018-05-17 13:48 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block Cc: mreitz, kwolf, pbonzini, den On 05/17/2018 04:54 AM, Vladimir Sementsov-Ogievskiy wrote: > What about patches 1-4? Still on my list to review (I'm first trying to post an updated proposal on the libvirt list for managing incremental backups); but on first glance, the idea of being able to reconnect instead of permanently switching to EIO failures on first error seems reasonable. > > 07.05.2018 18:44, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> Here are some preliminary refactoring patches, before NBD reconnect >> series. >> >> Vladimir Sementsov-Ogievskiy (5): >> block/nbd-client: split channel errors from export errors >> block/nbd: move connection code from block/nbd to block/nbd-client >> block/nbd-client: split connection from initialization >> block/nbd-client: fix nbd_reply_chunk_iter_receive >> block/nbd-client: don't check ioc >> >> block/nbd-client.h | 2 +- >> block/nbd-client.c | 163 >> ++++++++++++++++++++++++++++++++++------------------- >> block/nbd.c | 41 +------------- >> 3 files changed, 107 insertions(+), 99 deletions(-) >> > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring 2018-05-17 13:48 ` Eric Blake @ 2018-05-18 11:32 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 11+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-05-18 11:32 UTC (permalink / raw) To: Eric Blake, qemu-devel, qemu-block; +Cc: mreitz, kwolf, pbonzini, den 17.05.2018 16:48, Eric Blake wrote: > On 05/17/2018 04:54 AM, Vladimir Sementsov-Ogievskiy wrote: >> What about patches 1-4? > > Still on my list to review (I'm first trying to post an updated > proposal on the libvirt list for managing incremental backups); but on > first glance, the idea of being able to reconnect instead of > permanently switching to EIO failures on first error seems reasonable. It relates to the same case as CMD_CACHE. We need to start guest over new empty disk with backing = r-o nbd server (backup). Guest is already running, disconnect will lead to data loss (not very significant, we can retry, starting from the same backup), so ability to reconnect may help. > >> >> 07.05.2018 18:44, Vladimir Sementsov-Ogievskiy wrote: >>> Hi all! >>> >>> Here are some preliminary refactoring patches, before NBD reconnect >>> series. >>> >>> Vladimir Sementsov-Ogievskiy (5): >>> block/nbd-client: split channel errors from export errors >>> block/nbd: move connection code from block/nbd to block/nbd-client >>> block/nbd-client: split connection from initialization >>> block/nbd-client: fix nbd_reply_chunk_iter_receive >>> block/nbd-client: don't check ioc >>> >>> block/nbd-client.h | 2 +- >>> block/nbd-client.c | 163 >>> ++++++++++++++++++++++++++++++++++------------------- >>> block/nbd.c | 41 +------------- >>> 3 files changed, 107 insertions(+), 99 deletions(-) >>> >> >> > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-18 11:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-07 15:44 [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 1/5] block/nbd-client: split channel errors from export errors Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 2/5] block/nbd: move connection code from block/nbd to block/nbd-client Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 3/5] block/nbd-client: split connection from initialization Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 4/5] block/nbd-client: fix nbd_reply_chunk_iter_receive Vladimir Sementsov-Ogievskiy 2018-05-07 15:44 ` [Qemu-devel] [PATCH 5/5] block/nbd-client: don't check ioc Vladimir Sementsov-Ogievskiy 2018-05-07 18:08 ` Eric Blake 2018-05-08 6:36 ` Vladimir Sementsov-Ogievskiy 2018-05-17 9:54 ` [Qemu-devel] [PATCH 0/5] NBD reconnect: preliminary refactoring Vladimir Sementsov-Ogievskiy 2018-05-17 13:48 ` Eric Blake 2018-05-18 11:32 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.