Re: [Qemu-block] [PATCH 18/18] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Eric Blake]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal realization: only first extent from the answer is used.

If you're only going to use one extent, should you implement
NBD_CMD_FLAG_REQ_ONE support to save the server some effort?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/nbd-client.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  block/nbd-client.h  |  5 +++++
>  block/nbd.c         |  3 +++
>  include/block/nbd.h |  2 +-
>  nbd/client.c        | 23 ++++++++++++++++-------
>  qemu-nbd.c          |  2 +-
>  6 files changed, 66 insertions(+), 10 deletions(-)

> +int64_t coroutine_fn nbd_client_co_get_block_status(BlockDriverState *bs,
> +                                                    int64_t sector_num,
> +                                                    int nb_sectors, int 
> *pnum,
> +                                                    BlockDriverState **file)
> +{
> +    int64_t ret;
> +    uint32_t nb_extents;
> +    NBDExtent *extents;
> +    NBDClientSession *client = nbd_get_client_session(bs);
> +
> +    if (!client->block_status_ok) {
> +        *pnum = nb_sectors;
> +        ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
> +        if (bs->drv->protocol_name) {
> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
> +        }
> +        return ret;
> +    }

Looks like a sane fallback when we don't have anything more accurate.

> +
> +    ret = nbd_client_co_cmd_block_status(bs, sector_num << BDRV_SECTOR_BITS,
> +                                         nb_sectors << BDRV_SECTOR_BITS,
> +                                         &extents, &nb_extents);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    *pnum = extents[0].length >> BDRV_SECTOR_BITS;
> +    ret = (extents[0].flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_ALLOCATED) |
> +          (extents[0].flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
> +
> +    if ((ret & BDRV_BLOCK_ALLOCATED) && !(ret & BDRV_BLOCK_ZERO)) {
> +        ret |= BDRV_BLOCK_DATA;
> +    }

And this conversion looks correct.

> @@ -579,7 +617,8 @@ int nbd_client_init(BlockDriverState *bs,
>                                  &client->size,
>                                  &client->structured_reply,
>                                  bitmap_name,
> -                                &client->bitmap_ok, errp);
> +                                &client->bitmap_ok,
> +                                &client->block_status_ok, errp);

That's a lot of parameters we're adding; I'm wondering if its smarter to
start passing things through via a struct.  In fact, I have posted
patches previously (and need to rebase and repost them) that introduce
such a struct, in order to make the INFO extension viable.

> @@ -681,11 +681,19 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint16_t *flags,
>                                                false, NULL) == 0;
>              }
>  
> -            if (!!structured_reply && *structured_reply && !!bitmap_name) {
> +            if (!!structured_reply && *structured_reply) {
>                  int ret;
> -                assert(!!bitmap_ok);
> -                ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
> -                                               bitmap_ok, errp) == 0;
> +
> +                if (!!bitmap_name) {
> +                    assert(!!bitmap_ok);
> +                    ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
> +                                                   bitmap_ok, errp) == 0;
> +                } else {
> +                    ret = nbd_receive_query_meta_context(ioc, name,
> +                                                         "base:allocation",
> +                                                         block_status_ok,
> +                                                         errp);
> +                }

This looks weird trying to grab 'base:allocation' only if bitmap_name is
not provided.  The logic will probably have to be different if we want
to allow a client to track both pieces of information at once.

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

signature.asc
Description: OpenPGP digital signature