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

J. Bruce Fields
2009-05-14T21:17:02Z

On Wed, May 13, 2009 at 05:42:23PM -0500, Steve Wise wrote:
> 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?

No, I think you're probably right about both of these. Thanks for
taking a look.

--b.