On 12/14/2016 12:55 PM, Alex Bligh wrote: > >> On 14 Dec 2016, at 18:51, Eric Blake <eblake@...696...> wrote: >> >> It's working well in qemu 2.8 without needing tweaks to the >> documentation. Should we try and do some cross-implementation testing >> today, before doing the actual merge? > > If you have a minute to do so, that would be great. With qemu-io as the client and nbd-server as the server (a merge of the master branch, extensions-write-zeroes-branch, and the patch below), I was successfully able to validate that nbd-server correctly advertises the bit when asked to, that it handles NBD_CMD_WRITE_ZEROES requests at any alignment, and that it is okay with any combination of the flags NBD_CMD_FLAG_FUA and NBD_CMD_FLAG_NO_HOLE, insofar as the client can then re-read correct all-zero data. There's an oddity with the error reported when attempting to write to a read-only export: NBD_CMD_WRITE fails with EPERM, but NBD_CMD_WRITE_ZEROES fails with EINVAL. That's probably a bug in nbd-server, based on the recommendations of the spec, but one which qemu-io handled just fine as a client. I tested with this nbd-server config file, and the mentioned command lines: # Use with: # ./nbd-server -C local -d # ./qemu-io -f raw -t none nbd://localhost:6666/file2 [generic] port = 6666 allowlist = true [file2] exportname = /home/eblake/qemu/file2 flush = true fua = true trim = true # uncomment as needed # readonly = true [It would be nice if '/nbd-server -C file -d -r' would force read-only mode on ALL exports, regardless of the readonly settings in the individual export settings of the config file - but I suppose that's a patch for another day] A quick read of nbd-server.c shows that it never tries to punch holes (as permitted when NBD_CMD_FLAG_NO_HOLE is omitted), so the behavior is semantically correct even if not optimal when sparse files are supported. qemu-io as a client correctly refuses to send NBD_CMD_WRITE_ZEROES if the server doesn't advertise support, and does not allow me to easily attempt to try a write beyond EOF (which means I was unable to test the server response in the face of an ill-behaved client that sends the command when not advertised, or which sends bad length/offset). I could tweak the code to qemu's client to allow me to test these situations, if you think further testing of ill-behaved clients is worthwhile, but I'm at least glad that the positive testing was successful. As promised, here's the patches I used, above the merge of the master and extensions-write-zeroes branches (most of the merge conflicts were in doc/proto.md, I didn't closely review the resolution to those merges, but we'll have to do it correctly when we actually make the merge): diff --git c/nbd-server.c w/nbd-server.c index 9d031c3..312091a 100644 --- c/nbd-server.c +++ w/nbd-server.c @@ -2252,7 +2252,7 @@ int mainloop(CLIENT *client) { ERROR(client, reply, errno); continue; } - SEND(client->net, reply); + SEND(client, reply); continue; default: @@ -2690,11 +2690,15 @@ handle_modern_connection(GArray *const servers, const int sock, struct generic_c msg(LOG_ERR, "Modern initial negotiation failed"); goto handler_err; } - len = strlen(client->server->servename); - writeit(commsocket, &len, sizeof len); - writeit(commsocket, client->server->servename, len); - readit(commsocket, &acl, 1); - close(commsocket); + if (dontfork) { + acl = 'Y'; + } else { + len = strlen(client->server->servename); + writeit(commsocket, &len, sizeof len); + writeit(commsocket, client->server->servename, len); + readit(commsocket, &acl, 1); + close(commsocket); + } switch(acl) { case 'N': -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature