[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/2] nbd: Tolerate some server non-compliance in
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH 2/2] nbd: Tolerate some server non-compliance in NBD_CMD_BLOCK_STATUS |
Date: |
Mon, 25 Mar 2019 11:19:15 +0000 |
24.03.2019 0:26, Eric Blake wrote:
> The NBD spec states that NBD_CMD_FLAG_REQ_ONE (which we currently
> always use) should not reply with an extent larger than our request,
> and that the server's response should be exactly one extent. Right
> now, that means that if a server sends more than one extent, we treat
> the server as broken, fail the block status request, and disconnect,
> which prevents all further use of the block device. But while good
> software should be strict in what it sends, it should be tolerant in
> what it receives.
>
> While trying to implement NBD_CMD_BLOCK_STATUS in nbdkit, we
> temporarily had a non-compliant server sending too many extents in
> spite of REQ_ONE. Oddly enough, 'qemu-img convert' with qemu 3.1
> failed with a somewhat useful message:
> qemu-img: Protocol error: invalid payload for NBD_REPLY_TYPE_BLOCK_STATUS
>
> which then disappeared with commit d8b4bad8, on the grounds that an
> error message flagged only at the time of coroutine teardown is
> pointless, and instead we should rely on the actual failed API to
> report an error - in other words, the 3.1 behavior was masking the
> fact that qemu-img was not reporting an error. That has since been
> fixed in the previous patch, where qemu-img convert now fails with:
> qemu-img: Could not read sector 0 metadata: Invalid argument
>
> But even that is harsh. Since we already partially relaxed things in
> commit acfd8f7a to tolerate a server that exceeds the cap (although
> that change was made prior to the NBD spec actually putting a cap on
> the extent length during REQ_ONE - in fact, the NBD spec change was
> BECAUSE of the qemu behavior prior to that commit), it's not that much
> harder to argue that we should also tolerate a server that sends too
> many extents. But at the same time, it's nice to trace when we are
> being tolerant of server non-compliance, in order to help server
> writers fix their implementations to be more portable (if they refer
> to our traces, rather than just stderr).
if they.. which is unlikely. So don't it worth a warn_report instead?
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> Reported-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> block/nbd-client.c | 21 ++++++++++++++++-----
> block/trace-events | 1 +
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 5fc9ea40383..2b388b1e422 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -240,8 +240,8 @@ static int
> nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
> }
>
> /* nbd_parse_blockstatus_payload
> - * support only one extent in reply and only for
> - * base:allocation context
> + * Based on our request, we expect only one extent in reply, for the
> + * base:allocation context.
> */
> static int nbd_parse_blockstatus_payload(NBDClientSession *client,
> NBDStructuredReplyChunk *chunk,
> @@ -250,7 +250,8 @@ static int nbd_parse_blockstatus_payload(NBDClientSession
> *client,
> {
> uint32_t context_id;
>
> - if (chunk->length != sizeof(context_id) + sizeof(*extent)) {
> + /* The server succeeded, so it must have sent [at least] one extent */
> + if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
> error_setg(errp, "Protocol error: invalid payload for "
> "NBD_REPLY_TYPE_BLOCK_STATUS");
> return -EINVAL;
> @@ -276,10 +277,20 @@ static int
> nbd_parse_blockstatus_payload(NBDClientSession *client,
> return -EINVAL;
> }
>
> - /* The server is allowed to send us extra information on the final
> - * extent; just clamp it to the length we requested. */
> + /*
> + * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have
> + * sent us any more than one extent, nor should it have included
> + * status beyond our request in that extent. However, it's easy
> + * enough to ignore the server's noncompliance without killing the
> + * connection; just ignore trailing extents, and clamp things to
> + * the length of our request.
> + */
> + if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
> + trace_nbd_parse_blockstatus_compliance("more than one extent");
> + }
> if (extent->length > orig_length) {
> extent->length = orig_length;
> + trace_nbd_parse_blockstatus_compliance("extent length too large");
> }
>
> return 0;
> diff --git a/block/trace-events b/block/trace-events
> index 70056eafd20..01ae6475707 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -158,6 +158,7 @@ nvme_cmd_map_qiov_iov(void *s, int i, void *page, int
> pages) "s %p iov[%d] %p pa
> iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t
> dst_off, uint64_t bytes, int ret) "src_lun %p offset %"PRIu64" dst_lun %p
> offset %"PRIu64" bytes %"PRIu64" ret %d"
>
> # block/nbd-client.c
> +nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from
> non-compliant server: %s"
> nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
> nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t
> flags, uint16_t type, const char *name, int ret, const char *err) "Request
> failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ",
> .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
>
--
Best regards,
Vladimir