Re: [PATCH 2.6.30] svcrdma: clean up error paths.

Steve Wise
2009-05-13T22:42:15Z

Hey Bruce,

J. Bruce Fields wrote:
> On Wed, Apr 29, 2009 at 02:14:00PM -0500, Steve Wise wrote:
>
>> These fixes resolved crashes due to resource leak BUG_ON checks. The
>> resource leaks were detected by introducing asynchronous transport errors.
>>
>
> Thanks, applied for 2.6.30. (And also appropriate for stable (2.6.29),
> I assume?)
>
> But, could someone take a closer look at the error paths here? Questions:
>
> - svc_rdma_post_recv() does a svc_rdma_put_context() on error--
> are you sure its caller needs to as well?
>

The svc_rdma_put_context() call inside svc_rdma_post_recv() is for the
recv context that was allocated inside that function. The caller, in
this case send_reply() also does a svc_rdma_put_context(), but that is
for the send context. So I think this is correct.

> - In send_reply, some of the cleanout is shared between the
> first return -ENOTCONN and the final err: cleanup. Could we
> add another err: label and share some of that cleanup?
>

The only common logic I see is the svc_rdma_put_context() call that
could be shared. But one case calls it with free_pages == 1 after the
pages have been mapped, and the other with 0 since no pages are mapped
at that point (when the call to svc_rdma_post_recv() fails). So I'm
not sure its worth doing?

Steve.