Message ID | 20190329042750.14704-7-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | NBD server alignment improvement | expand |
On 3/28/19 11:27 PM, Eric Blake wrote: > Both NBD_CMD_BLOCK_STATUS and structured NBD_CMD_READ will split their > reply according to bdrv_block_status() boundaries. If the block device > has a request_alignment smaller than 512, but we advertise a block > alignment of 512 to the client, then this can result in the server > reply violating client expectations by reporting a smaller region of > the export than what the client is permitted to address (although this > is less of an issue for qemu 4.0 clients, given recent client patches > to overlook our non-compliance at EOF). Since it's always better to > be strict in what we send, it is worth advertising the actual minimum > block limit rather than blindly rounding it up to 512. > > Note that the iotests output changes - both pre- and post-patch, the > server is reporting a mid-sector hole; but pre-patch, the client was > then rounding that up to a sector boundary as a workaround, while > post-patch the client doesn't have to round because it sees the > server's smaller advertised block size. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/server.c | 13 ++++++++----- > tests/qemu-iotests/241.out | 3 ++- > 2 files changed, 10 insertions(+), 6 deletions(-) I missed some iotest changes; 223 and 233 also need patches along the lines of: @@ -41,7 +41,7 @@ export: 'n' size: 4194304 flags: 0x4ef ( readonly flush fua trim zeroes df cache ) - min block: 512 + min block: 1
diff --git a/nbd/server.c b/nbd/server.c index fd013a2817a..218a2aa5e65 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -607,13 +607,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size * according to whether the client requested it, and according to * whether this is OPT_INFO or OPT_GO. */ - /* minimum - 1 for back-compat, or 512 if client is new enough. - * TODO: consult blk_bs(blk)->bl.request_alignment? */ - sizes[0] = - (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1; + /* minimum - 1 for back-compat, or actual if client will obey it. */ + if (client->opt == NBD_OPT_INFO || blocksize) { + sizes[0] = blk_get_request_alignment(exp->blk); + } else { + sizes[0] = 1; + } + assert(sizes[0] <= NBD_MAX_BUFFER_SIZE); /* preferred - Hard-code to 4096 for now. * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */ - sizes[1] = 4096; + sizes[1] = MAX(4096, sizes[0]); /* maximum - At most 32M, but smaller as appropriate. */ sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE); trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]); diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out index 91dcde97560..eccc7254d12 100644 --- a/tests/qemu-iotests/241.out +++ b/tests/qemu-iotests/241.out @@ -2,6 +2,7 @@ QA output created by 241 === Exporting unaligned raw image === -[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}] +[{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) *** done