Re: [Qemu-devel] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3830 bytes --]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal realization: only one extent in server answer is supported.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h |  3 ++
>  nbd/nbd-internal.h  |  1 +
>  nbd/server.c        | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 08d5e51f21..69aee1eda1 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -181,6 +181,9 @@ enum {
>  #define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
>  #define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)
>  
> +#define NBD_STATE_HOLE 1
> +#define NBD_STATE_ZERO (1 << 1)

Why not (1 << 0) for NBD_STATE_HOLE?

I almost wonder if it is better to rebase the series to implement
base:allocation first, rather than last, just for easier
interoperability testing with other implementations that still need to
implement structured replies; but I don't think it's a show-stopper.

>  
> +static int blockstatus_to_extent(BlockDriverState *bs, uint64_t offset,
> +                                  uint64_t length, NBDExtent *extent)
> +{
> +    BlockDriverState *file;
> +    uint64_t start_sector = offset >> BDRV_SECTOR_BITS;
> +    uint64_t last_sector = (offset + length - 1) >> BDRV_SECTOR_BITS;

Converting from bytes to sectors by rounding...

> +    uint64_t begin = start_sector;
> +    uint64_t end = last_sector + 1;
> +
> +    int nb = MIN(INT_MAX, end - begin);
> +    int64_t ret = bdrv_get_block_status_above(bs, NULL, begin, nb, &nb, &file);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    extent->flags =
> +        cpu_to_be32((ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
> +                    (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0));
> +    extent->length = cpu_to_be32((nb << BDRV_SECTOR_BITS) -
> +                                 (offset - (start_sector << BDRV_SECTOR_BITS)));

...then computing the length by undoing the rounding. I really think we
should consider fixing bdrv_get_block_status_above() to be byte-based,
but that's a separate series.  Your calculations look correct in the
meantime, although '(offset & (BDRV_SECTOR_SIZE - 1))' may be a bit
easier to read than '(offset - (start_sector << BDRV_SECTOR_BITS))'.

> @@ -1869,13 +1930,18 @@ static void nbd_trip(void *opaque)
>          break;
>      case NBD_CMD_BLOCK_STATUS:
>          TRACE("Request type is BLOCK_STATUS");
> -        if (client->export_bitmap == NULL) {
> +        if (!!client->export_bitmap) {

!! is not necessary here.

> +            ret = nbd_co_send_bitmap(req->client, request.handle,
> +                                     client->export_bitmap, request.from,
> +                                     request.len, 0);
> +        } else if (client->export_block_status) {
> +            ret = nbd_co_send_block_status(req->client, request.handle,
> +                                           blk_bs(exp->blk), request.from,
> +                                           request.len, 0);

Umm, why are we sending only one status? If the client requests two ids
during NBD_OPT_SET_META_CONTEXT, we should be able to provide both
pieces of information at once.  For a minimal implementation, it works
for proof of concept, but it is pretty restrictive to tell clients that
they can only request a single status context.  I'm fine if we add that
functionality in a later patch, but we'd better have the implementation
ready for the same release as this patch (I still think 2.9 is a
reasonable goal).


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]