Re: [Qemu-block] [PATCH v3 4/6] nbd/client: Support qemu-img convert from unaligned size

[Vladimir Sementsov-Ogievskiy]

29.03.2019 7:27, Eric Blake wrote:
> If an NBD server advertises a size that is not a multiple of a sector,
> the block layer rounds up that size, even though we set info.size to
> the exact byte value sent by the server. The block layer then proceeds
> to let us read or query block status on the hole that it added past
> EOF, which the NBD server is unlikely to be happy with. Fortunately,
> qemu as a server never advertizes an unaligned size, so we generally
> don't run into this problem; but the nbdkit server makes it easy to
> test:
> 
> $ printf %1000d 1 > f1
> $ ~/nbdkit/nbdkit -fv file f1 & pid=$!
> $ qemu-img convert -f raw nbd://localhost:10809 f2
> $ kill $pid
> $ qemu-img compare f1 f2
> 
> Pre-patch, the server attempts a 1024-byte read, which nbdkit
> rightfully rejects as going beyond its advertised 1000 byte size; the
> conversion fails and the output files differ (not even the first
> sector is copied, because qemu-img does not follow ddrescue's habit of
> trying smaller reads to get as much information as possible in spite
> of errors). Post-patch, the client's attempts to read (and query block
> status, for new enough nbdkit) are properly truncated to the server's
> length, with sane handling of the hole the block layer forced on
> us. Although f2 ends up as a larger file (1024 bytes instead of 1000),
> qemu-img compare shows the two images to have identical contents for
> display to the guest.
> 
> I didn't add iotests coverage since I didn't want to add a dependency
> on nbdkit in iotests. I also did NOT patch write, trim, or write
> zeroes - these commands continue to fail (usually with ENOSPC, but
> whatever the server chose), because we really can't write to the end
> of the file, and because 'qemu-img convert' is the most common case
> where we care about being tolerant (which is read-only). Perhaps we
> could truncate the request if the client is writing zeros to the tail,
> but that seems like more work, especially if the block layer is fixed
> in 4.1 to track byte-accurate sizing (in which case this patch would
> be reverted as unnecessary).
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   block/nbd-client.c | 39 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 3edb508f668..409c2171bc3 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -848,6 +848,25 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
> offset,
>       if (!bytes) {
>           return 0;
>       }
> +    /*
> +     * Work around the fact that the block layer doesn't do
> +     * byte-accurate sizing yet - if the read exceeds the server's
> +     * advertised size because the block layer rounded size up, then
> +     * truncate the request to the server and tail-pad with zero.
> +     */
> +    if (offset >= client->info.size) {
> +        assert(bytes < BDRV_SECTOR_SIZE);
> +        qemu_iovec_memset(qiov, 0, 0, bytes);
> +        return 0;
> +    }
> +    if (offset + bytes > client->info.size) {
> +        uint64_t slop = offset + bytes - client->info.size;
> +
> +        assert(slop < BDRV_SECTOR_SIZE);
> +        qemu_iovec_memset(qiov, bytes - slop, 0, slop);
> +        request.len -= slop;
> +    }
> +
>       ret = nbd_co_send_request(bs, &request, NULL);
>       if (ret < 0) {
>           return ret;
> @@ -966,7 +985,8 @@ int coroutine_fn 
> nbd_client_co_block_status(BlockDriverState *bs,
>           .from = offset,
>           .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX,
>                                                   bs->bl.request_alignment),
> -                                client->info.max_block), bytes),
> +                                client->info.max_block),
> +                   MIN(bytes, client->info.size - offset)),
>           .flags = NBD_CMD_FLAG_REQ_ONE,
>       };
> 
> @@ -977,6 +997,23 @@ int coroutine_fn 
> nbd_client_co_block_status(BlockDriverState *bs,
>           return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>       }
> 
> +    /*
> +     * Work around the fact that the block layer doesn't do
> +     * byte-accurate sizing yet - if the status request exceeds the
> +     * server's advertised size because the block layer rounded size
> +     * up, we truncated the request to the server (above), or are
> +     * called on just the hole.
> +     */
> +    if (offset >= client->info.size) {
> +        *pnum = bytes;
> +        assert(bytes < BDRV_SECTOR_SIZE);
> +        /* Intentionally don't report offset_valid for the hole */
> +        return BDRV_BLOCK_ZERO;
> +    }
> +
> +    if (client->info.min_block) {
> +        assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));

it will crash if info.size is unaligned..

" If a server advertises a minimum block size, the advertised export size 
SHOULD be an integer multiple of that block size"

violation "SHOULD" by server, should it lead to client crash?

> +    }
>       ret = nbd_co_send_request(bs, &request, NULL);
>       if (ret < 0) {
>           return ret;
> 


-- 
Best regards,
Vladimir