Re: [Qemu-devel] [PATCH for-4.0] nbd-client: Work around server BLOCK_STATUS misalignment at EOF

[Vladimir Sementsov-Ogievskiy]

26.03.2019 20:13, Eric Blake wrote:
> The NBD spec is clear that a server that advertises a minimum block
> size should reply to NBD_CMD_BLOCK_STATUS with extents aligned
> accordingly. However, we know that the qemu NBD server implementation
> has had a corner-case bug where it is not compliant with the spec,
> present since the introduction of NBD_CMD_BLOCK_STATUS in qemu 2.12
> (and unlikely to be patched in time for 4.0). Namely, when qemu is
> serving a file that is not a multiple of 512 bytes, it rounds the size
> advertised over NBD up to the next sector boundary (someday, I'd like
> to fix that to be byte-accurate, but it's a much bigger audit not
> appropriate for this release); yet if the final sector contains data
> prior to EOF, lseek(SEEK_HOLE) will point to the implicit hole
> mid-sector which qemu then reported over NBD.
> 
> We are well within our rights to hang up on a server that can't follow
> the spec, but it is more useful to try and keep the connection alive
> in spite of the problem. Do so by tracing a message about the problem,
> and then either truncating the request back to an aligned boundary (if
> it covered more than the final sector) or widening it out to the full
> boundary with a forced status of data (since truncating would result
> in 0 bytes, but we have to make progress, and valid since data is a
> default-safe answer). And in practice, since the problem only happens
> on a sector that starts with data and ends with a hole, we are going
> to want to read that full sector anyway (where qemu as the server
> fills in the tail beyond EOF with appropriate NUL bytes).
> 
> Easy reproduction:
> $ printf %1000d 1 > file
> $ qemu-nbd -f raw -t file & pid=$!
> $ qemu-img map --output=json -f raw nbd://localhost:10809
> qemu-img: Could not read file metadata: Invalid argument
> $ kill $pid

would be good to add it to iotests

> 
> where the patched version instead succeeds with:
> [{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> 
> Signed-off-by: Eric Blake <address@hidden>


Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/nbd-client.c | 28 +++++++++++++++++++++++++---
>   1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index a3b70d14004..241cc555246 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -269,14 +269,36 @@ static int 
> nbd_parse_blockstatus_payload(NBDClientSession *client,
>       extent->length = payload_advance32(&payload);
>       extent->flags = payload_advance32(&payload);
> 
> -    if (extent->length == 0 ||
> -        (client->info.min_block && !QEMU_IS_ALIGNED(extent->length,
> -                                                    
> client->info.min_block))) {
> +    if (extent->length == 0) {
>           error_setg(errp, "Protocol error: server sent status chunk with "
>                      "invalid length");

may be improved to s/invalid/zero/

>           return -EINVAL;
>       }
> 
> +    /*
> +     * A server sending unaligned block status is in violation of the
> +     * protocol, but as qemu-nbd 3.1 is such a server (at least for
> +     * POSIX files that are not a multiple of 512 bytes, since qemu
> +     * rounds files up to 512-byte multiples but lseek(SEEK_HOLE)
> +     * still sees an implicit hole beyond the real EOF), it's nicer to
> +     * work around the misbehaving server. If the request included
> +     * more than the final unaligned block, truncate it back to an
> +     * aligned result; if the request was only the final block, round
> +     * up to the full block and change the status to fully-allocated
> +     * (always a safe status, even if it loses information).
> +     */
> +    if (client->info.min_block && !QEMU_IS_ALIGNED(extent->length,
> +                                                   client->info.min_block)) {
> +        trace_nbd_parse_blockstatus_compliance("extent length is unaligned");
> +        if (extent->length > client->info.min_block) {
> +            extent->length = QEMU_ALIGN_DOWN(extent->length,
> +                                             client->info.min_block);
> +        } else {
> +            extent->length = client->info.min_block;
> +            extent->flags = 0;
> +        }
> +    }
> +
>       /*
>        * 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
> 


-- 
Best regards,
Vladimir