2015-02-13 16:30:10

by Chuck Lever III

Subject: rq_bc_pa_list manipulated under incorrect lock?

Hi Trond-

While reviewing xprt_complete_bc_request(), I noticed this:

326 spin_lock(&bc_serv->sv_cb_lock);

327 list_del(&req->rq_bc_pa_list); <<<<<<<

328 list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
329 wake_up(&bc_serv->sv_cb_waitq);
330 spin_unlock(&bc_serv->sv_cb_lock);

Other places in the code hold xprt->bc_pa_lock when updating
rq_bc_pa_list. Maybe not a big deal because xprt->transport_lock is
held across the lookup and complete calls?

Introduced by commit 2ea24497a1b3 ("SUNRPC: RPC callbacks
may be split across several TCP segments?).

Since I?m new to this code, maybe I misunderstood something.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2015-02-13 17:24:04

by Trond Myklebust

Subject: Re: rq_bc_pa_list manipulated under incorrect lock?

On Fri, Feb 13, 2015 at 11:30 AM, Chuck Lever <[email protected]> wrote:
>
> Hi Trond-
>
> While reviewing xprt_complete_bc_request(), I noticed this:
>
> 326 spin_lock(&bc_serv->sv_cb_lock);
>
> 327 list_del(&req->rq_bc_pa_list); <<<<<<<
>
> 328 list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
> 329 wake_up(&bc_serv->sv_cb_waitq);
> 330 spin_unlock(&bc_serv->sv_cb_lock);
>
> Other places in the code hold xprt->bc_pa_lock when updating
> rq_bc_pa_list. Maybe not a big deal because xprt->transport_lock is
> held across the lookup and complete calls?
>
> Introduced by commit 2ea24497a1b3 ("SUNRPC: RPC callbacks
> may be split across several TCP segments”).
>

Ack. That looks like a bug. It's not an actual problem as long as we
only have 1 entry on that list, but it will bite us if we ever decide
to grow the slot table...

Can you send a patch?

--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]