I'll review these call paths, over the next week or so: 1. the intent was to provide locking for tcp, which certainly was needed when these macros were introduced 1.1 the situation MAY be different now, but I want to be cautious 2. there is no sctp provider, so, those cases do not arise 3. vsock reuses the svc_vc provider (TCP), so should be selected in the same cases as TCP
Matt ----- Original Message ----- > From: "William Allen Simpson" <[email protected]> > To: "NFS Ganesha Developers" <[email protected]> > Sent: Saturday, March 19, 2016 1:05:28 PM > Subject: Re: [Nfs-ganesha-devel] dispatcher locking and error recovery > > On 3/18/16 2:18 PM, William Allen Simpson wrote: > > I'm trying to make sense of the error recovery (for everything, > > because it intersects with RDMA). Walking through the > > dispatcher code, I've found (again) something that makes no > > sense to me. Straightening the code for comprehension: > > > > thr_decode_rpc_request() > > > > ... > > DISP_RLOCK(xprt); > > > > ... > > if (!SVC_GETARGS(xprt, ...){ > > ... > > DISP_SLOCK2(xprt); > > svcerr_decode(xprt, ...); > > DISP_SUNLOCK(xprt); > > ... > > } > > ... > > > > finish: > > stat = SVC_STAT(xprt); > > DISP_RUNLOCK(xprt); > > > > === > > > > DISP_SLOCK2 expands as: > > > > /* For the special case of SLOCK taken from a dispatcher thread */ > > #define DISP_SLOCK2(x) \ > > do { \ > > if (!slocked) { \ > > if (!(rlocked && ((x)->xp_type == XPRT_UDP))) { \ > > SVC_LOCK((x), XP_LOCK_SEND, __func__, \ > > __LINE__); \ > > } \ > > slocked = true; \ > > } \ > > } while (0) > > > > In this case, !slocked is true, but rlocked is true and > > ((x)->xp_type == XPRT_UDP) is false (for RDMA) -- so the > > SVC_LOCK() occurs. > > > > We get no lock for UDP, but we get re-locks for all others > > (TCP, SCTP, RDMA, VSOCK). > > > > I'm pretty sure this isn't what was intended. There's no > > reason to lock here at all for TCP, because the function > > called -- libntirpc/src/svc.c svcerr_decode() -- is > > explicitly labeled MT safe. > > > > For RDMA, I've eliminated the locking code (in the ops), so > > nothing happens.... > > > > Is there any reason to lock for SCTP or VSOCK? > > > > Is the test backwards? Is there a reason to lock for UDP? > > Following up, 2 of 3 of these are paired with DISP_SUNLOCK(), > which if slocked always unlocks. > > But one instance in is_rpc_call_valid() makes even less sense. > It is paired with DISP_SUNLOCK2(): > > DISP_SLOCK2(req->rq_xprt); > svcerr_noprog(req->rq_xprt, req); > DISP_SUNLOCK2(req->rq_xprt); > > svcerr_noprog() is explicitly MT safe. > > #define DISP_SUNLOCK2(x) \ > do { \ > if (slocked) { \ > if (!(((x)->xp_type == XPRT_UDP) && !rlocked)) { \ > SVC_UNLOCK((x), XP_LOCK_SEND, __func__, \ > __LINE__); \ > } \ > slocked = false; \ > } \ > } while (0) > > If UDP it only unlocks for !rlocked. But rlocked has to be true, > or DISP_SLOCK2() wouldn't have locked. This will never happen! > Basically, it never locks or unlocks for UDP. > > If not UDP, it always unlocks (due to the "&&"). > > === > > None of this makes any rhyme nor reason. These are all error > calls. If we need to do any input or output blocking during > sending of error messages, this should be moved into ntirpc > itself (as part of its MT safeness of output). > > Agreed? > > > ------------------------------------------------------------------------------ > Transform Data into Opportunity. > Accelerate data analysis in your applications with > Intel Data Analytics Acceleration Library. > Click to learn more. > http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140 > _______________________________________________ > Nfs-ganesha-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel > -- Matt Benjamin Red Hat, Inc. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://www.redhat.com/en/technologies/storage tel. 734-707-0660 fax. 734-769-8938 cel. 734-216-5309 ------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140 _______________________________________________ Nfs-ganesha-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
