[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_status function: server part |
Date: |
Thu, 9 Feb 2017 09:38:51 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
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 <address@hidden>
> ---
> 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
- Re: [Qemu-block] [PATCH 04/18] nbd/client: refactor nbd_receive_starttls, (continued)
[Qemu-block] [PATCH 11/18] nbd: BLOCK_STATUS for bitmap export: server part, Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 07/18] nbd: Minimal structured read for client, Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 09/18] block/dirty-bitmap: add bdrv_dirty_bitmap_next(), Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_status function: server part, Vladimir Sementsov-Ogievskiy, 2017/02/03
- Re: [Qemu-block] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_status function: server part,
Eric Blake <=
[Qemu-block] [PATCH 14/18] qmp: add x-debug-block-dirty-bitmap-sha256, Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 03/18] nbd: Minimal structured read for server, Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 12/18] nbd: BLOCK_STATUS for bitmap export: client part, Vladimir Sementsov-Ogievskiy, 2017/02/03
[Qemu-block] [PATCH 08/18] hbitmap: add next_zero function, Vladimir Sementsov-Ogievskiy, 2017/02/03