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 <vsement...@virtuozzo.com> > --- > 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