On 03/29/2018 08:19 AM, Vladimir Sementsov-Ogievskiy wrote: > 28.03.2018 22:43, Eric Blake wrote: >> The previous patch mentioned that a server that honors larger >> TRIM/WRITE_ZEROES requests than accepted for WRITE has to choose >> whether to advertise the maximum block size as the smaller limit >> at which it does hard disconnect for WRITE, or the larger limit >> at which it returns EINVAL for too-large trim/zero. Let's make >> the situation less ambiguous by allowing a client and server to >> negotiate explicit (larger) alternate limits for these two >> commands, using the fact that NBD_OPT_GO already documents that >> the client may request additional NBD_INFO items, the server may >> reply with unrequested additional NBD_INFO items, and that both >> sides must ignore NBD_INFO items that they don't recognize. >> >> Note that since there are existing clients that do not expect >> the new info items, the server MUST NOT fail a request that is >> unaligned to the (larger) minimum trim/zero size with EINVAL; >> only something unaligned to the main minimum block size can >> result in EINVAL. Rather, the new minimum sizes are for the >> client's information about what is efficient. >> >> + any. The maximum trim size MUST be either 0xffffffff (for no >> + inherent 32-bit limit) or a multiple of the minimum trim size, >> + and MUST be at least as large as the maximum block size >> + advertised in `NBD_INFO_BLOCK_SIZE`. If the server does not >> + advertise alternative trim limits, then the client SHOULD limit >> + `NBD_CMD_TRIM` alignment and sizes to the same limits as any >> + other command. > > "to the same limits as any other command" - may be a bit incorrect, as > we have different (not "the same") limitations for different command > (even without INFO_ZERO_SIZE (which is possible without INFO_TRIM_SIZE), > we have wording, which actually allows different > behavior and limitation for READ/WRITE and WRITE_ZERO/TRIM). So, I > think, not "the same limits as any..", but better "follow general > limitation rules", or something like this. > > Moreover, is it SHOULD? So, actually, we allow client to not limit > request at all? I think, if there are no special limits, TRIM MUST > follow general rules (really, we can say, client MUST follow the > specification (and this will include all MUST, SHOULD, etc in the spec), > but if we say client SHOULD follow the specification, it is wrong (all > MUST's will become SHOULD). > > Actually, I think we can safely drop this sentence, as it is obvious, > that this option defines additional feature, and if server didn't send > it, client MUST follow general rules, i.e., it MUST follow all other > parts of specification. You may be right about not needing that last sentence, particularly if... >> @@ -1634,7 +1692,9 @@ The following request types exist: >> a portion of the client's request is actually discarded. The >> client SHOULD NOT request a trim length of 0; the behavior of a >> server on such a request is unspecified although the server SHOULD >> - NOT disconnect. >> + NOT disconnect. The optional `NBD_INFO_TRIM_SIZE` information >> + sent during `NBD_OPT_GO` can provide more details about any trim >> + alignment constraints. > > interesting that we didn't mention here general NBD_INFO_BLOCK_SIZE > limitations ...I tweak the individual commands to refer back mention general NBD_INFO_BLOCK_SIZE constraints. I've pushed patches 1-3, and will try a v4 of this patch to tweak the wording yet again. That, and qemu has not yet implemented the proposal in this patch; so patching mainline NBD spec without an actual implementation to prove that it is easy to add may be a bit premature from the NBD spec side of things; so my v4 of this patch will be accompanied by a qemu implementation. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature