Date: Thu, 2 Jul 2015 19:40:15 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Jeff Layton <jlayton@poochiereds.net>
Cc: Andrey Ryabinin <a.ryabinin@samsung.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [git pull] vfs part 2
Message-ID: <20150702184015.GO17109@ZenIV.linux.org.uk>
References: <20150701082753.GD17109@ZenIV.linux.org.uk>
 <5593A7A0.6050400@samsung.com>
 <20150701085507.GE17109@ZenIV.linux.org.uk>
 <5593CE37.4070307@samsung.com>
 <20150701184408.GF17109@ZenIV.linux.org.uk>
 <20150702032042.GA32613@ZenIV.linux.org.uk>
 <20150702080026.1c32f1c7@tlielax.poochiereds.net>
 <20150702080738.615e1c52@tlielax.poochiereds.net>
 <20150702164535.GM17109@ZenIV.linux.org.uk>
 <20150702130139.35e01106@tlielax.poochiereds.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150702130139.35e01106@tlielax.poochiereds.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2787
Lines: 55

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/