| Message ID | 20190330165349.32256-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
30.03.2019 19:53, Eric Blake wrote: > Just as we recently added a trace for a server sending block status > that doesn't match the server's advertised minimum block alignment, > let's do the same for read chunks. But since qemu 3.1 is such a > server (because it advertised 512-byte alignment, but when serving a > file that ends in data but is not sector-aligned, NBD_CMD_READ would > detect a mid-sector change between data and hole at EOF and the > resulting read chunks are unaligned), we don't want to change our > behavior of otherwise tolerating unaligned reads. > > Note that even though we fixed the server for 4.0 to advertise an > actual block alignment (which gets rid of the unaligned reads at EOF > for posix files), we can still trigger it via other means: > > $ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file > > Arguably, that is a bug in the blkdebug block status function, for > leaking a block status that is not aligned. It may also be possible to > observe issues with a backing layer with smaller alignment than the > active layer, although so far I have been unable to write a reliable > iotest for that scenario. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 3/30/19 11:53 AM, Eric Blake wrote: > Just as we recently added a trace for a server sending block status > that doesn't match the server's advertised minimum block alignment, > let's do the same for read chunks. But since qemu 3.1 is such a > server (because it advertised 512-byte alignment, but when serving a > file that ends in data but is not sector-aligned, NBD_CMD_READ would > detect a mid-sector change between data and hole at EOF and the > resulting read chunks are unaligned), we don't want to change our > behavior of otherwise tolerating unaligned reads. > > Note that even though we fixed the server for 4.0 to advertise an > actual block alignment (which gets rid of the unaligned reads at EOF > for posix files), we can still trigger it via other means: > > $ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file > > Arguably, that is a bug in the blkdebug block status function, for > leaking a block status that is not aligned. It may also be possible to > observe issues with a backing layer with smaller alignment than the > active layer, although so far I have been unable to write a reliable > iotest for that scenario. More details on why I can't write an iotest (yet) - I wanted to use blkdebug to let me expose an active image with a required alignment greater than that of a backing image, but sadly, the current implementation of blkdebug (and filters in general) reports block status ONLY for the active layer (it does not properly follow the backing chain, without Max's patches): https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg00221.html But I _finally_ found a way around the fact that blkdebug doesn't (yet) do what I want - the qcow2 driver has a way to force alignment of 512, by using encryption. Thus, with this patch applied, I was able to set up a scenario that proves our server is still non-compliant (and I hope to have my patch for fixing it polished enough to send, soon, now that I can actually test my patch without one-off hacks to manually force the server into a fixed-size advertisement). $ printf %01000d 0 > base $ qemu-img create -f qcow2 -b base -F raw \ --object=secret,id=s0,data=12345 \ -o encrypt.format=luks,encrypt.key-secret=s0,encrypt.iter-time=10 \ top.qcow2 16k $ qemu-nbd --object secret,id=s0,data=12345 \ --image-opts driver=qcow2,file.filename=top.qcow2,encrypt.key-secret=s0 $ # in second terminal $ qemu-io --trace=nbd_\* -f raw nbd://localhost:10809 -c 'r 0 2k' ... 23211@1554150310.580828:nbd_send_request Sending request to server: { .from = 0, .len = 2048, .handle = 93824995142096, .flags = 0x0, .type = 0 (read) } 23211@1554150310.581049:nbd_receive_structured_reply_chunk Got structured reply chunk: { flags = 0x0, type = 1 (data), handle = 93824995142096, length = 1008 } 23211@1554150310.581063:nbd_structured_read_compliance server sent non-compliant unaligned read data chunk 23211@1554150310.581076:nbd_receive_structured_reply_chunk Got structured reply chunk: { flags = 0x1, type = 2 (hole), handle = 93824995142096, length = 12 } 23211@1554150310.581085:nbd_structured_read_compliance server sent non-compliant unaligned read hole chunk
diff --git a/block/nbd-client.c b/block/nbd-client.c index 409c2171bc3..790ecc1ee1c 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -211,7 +211,8 @@ static inline uint64_t payload_advance64(uint8_t **payload) return ldq_be_p(*payload - 8); } -static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk, +static int nbd_parse_offset_hole_payload(NBDClientSession *client, + NBDStructuredReplyChunk *chunk, uint8_t *payload, uint64_t orig_offset, QEMUIOVector *qiov, Error **errp) { @@ -233,6 +234,10 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk, " region"); return -EINVAL; } + if (client->info.min_block && + !QEMU_IS_ALIGNED(hole_size, client->info.min_block)) { + trace_nbd_structured_read_compliance("hole"); + } qemu_iovec_memset(qiov, offset - orig_offset, 0, hole_size); @@ -390,6 +395,9 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s, " region"); return -EINVAL; } + if (s->info.min_block && !QEMU_IS_ALIGNED(data_size, s->info.min_block)) { + trace_nbd_structured_read_compliance("data"); + } qemu_iovec_init(&sub_qiov, qiov->niov); qemu_iovec_concat(&sub_qiov, qiov, offset - orig_offset, data_size); @@ -712,7 +720,7 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle, * in qiov */ break; case NBD_REPLY_TYPE_OFFSET_HOLE: - ret = nbd_parse_offset_hole_payload(&reply.structured, payload, + ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload, offset, qiov, &local_err); if (ret < 0) { s->quit = true; diff --git a/block/trace-events b/block/trace-events index debb25c0ac8..7335a425404 100644 --- a/block/trace-events +++ b/block/trace-events @@ -158,6 +158,7 @@ iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, ui # nbd-client.c nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from non-compliant server: %s" +nbd_structured_read_compliance(const char *type) "server sent non-compliant unaligned read %s chunk" nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s" nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"
Just as we recently added a trace for a server sending block status that doesn't match the server's advertised minimum block alignment, let's do the same for read chunks. But since qemu 3.1 is such a server (because it advertised 512-byte alignment, but when serving a file that ends in data but is not sector-aligned, NBD_CMD_READ would detect a mid-sector change between data and hole at EOF and the resulting read chunks are unaligned), we don't want to change our behavior of otherwise tolerating unaligned reads. Note that even though we fixed the server for 4.0 to advertise an actual block alignment (which gets rid of the unaligned reads at EOF for posix files), we can still trigger it via other means: $ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file Arguably, that is a bug in the blkdebug block status function, for leaking a block status that is not aligned. It may also be possible to observe issues with a backing layer with smaller alignment than the active layer, although so far I have been unable to write a reliable iotest for that scenario. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/nbd-client.c | 12 ++++++++++-- block/trace-events | 1 + 2 files changed, 11 insertions(+), 2 deletions(-)