Eric, >>> On 14 Dec 2016, at 18:51, Eric Blake <ebl...@redhat.com> 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.
Thanks for this
> 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.
Thanks - fixed in my merge I think (see below).
> 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]
+1
> 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.
Correct. There is a comment in the code saying it should probably
do something other than dumbly write zeroes. Patches welcome.
> 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):
OK I've merged master up into extension-write-zeroes in preparation
for a merge back down in the opposite direction.
I changed the handling of fua to make it more compatible with the
way master currently works, then:
> 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;
... took that, but:
> 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':
>
I don't understand this bit of the patch. This seems to disable
acls if 'dontfork' is enabled, and also change where socket
closing is done. There may well be a reason for this, but
it doesn't seem to be anything to do with writezeroes.
What's going on here?
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________ Nbd-general mailing list Nbd-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nbd-general
