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 <vsement...@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
signature.asc
Description: OpenPGP digital signature
