On Thu, Jul 02, 2015 at 01:01:39PM -0400, Jeff Layton wrote: > So p9_idpool_create should take an argument for the "end" value, and > then store that in a new field in p9_idpool. Then they can pass that in > as the "end" parm in idr_alloc. Or, they could give up using the same > function there and use a different one for tags and FIDs. > > In any case...allowing this thing to allocate tag values that can > collide seems fundamentally wrong. Using idr_alloc_cyclic might also > not hurt either, particularly given that these tag values are supposed > to function something like an XID and you probably don't want to be > reusing them too quickly. All they are used for is matching response to request. Basically, you can have up to 65535 pending requests. Reusing it right after getting the response is fine. Keep in mind that it's not supposed to be used on top of something like UDP - *all* retransmits, etc., are responsibility of transport. It's connection-oriented, reliable and order-preserving, with a shitload of state tied to connection, starting with FIDs - unlike FHANDLE, FID meaning depends upon connection history. Tags are even more transient. Basically, the rules are * request bears a 16bit tag. * server can process pending requests in any order (with one exception) and it must put the same tag into responses. * client can ask to abort a pending request by issuing Tflush[old_tag]; * server must handle Tflush immediately. It must drop any pending request matching old_tag and send Rflush confirming that. No response to any request matching old_tag sent before Tflush should be issued after issuing Rflush. * if client has not issued Tflush, it must not reuse the tag until getting a response bearing that tag. * if client *has* issued Tflush, it must not reuse the tag until getting Rflush, even if it does get response to the request it has tried to abort. BTW, failure to send Tflush means that we should leave the tag in use, period. As it is, p9_client_rpc()/p9_client_zc_rpc() ignore such situations - failure from p9_client_flush() is simply not noticed. I seriously doubt that this is what we are hitting here, but it's a bug all the same. We also must _not_ let p9_client_cb() do anything unless req is non-NULL and req->status is REQ_STATUS_SENT - stray tags from server shouldn't be acted upon. As it is, the whole thing is trivial to oops - just have server send _any_ R-message with something like 0xfff0 for tag. End of story, p9_tag_lookup() returns NULL, p9_client_cb() oopses. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
