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
