The NBD spec requires that responses to NBD_CMD_BLOCK_STATUS be aligned to the server's advertised min_block alignment, if the client agreed to abide by alignments. In general, since dirty bitmap granularity cannot go below 512, and it is hard to provoke qcow2 to go above an alignment of 512, this is not an issue. And until the block layer can see through filters, the moment you use blkdebug, you can no longer export dirty bitmaps over NBD. But once we fix the traversal through filters, then blkdebug can create a scenario where the server is provoked into supplying a non-compliant reply.
Thus, it is time to fix the dirty bitmap code to round requests to aligned boundaries. This hack patch lets blkdebug be used; although note that Max has proposed a more complete solution https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03534.html | diff --git a/nbd/server.c b/nbd/server.c | index 576deb931c8..1d370b5b2c7 100644 | --- a/nbd/server.c | +++ b/nbd/server.c | @@ -1507,6 +1507,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, | BdrvDirtyBitmap *bm = NULL; | | while (true) { | + while (bs->drv && bs->drv->is_filter && bs->file) { | + bs = bs->file->bs; | + } | bm = bdrv_find_dirty_bitmap(bs, bitmap); | if (bm != NULL || bs->backing == NULL) { | break; Then the following sequence creates a dirty bitmap with granularity 512, exposed through a blkdebug interface with minimum block size 4k; correct behavior is to see a map showing the first 4k as "data":false (corresponding to the dirty bit in the 2nd of 8 sectors, rounded up). Without this patch, the code instead showed the entire image as "data":true (due to the client working around the server non-compliance by coalescing regions, but assuming that data:true takes precedence, which is the opposite of the behavior we want during x-dirty-bitmap). $ printf %01000d 0 > img $ qemu-img create -f qcow2 -F raw -b img img.wrap 64k $ qemu-system-x86_64 -nodefaults -nographic -qmp stdio {'execute':'qmp_capabilities'} {'execute':'blockdev-add','arguments':{'node-name':'n','driver':'qcow2','file':{'driver':'file','filename':'img.wrap'}}} {'execute':'nbd-server-start','arguments':{'addr':{'type':'inet','data':{'host':'localhost','port':'10809'}}}} {'execute':'block-dirty-bitmap-add','arguments':{'node':'n','name':'b','persistent':true,'granularity':512}} {'execute':'nbd-server-add','arguments':{'device':'n','bitmap':'b','writable':true}} ^z $ bg $ qemu-io -f raw -c 'w 513 1' nbd://localhost:10809 $ fg {'execute':'nbd-server-remove','arguments':{'name':'n'}} {'execute':'block-dirty-bitmap-disable','arguments':{'node':'n','name':'b'}} {'execute':'blockdev-add','arguments':{'driver':'blkdebug','align':4096,'image':'n','node-name':'wrap'}} {'execute':'nbd-server-add','arguments':{'device':'wrap','bitmap':'b'}} ^z $ bg $ qemu-img map --output=json --image-opts driver=nbd,x-dirty-bitmap=qemu:dirty-bitmap:b,server.type=inet,server.host=localhost,server.port=10809,export=wrap $ fg {'execute':'quit'} Signed-off-by: Eric Blake <ebl...@redhat.com> --- Not for 4.0; once Max's patches land to see through filters, then this commit message will need to be rewritten, and the steps outlined above instead turned into an addition for iotest 241. --- nbd/server.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 576deb931c8..82da0bae9ba 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1982,7 +1982,8 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle, * byte length encoded (which may be smaller or larger than the * original), and return the number of extents used. */ -static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, +static unsigned int bitmap_to_extents(uint32_t align, + BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t *length, NBDExtent *extents, unsigned int nb_extents, bool dont_fragment) @@ -2008,13 +2009,30 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, bdrv_set_dirty_iter(it, begin); end = bdrv_dirty_iter_next(it); } - if (end == -1 || end - begin > UINT32_MAX) { + if (end == -1 || end - begin > UINT32_MAX - align) { /* Cap to an aligned value < 4G beyond begin. */ end = MIN(bdrv_dirty_bitmap_size(bitmap), begin + UINT32_MAX + 1 - - bdrv_dirty_bitmap_granularity(bitmap)); + MAX(align, bdrv_dirty_bitmap_granularity(bitmap))); next_dirty = dirty; } + /* + * Round unaligned bits: any transition mid-alignment makes + * that entire aligned region appear dirty + */ + if (!QEMU_IS_ALIGNED(end, align)) { + if (dirty) { + end = QEMU_ALIGN_UP(end, align); + } else if (end - begin < align) { + end = begin + align; + dirty = true; + } else { + end = QEMU_ALIGN_DOWN(end, align); + } + if (end < bdrv_dirty_bitmap_size(bitmap)) { + next_dirty = bdrv_get_dirty_locked(NULL, bitmap, end); + } + } if (dont_fragment && end > overall_end) { end = overall_end; } @@ -2042,11 +2060,21 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle, { int ret; unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS; - NBDExtent *extents = g_new(NBDExtent, nb_extents); + NBDExtent *extents; uint64_t final_length = length; - nb_extents = bitmap_to_extents(bitmap, offset, &final_length, extents, - nb_extents, dont_fragment); + /* Easiest to just refuse to answer an unaligned query */ + if (client->check_align && + !QEMU_IS_ALIGNED(offset | length, client->check_align)) { + return nbd_co_send_structured_error(client, handle, -EINVAL, + "unaligned dirty bitmap request", + errp); + } + + extents = g_new(NBDExtent, nb_extents); + nb_extents = bitmap_to_extents(client->check_align ?: 1, bitmap, offset, + &final_length, extents, nb_extents, + dont_fragment); ret = nbd_co_send_extents(client, handle, extents, nb_extents, final_length, last, context_id, errp); -- 2.20.1
