On 3/18/20 3:04 AM, Wouter Verhelst wrote:
On Wed, Mar 18, 2020 at 09:19:45AM +0300, Vladimir Sementsov-Ogievskiy wrote:
OK, understand. Reasonable thing, agreed. I'll resend.
Hmm. But we can't read in one go anyway, because we need to distinguish
NBD_REQUEST_MAGIC
from NBD_EXTENDED_REQUEST_MAGIC..
Yes, that needs to happen at any rate, indeed. So the difference will be
two reads rather than three, instead of one read rather than two.
That's still an advantage.
Not much of one. You're micro-optimizing the read()s, but in reality,
the sender will likely send() the entire packet at once, at which point
the data is in the kernel and the reads will succeed back-to-back, or
you can even write the client to read into a buffer to minimize syscalls
and then parse as much as needed out of the buffer.
You've got a LOT more overhead in the TCP packet and network
transmission time than you do in deciding whether the server has to do 2
or 3 read()s per client message.
While it might be nice to design a message that doesn't need the server
to do additional decision points mid-packet in determining how much
packet remains, that should not be your driving factor. Even with
current servers, that is already the case (the server has to decide
mid-packet whether it is handling NBD_CMD_WRITE and thus has more data
to read).
I think, that modern client will use mostly NBD_EXTENDED_REQUEST_MAGIC
interface, so it will
be great to optimize it. But to read extended request in one go, we should make
it
shorter than simple request, and it doesn't seem possible.
May be we should not support simple and extended requests together? May be
better to force
using only extended requests if they are negotiated? Then we will read extended
request in
on go, and we may just define it like
As extended requests already have to be negotiated, and no client nor
server exists yet that supports them, we can indeed declare that on
successful handshake of the new feature, a client may ONLY send extended
requests. However, a server already has to handle both packet types (if
the client doesn't request the feature) and a client already has to be
able to send both packet types (for fallback when talking to a server
that lacks the feature), so what does it buy us to require that when the
feature is negotiated, only extended packets may be sent? I guess it
boils down to whether the server implementation is simplified or made
more complex, depending on whether we state that once negotiated both
packet types are allowed (server must decide on a per-packet basis which
type it is receiving) or whether only extended packets are allowed
(server must now be more stateful in order to reject an ill-behaved
client that sends wrong type). In fact, I argue that a server that
replies to an extended packet even when an ill-behaved client that
forgot to negotiate them is a reasonable server implementation (the
client can't depend on that behavior, though).
C: 32 bits, 0x23876289, magic (`NBD_EXTENDED_REQUEST_MAGIC`)
C: 16 bits, flags
C: 16 bits, type
C: 64 bits, handle
C: 32 bits, length of payload (unsigned), MUST be greater or equal to 16
C: *length* bytes of payload data (if *length* is nonzero)
- so, we'll just read 36 bytes in one go, and then additional payload, if length
is more than 16.
That is, certainly, also an option, although I would define length of
payload to not include the offset and length bytes.
Agreed - mixing header length into the length of the payload field makes
life a bit more confusing. I'd rather have the length field be 0 when
sending a minimum-sized extended request packet.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
