On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
> Fix the return value on close to -ENOTCONN so caller knows to free context.
> Also if a thread is waiting for free SQ space, check for close when waking
> to avoid posting WR to a closing transport.
A random related question: svc_rdma_send_error() seems to have only one
caller, which ignores its return value. Should its return value be
made void?
--b.
>
> Signed-off-by: Tom Tucker <[email protected]>
>
> ---
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 14f83a1..8adb2f0 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
> int ret;
>
> if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
> - return 0;
> + return -ENOTCONN;
>
> BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
> BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=
On 5/5/08 5:08 PM, "J. Bruce Fields" <[email protected]> wrote:
> On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
>> Fix the return value on close to -ENOTCONN so caller knows to free context.
>> Also if a thread is waiting for free SQ space, check for close when waking
>> to avoid posting WR to a closing transport.
>
> A random related question: svc_rdma_send_error() seems to have only one
> caller, which ignores its return value. Should its return value be
> made void?
Honestly, I don't think this code path has ever been excercised. This is the
"protocol error" path. Given my experience with the "random transport error"
tests that resulted in the patchset your currently looking at, I'd be
inclined to defer this kind of thing until I've built a "protocol error" set
of tests and worked through the "evil client" scenarios.
Thoughts?
>
> --b.
>
>>
>> Signed-off-by: Tom Tucker <[email protected]>
>>
>> ---
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 14f83a1..8adb2f0 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
>> ib_send_wr *wr)
>> int ret;
>>
>> if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
>> - return 0;
>> + return -ENOTCONN;
>>
>> BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
>> BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 05, 2008 at 09:03:02PM -0500, Tom Tucker wrote:
>
>
> On 5/5/08 5:08 PM, "J. Bruce Fields" <[email protected]> wrote:
>
> > On Fri, May 02, 2008 at 11:28:35AM -0500, Tom Tucker wrote:
> >> Fix the return value on close to -ENOTCONN so caller knows to free context.
> >> Also if a thread is waiting for free SQ space, check for close when waking
> >> to avoid posting WR to a closing transport.
> >
> > A random related question: svc_rdma_send_error() seems to have only one
> > caller, which ignores its return value. Should its return value be
> > made void?
>
> Honestly, I don't think this code path has ever been excercised. This is the
> "protocol error" path. Given my experience with the "random transport error"
> tests that resulted in the patchset your currently looking at, I'd be
> inclined to defer this kind of thing until I've built a "protocol error" set
> of tests and worked through the "evil client" scenarios.
>
> Thoughts?
None, just the observation that it's ugly to have something defined as
int svc_rdma_send_error(struct svcxprt_rdma *xprt, ...
whose only caller is
(void)svc_rdma_send_error(rdma_xprt, rmsgp, ERR_VERS);
But it's not urgent--if you want to wait till you're fixing something
related, OK.
--b.
> >
> > --b.
> >
> >>
> >> Signed-off-by: Tom Tucker <[email protected]>
> >>
> >> ---
> >> net/sunrpc/xprtrdma/svc_rdma_transport.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> index 14f83a1..8adb2f0 100644
> >> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> >> @@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
> >> ib_send_wr *wr)
> >> int ret;
> >>
> >> if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
> >> - return 0;
> >> + return -ENOTCONN;
> >>
> >> BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
> >> BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>