29.03.2019 7:27, Eric Blake wrote: > If an NBD server advertises a size that is not a multiple of a sector, > the block layer rounds up that size, even though we set info.size to > the exact byte value sent by the server. The block layer then proceeds > to let us read or query block status on the hole that it added past > EOF, which the NBD server is unlikely to be happy with. Fortunately, > qemu as a server never advertizes an unaligned size, so we generally > don't run into this problem; but the nbdkit server makes it easy to > test: > > $ printf %1000d 1 > f1 > $ ~/nbdkit/nbdkit -fv file f1 & pid=$! > $ qemu-img convert -f raw nbd://localhost:10809 f2 > $ kill $pid > $ qemu-img compare f1 f2 > > Pre-patch, the server attempts a 1024-byte read, which nbdkit > rightfully rejects as going beyond its advertised 1000 byte size; the > conversion fails and the output files differ (not even the first > sector is copied, because qemu-img does not follow ddrescue's habit of > trying smaller reads to get as much information as possible in spite > of errors). Post-patch, the client's attempts to read (and query block > status, for new enough nbdkit) are properly truncated to the server's > length, with sane handling of the hole the block layer forced on > us. Although f2 ends up as a larger file (1024 bytes instead of 1000), > qemu-img compare shows the two images to have identical contents for > display to the guest. > > I didn't add iotests coverage since I didn't want to add a dependency > on nbdkit in iotests. I also did NOT patch write, trim, or write > zeroes - these commands continue to fail (usually with ENOSPC, but > whatever the server chose), because we really can't write to the end > of the file, and because 'qemu-img convert' is the most common case > where we care about being tolerant (which is read-only). Perhaps we > could truncate the request if the client is writing zeros to the tail, > but that seems like more work, especially if the block layer is fixed > in 4.1 to track byte-accurate sizing (in which case this patch would > be reverted as unnecessary). > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > block/nbd-client.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 3edb508f668..409c2171bc3 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -848,6 +848,25 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t > offset, > if (!bytes) { > return 0; > } > + /* > + * Work around the fact that the block layer doesn't do > + * byte-accurate sizing yet - if the read exceeds the server's > + * advertised size because the block layer rounded size up, then > + * truncate the request to the server and tail-pad with zero. > + */ > + if (offset >= client->info.size) { > + assert(bytes < BDRV_SECTOR_SIZE); > + qemu_iovec_memset(qiov, 0, 0, bytes); > + return 0; > + } > + if (offset + bytes > client->info.size) { > + uint64_t slop = offset + bytes - client->info.size; > + > + assert(slop < BDRV_SECTOR_SIZE); > + qemu_iovec_memset(qiov, bytes - slop, 0, slop); > + request.len -= slop; > + } > + > ret = nbd_co_send_request(bs, &request, NULL); > if (ret < 0) { > return ret; > @@ -966,7 +985,8 @@ int coroutine_fn > nbd_client_co_block_status(BlockDriverState *bs, > .from = offset, > .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX, > bs->bl.request_alignment), > - client->info.max_block), bytes), > + client->info.max_block), > + MIN(bytes, client->info.size - offset)), > .flags = NBD_CMD_FLAG_REQ_ONE, > }; > > @@ -977,6 +997,23 @@ int coroutine_fn > nbd_client_co_block_status(BlockDriverState *bs, > return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > } > > + /* > + * Work around the fact that the block layer doesn't do > + * byte-accurate sizing yet - if the status request exceeds the > + * server's advertised size because the block layer rounded size > + * up, we truncated the request to the server (above), or are > + * called on just the hole. > + */ > + if (offset >= client->info.size) { > + *pnum = bytes; > + assert(bytes < BDRV_SECTOR_SIZE); > + /* Intentionally don't report offset_valid for the hole */ > + return BDRV_BLOCK_ZERO; > + } > + > + if (client->info.min_block) { > + assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));
it will crash if info.size is unaligned.. " If a server advertises a minimum block size, the advertised export size SHOULD be an integer multiple of that block size" violation "SHOULD" by server, should it lead to client crash? > + } > ret = nbd_co_send_request(bs, &request, NULL); > if (ret < 0) { > return ret; > -- Best regards, Vladimir